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

Segmentation fault with import() instead of calling importModuleDynamically #35889

Closed
Tracked by #37648
nicolo-ribaudo opened this issue Oct 30, 2020 · 37 comments
Closed
Tracked by #37648
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. vm Issues and PRs related to the vm subsystem.

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Oct 30, 2020

  • Version: 14.15.0, 15.0.1. It doesn't crash with 12.9.0.
  • Platform: Linux nicolo-XPS-15-9570 5.4.0-52-generic #57-Ubuntu SMP Thu Oct 15 10:57:00 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: vm

What steps will reproduce the bug?

https://github.com/nicolo-ribaudo/babel/tree/node-segfault

I'm sorry but I can't create a smaller reproduction example (EDIT: #35889 (comment)): I managed to create a small test that reproduces the crash almost always, but it still far from being "self contained".

  1. Run make bootstrap to install dependencies and compile everything
  2. Run node --experimental-vm-modules ./node_modules/.bin/jest -i babel-core/test/segfault to see the segfault (try 2-3 times, sometimes the first run doesn't fail).

That test will run this file: I have placed a debugger; statement so that you can --inspect-brk it. After debugger, if you move into the import() call, it will crash.

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

Almost 100%

What is the expected behavior?

import() should call Jest's importModuleDynamically function.

What do you see instead?

@SimenB tried debugging this segfault and extracted this stacktrace (babel/babel#12288 (comment)):

PID 43445 received SIGSEGV for address: 0x0
0   segfault-handler.node               0x00000001055e3fa0 _ZL16segfault_handleriP9__siginfoPv + 304
1   libsystem_platform.dylib            0x00007fff6951f5fd _sigtramp + 29
2   ???                                 0x0000000000000007 0x0 + 7
3   node                                0x000000010033df4a _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEE + 138
4   node                                0x00000001006f82f4 _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE + 340
5   node                                0x0000000100a797b4 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit + 52
[1]    43445 segmentation fault  node --experimental-vm-modules --inspect-brk ./node_modules/.bin/jest 

Additional information

  • When the test doesn't crash, it throws that file:///home/nicolo/Documenti/dev/babel/babel/packages/babel-core/test/fixtures/example.mjs doesn't exist even if it does. However, this might be caused by Jest?
  • If you can't reproduce the bug, you can try running node --experimental-vm-modules ./node_modules/.bin/jest -i babel-core/test/config-chain which is how I originally discovered this issue. You can stop right before crashing by adding a debugger; right before the compiled version (which will be generated in packages/babel-core/lib/config/files/import.js) of this import() call.

Similar bugs

@SimenB
Copy link
Member

SimenB commented Oct 30, 2020

  • When the test doesn't crash, it throws that file:///home/nicolo/Documenti/dev/babel/babel/packages/babel-core/test/fixtures/example.mjs doesn't exist even if it does. However, this might be caused by Jest?

Yes, Jest doesn't deal with file URLs, that's a bug that should be simple to fix. I can fix that right away.

EDIT: jestjs/jest#10744

@nicolo-ribaudo "Subsystem" is vm, btw

EDIT2: For anyone looking into this, the importModuleDynamically call is implemented here: https://github.com/facebook/jest/blob/2b748f67c25615a111330017a2bffc0baf51d558/packages/jest-runtime/src/index.ts#L1176-L1182

@SimenB
Copy link
Member

SimenB commented Nov 18, 2020

@devsnek sorry to ping you, but do you have any idea of the top of your head for what we could be doing wrong in Jest? It's probably something that should be tweaked in Node regardless since it segfaults, but I'm also quite certain it's Jest doing something unexpected 🙂

@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Dec 24, 2020

If it helps, I created a minimal (almost, it still uses Jest) reproduction: https://github.com/nicolo-ribaudo/node-segfault-jest-dynamic-import

Also @bmeck kindly told me that this is probably a known v8 issue, but I couldn't find it in the v8 bug tracker 😅


EDIT: I created a minimal reproduction without any dependency, but it's the first time I use the vm module so I might have done something wrong. https://github.com/nicolo-ribaudo/node-vm-dynamic-import-segfault

@bmeck
Copy link
Member

bmeck commented Dec 24, 2020

It is from the VM module dealing with https://bugs.chromium.org/p/v8/issues/detail?id=10284 , which means that these fake modules from source text alone don't get properly resolved and it segfaults

@daniele-orlando
Copy link

daniele-orlando commented Sep 27, 2021

I can reproduce this bug on Node v16.9.1 using Webpack and Babel configured with ESM configuration files.

PID 19496 received SIGSEGV for address: 0x18
0   segfault-handler.node               0x0000000105b630aa _ZL16segfault_handleriP9__siginfoPv + 298
1   libsystem_platform.dylib            0x00007fff20399d7d _sigtramp + 29
2   ???                                 0x00002afc24f31c6b 0x0 + 47262440037483
3   node                                0x000000010314ada6 _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEENS0_11MaybeHandleIS5_EE + 218
4   node                                0x000000010340c3de _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE + 329
5   node                                0x0000000103696b54 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit + 52
6   node                                0x00000001037223ce Builtins_CallRuntimeHandler + 78
7   node                                0x0000000103629fea Builtins_InterpreterEntryTrampoline + 202
zsh: segmentation fault  node --trace-event-categories v8,node,node.async_hooks 

@devsnek
Copy link
Member

devsnek commented Sep 27, 2021

v8 has landed a partial fix for this in that they check for host_defined_options in the cache now, but this causes a large amount of cache misses and extra memory usage in both chrome and node, so they're still working on it.

@alexander-akait
Copy link

To be honest, I am very upset with this problem, in fact it blocks the transition very much on ESM modules, for large projects it is very difficult to just take and translate the entire codebase to ESM, but small projects started to transition, and in order to keep versions up to day we need to use import(), but once we use it we cannot test anymore, jest just failed with Segmentation fault. I am incredibly surprised that this problem is still not on the top priority list.

It's not just about updating dependencies. This actually blocks the ecosystem from transitioning smoothly. Also, old package versions stop receiving updates, which we can potentially lead to security problems.

I really hope that the problem will be fixed in the near future.

@suspiciousfellow
Copy link

Wouldn't describe myself as "very upset" per se, but it is stopping us from transitioning too atm

@AlonMiz
Copy link

AlonMiz commented Nov 11, 2021

this is a critical issue for our organization as well.
facing it while testing with jest.
jestjs/jest#11438

@benmccann
Copy link
Contributor

v8 has landed a partial fix for this in that they check for host_defined_options in the cache now, but this causes a large amount of cache misses and extra memory usage in both chrome and node, so they're still working on it.

If I'm reading the issue correctly then it sounds like that change was reverted and things are still broken. And that bug can't be actively worked on at the moment because there's a blocker.

@alexander-akait
Copy link

Maybe be need to ping somebody from V8 here...

@suspiciousfellow
Copy link

Worth mentioning that it isn't only Jest that is affected too - our test framework which is built on top of mocha cannot run and furthermore am being unable to (practicably) upgrade to latest version of dependencies and get a completely rosy npm audit output when groovy people like e.g. @sindresorhus are taking everything to ESM...

@SimenB
Copy link
Member

SimenB commented Sep 14, 2023

@joyeecheung this can be closed as well 🙂

@joyeecheung
Copy link
Member

Closing as #48510 has landed and should fix this. We can re-open if that turns out to be incorrect (hopefully not).

ryanwilsonperkin added a commit to ryanwilsonperkin/unimported that referenced this issue Sep 22, 2023
Consistently running into a segfault coming from Node as a result of
cosmiconfig attempting to run a dynamic import on the listed .js config
file caused by:

nodejs/node#35889
jestjs/jest#11438

Using cosmiconfigSync to workaround this which will fall back to using a
synchronous require() call.
smeijer added a commit to smeijer/unimported that referenced this issue Sep 22, 2023
Fixes #118 

Introduces `cosmiconfig` as requested in #118 in order to support
loading the config from alternative file formats like:
```
.unimportedrc.js
.unimportedrc.yml
package.json > "unimported" key
```

I'm using `cosmiconfigSync` utilities instead of the async version,
despite them being available since the `getConfig` is an async method.
When we use the async equivalent, we hit a segfault in Node as a result
of `cosmiconfig` trying to call a dynamic import on the file within a
Jest context that doesn't allow it to. Patched in a recent version of
Node, and once the test infra can require the latest version it should
be fine to switch to the async version.

See nodejs/node#35889 and
jestjs/jest#11438

I haven't made any efforts to change the `update` function to write
updates to the loaded files, as this would be difficult/impossible to
update something like a .js or .yml (if using features like anchors) in
a meaningful way.

A few notes on the other changes included:
1. `cosmiconfig` pulls in a newer version of TypeScript which was
incompatible with the version of `@types/node` we used, updated it
2. One of the tests produced invalid JSON to a config file, which failed
silently before and passed the test, but now fails loudly when
`cosmiconfig` tries to read and parse the JSON. Updated it to be valid
to fulfill the spirit of the test.

---------

Co-authored-by: Stephan Meijer <stephan.meijer@gmail.com>
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 3, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 5, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 5, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 5, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 5, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 5, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 6, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
joachimvh added a commit to CommunitySolidServer/CommunitySolidServer that referenced this issue Oct 6, 2023
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests