Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] --abort-on-uncaught-exception causes containsModuleSyntax to improperly throw Uncaught SyntaxError: Cannot use import statement outside a module #50878

Closed
KidkArolis opened this issue Nov 23, 2023 · 14 comments · Fixed by #50987
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.

Comments

@KidkArolis
Copy link

Version

20.10.0

Platform

Linux Alpine

Subsystem

No response

What steps will reproduce the bug?

This command works in 20.9.0, but not in 20.10.0, specifically when --abort-on-uncaught-exception opt is passed in:

$ NODE_OPTIONS="--abort-on-uncaught-exception" node_modules/.bin/knex migrate:make test

# outputs
Uncaught SyntaxError: Cannot use import statement outside a module

The version of knex:

node_modules/.bin/knex --version
Knex CLI version: 2.4.2
Knex Local version: 2.4.2

How often does it reproduce? Is there a required condition?

Fails every time!

What is the expected behavior? Why is that the expected behavior?

The migration was created by knex in 20.9.0

What do you see instead?

Now it fails with this output:

Uncaught SyntaxError: Cannot use import statement outside a module

FROM
Module._extensions..js (node:internal/modules/cjs/loader:1407:23)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/debug@4.3.4_supports-color@9.2.2/node_modules/debug/src/node.js:32:24)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/debug@4.3.4_supports-color@9.2.2/node_modules/debug/src/index.js:9:19)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/lib/execution/transaction.js:4:15)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/lib/client.js:10:21)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/lib/knex-builder/Knex.js:1:16)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/lib/index.js:1:14)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
Object.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/knex.js:8:14)
Module._compile (node:internal/modules/cjs/loader:1376:14)
Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
Module.load (node:internal/modules/cjs/loader:1207:32)
Module._load (node:internal/modules/cjs/loader:1023:12)
Module.require (node:internal/modules/cjs/loader:1235:19)
require (node:internal/modules/helpers:176:18)
initKnex (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/bin/cli.js:83:16)
async Command.<anonymous> (/app/node_modules/.pnpm/knex@2.4.2_pg@8.11.0/node_modules/knex/bin/cli.js:221:26)Trace/breakpoint trap

Additional information

No response

@KidkArolis
Copy link
Author

Let me know if you need more details on how to reproduce, I could perhaps create a small repo to show the issue if it helps.

@aduh95
Copy link
Contributor

aduh95 commented Nov 23, 2023

Could you produce a repro that doesn't include downloading any external code? If not, you should report it to the dependency you are using.

@KidkArolis
Copy link
Author

Working on reproducing it in a standalone file, but this behaviour change happens strictly if running this with Node.js 20.10.0:

# Node 20.9.0 - works
NODE_OPTIONS="--abort-on-uncaught-exception" ./node_modules/.bin/knex migrate:make test

# Node 20.10.0 - does not work
NODE_OPTIONS="--abort-on-uncaught-exception" ./node_modules/.bin/knex migrate:make test

# Node 20.10.0 - works when no flag passed
./node_modules/.bin/knex migrate:make test

@tjenkinson
Copy link

tjenkinson commented Nov 23, 2023

I've put together a repo here: https://github.com/tjenkinson/node-20.10.0-ERR_REQUIRE_ESM-crash

It seems instead of the ERR_REQUIRE_ESM error being thrown now when esm is require'd it crashes.

From the debugger it looks like it might be a crash happening in the internal containsModuleSyntax module?

@tjenkinson
Copy link

Just pushed a new commit to that repro that changes it slightly. It seems to be when it's a module in node_modules that it trips up.

@tjenkinson
Copy link

tjenkinson commented Nov 23, 2023

Looks like the crash is here:

const usesEsm = containsModuleSyntax(content, filename);

And this was added in a9b2535

@targos
Copy link
Member

targos commented Nov 23, 2023

Here's a C++ backtrace:

  * frame #0: 0x0000000102254f78 node`v8::base::OS::Abort() [inlined] v8::base::OS::Abort()::$_0::operator()(this=<unavailable>) const at platform-posix.cc:698:5 [opt]
    frame #1: 0x0000000102254f78 node`v8::base::OS::Abort() at platform-posix.cc:698:5 [opt]
    frame #2: 0x000000010095ef6c node`v8::internal::Isolate::CreateMessageOrAbort(this=<unavailable>, exception=<unavailable>, location=<unavailable>) at isolate.cc:1799:7 [opt]
    frame #3: 0x000000010095e0e4 node`v8::internal::Isolate::ThrowInternal(this=0x0000000128008000, raw_exception=Tagged<v8::internal::Object> @ x24, location=0x000000016fdfc390) at isolate.cc:1888:36 [opt]
    frame #4: 0x000000010095d9ac node`v8::internal::Isolate::ThrowAt(this=0x0000000128008000, exception=Handle<v8::internal::JSObject> @ x21, location=0x000000016fdfc390) at isolate.cc:1655:10 [opt]
    frame #5: 0x0000000101049728 node`v8::internal::PendingCompilationErrorHandler::ThrowPendingError(this=<unavailable>, isolate=0x0000000128008000, script=Handle<v8::internal::Script> @ x21) const at pending-compilation-error-handler.cc:200:12 [opt]
    frame #6: 0x00000001007e018c node`v8::internal::(anonymous namespace)::CompileToplevel(v8::internal::ParseInfo*, v8::internal::Handle<v8::internal::Script>, v8::internal::MaybeHandle<v8::internal::ScopeInfo>, v8::internal::Isolate*, v8::internal::IsCompiledScope*) [inlined] v8::internal::(anonymous namespace)::FailWithPreparedPendingException(isolate=0x0000000128008000, script=Handle<v8::internal::Script> @ x20, pending_error_handler=0x000000016fdfc750, flag=KEEP_EXCEPTION) at compiler.cc:1437:30 [opt]
    frame #7: 0x00000001007e0174 node`v8::internal::(anonymous namespace)::CompileToplevel(v8::internal::ParseInfo*, v8::internal::Handle<v8::internal::Script>, v8::internal::MaybeHandle<v8::internal::ScopeInfo>, v8::internal::Isolate*, v8::internal::IsCompiledScope*) [inlined] v8::internal::(anonymous namespace)::FailWithPendingException(isolate=0x0000000128008000, script=Handle<v8::internal::Script> @ x20, parse_info=0x000000016fdfc5b8, flag=KEEP_EXCEPTION) at compiler.cc:1449:10 [opt]
    frame #8: 0x00000001007e0174 node`v8::internal::(anonymous namespace)::CompileToplevel(parse_info=0x000000016fdfc5b8, script=Handle<v8::internal::Script> @ x20, maybe_outer_scope_info=MaybeHandle<v8::internal::ScopeInfo> @ x23, isolate=0x0000000128008000, is_compiled_scope=<unavailable>) at compiler.cc:1571:5 [opt]
    frame #9: 0x00000001007e3e30 node`v8::internal::Compiler::GetWrappedFunction(source=<unavailable>, arguments=<unavailable>, context=<unavailable>, script_details=0x000000016fdfc858, cached_data=<unavailable>, compile_options=<unavailable>, no_cache_reason=<unavailable>) at compiler.cc:3810:20 [opt]
    frame #10: 0x000000010061521c node`v8::ScriptCompiler::CompileFunctionInternal(v8_context=<unavailable>, source=0x000000016fdfcda0, arguments_count=<unavailable>, arguments=0x00006000009650e0, context_extension_count=<unavailable>, context_extensions=<unavailable>, options=kNoCompileOptions, no_cache_reason=kNoCacheNoReason, script_or_module_out=0x0000000000000000) at api.cc:2724:10 [opt]
    frame #11: 0x0000000100614bc4 node`v8::ScriptCompiler::CompileFunction(context=Local<v8::Context> @ x0, source=<unavailable>, arguments_count=<unavailable>, arguments=0x00006000009650e0, context_extension_count=<unavailable>, context_extensions=<unavailable>, options=kNoCompileOptions, no_cache_reason=<unavailable>) at api.cc:2646:10 [opt]
    frame #12: 0x000000010024a030 node`node::contextify::ContextifyContext::CompileFunctionAndCacheResult(env=0x0000000121834000, parsing_context=Local<v8::Context> @ 0x000000016fdfcb50, source=0x000000016fdfcda0, params=size=5, context_extensions=size=0, options=kNoCompileOptions, produce_cached_data=true, id_symbol=Local<v8::Symbol> @ 0x000000016fdfcb48, try_catch=0x000000016fdfccf0) at node_contextify.cc:1345:35
    frame #13: 0x000000010024695c node`node::contextify::ContextifyContext::ContainsModuleSyntax(args=0x000000016fdfd788) at node_contextify.cc:1472:3

@targos
Copy link
Member

targos commented Nov 23, 2023

The crash happens within a TryCatch scope, so it seems like a V8 bug. It shouldn't abort in this case.

@targos
Copy link
Member

targos commented Nov 23, 2023

I added some logs to the PredictExceptionCatcher and it returns here:

// Handler not found.
return NOT_CAUGHT;

/cc @nodejs/cpp-reviewers

@joyeecheung
Copy link
Member

joyeecheung commented Nov 30, 2023

This looks like a negligence from our side of not disabling aborting temporarily for the syntax check in our ShouldAbortOnUncaughtException callback.

@joyeecheung
Copy link
Member

cc @nodejs/loaders

@joyeecheung joyeecheung added the loaders Issues and PRs related to ES module loaders label Nov 30, 2023
@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. and removed loaders Issues and PRs related to ES module loaders labels Nov 30, 2023
@anonrig
Copy link
Member

anonrig commented Dec 1, 2023

@KidkArolis I opened a PR that fixes this issue.

@targos
Copy link
Member

targos commented Dec 1, 2023

@joyeecheung how do you explain that PredictExceptionCatcher doesn't find the TryCatch scope?

@joyeecheung
Copy link
Member

how do you explain that PredictExceptionCatcher doesn't find the TryCatch scope?

I think that's for JavaScript try {} catch {}, not v8::TryCatch.

@GeoffreyBooth GeoffreyBooth added the confirmed-bug Issues with confirmed bugs. label Dec 3, 2023
@GeoffreyBooth GeoffreyBooth changed the title Uncaught SyntaxError: Cannot use import statement outside a module in 20.10.0 [bug] --abort-on-uncaught-exception causes containsModuleSyntax to improperly throw Uncaught SyntaxError: Cannot use import statement outside a module Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants