Segfault in 16.3.0 with promise hooks #39019
Comments
|
@stephenh - do you have a core dump produced out of the crash that I can have a look at? |
|
@gireeshpunathil yep, attached! |
(gdb) x/12i 0xD0CFB0
0xd0cfb0 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_>: push rbp
0xd0cfb1 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+1>: mov rbp,rsp
0xd0cfb4 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+4>: push r15
0xd0cfb6 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+6>: push r14
0xd0cfb8 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+8>: push r13
0xd0cfba <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+10>: push r12
0xd0cfbc <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+12>: mov r12,rdi
0xd0cfbf <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+15>: push rbx
0xd0cfc0 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+16>: mov rbx,r8
0xd0cfc3 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+19>: sub rsp,0x18
=> 0xd0cfc7 <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+23>: mov rax,QWORD PTR [rdi]
0xd0cfca <_ZN2v87Context15SetPromiseHooksENS_5LocalINS_8FunctionEEES3_S3_S3_+26>: and rax,0xfffffffffffc0000
(gdb) i r rdi
rdi 0x0 0
(gdb) basically, the /cc @nodejs/v8 |
|
There are known issues regarding PromiseHooks in 16.2.0 and 16.3.0. Latest fix was #38912 Above fix is already in master but not on 16.x yet. Could you test with a nightly build? |
Flarna
added
the
async_hooks
|
@Flarna sure, I just tried a nightly, assuming I did it right, and still got a core dump: I have the 6/13 nightly: Running this test segfaults: I got this core dump file: Which I'm attaching (...er, I'm getting a github error about "not in the list", but I'm uploading a zip file which worked before...still poking at it...) |
|
I can't seem to upload either a zip or a tar.gz file, and the error looks kinda weird/not display correctedly, in both firefox and chrome:
Not sure how I was able to upload the first one, it just worked... |
|
may be that this issue is already loaded with a heavy core file, and github is capping on size limit? |
|
Okay, I've put the core dump in this other repo (which was for reproducing a prior async hooks issue, the one in 16.2.0, not this current issue, I'm just borrowing that repository as a place to shove this file): https://github.com/stephenh/async-local-storage-repro/blob/main/core-nightly-0613.tar.gz |
|
This is the corresponding function. I don't see anything jumping out at me about it. 🤔 https://github.com/v8/v8/blob/master/src/api/api.cc#L6314-L6351 |
|
I suspect the issue may have been introduced by #38821. I think it's trying to do a |
|
I can attempt a bisect. @stephenh, the reproduce repo that is linked - it has different steps and results, but I could not locate the one that produce the above crash. Can you guide me how do I reproduce the crash? |
|
@gireeshpunathil sorry, I don't have a repro for this one yet :-/ That repository was for reproducing #38821, and I just borrowed it for this issue's coredump. For #38821, it was failing ~most/all of our tests, so it is was fairly easy to scope down to a few lines of code. This issue fails maybe ~10% of our tests (but fails those tests deterministically, which is at least good), but I haven't had a chance to dig into why. At first I thought it was b/c of a particularly complex test, but later I noticed it is also segfaulting what I thought was a relatively simple test (we use (...erg, last time we were also able to reproduce this in the ORM's public test suite, but it looks like the public test suite is passing on node 16.3.0, so it's only our internal application that is failing here...) |
Thanks for this note. My team encountered the same Segfault call stack as Stephen when running |
|
@karanbirsingh - thanks for the additional info, and the repro repo; unfortunately, I am unable to reproduce in 100 iterations. I always see the test passing. $ yarn docker:run
$ docker run -t demo-segfault
PASS ./sum.test.js
test
✓ test (7 ms)
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 1 passed, 1 total
Time: 0.557 s
Ran all test suites.
Done in 2.84s.
$any other info, that may be crucial here? |
|
I am able to reproduce the issue @karanbirsingh has via the repro repo. Here's my branch of that, with some notes. It seems to happen with either node 16.3.0 or nightly, but only sometimes, typically on the first run in a running container. The backtrace is similar (but shorter) to the one provided by @stephenh. Here are From @stephenh's core dumpFrom @karanbirsingh's repro |
|
I was able to narrow this down to the following snippet to reproduce. It works for me on both Linux and macOS, with v16.3.0. // flags: --expose-gc
const asyncHooks = require('async_hooks');
const vm = require('vm');
const hook = asyncHooks.createHook({ init() {} }).enable();
vm.createContext();
gc();
hook.disable();It looks like weak references to the contexts are still being held in in the For reference, here are Lines 234 to 267 in cd43073 And here is Lines 98 to 110 in cd43073 This code was introduced by #38821. |
|
Seems very strange to me that the context would be collected before the ContextifyScript destructor. 🤔 |
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: #39019 PR-URL: #39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: #39019 PR-URL: #39095 Backport-PR-URL: #38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: #39019 PR-URL: #39095 Backport-PR-URL: #38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: #39019 PR-URL: #39095 Backport-PR-URL: #38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Backport-PR-URL: nodejs#38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
v16.3.0Linux sh12 5.11.0-18-generic #19-Ubuntu SMP Fri May 7 14:22:03 UTC 2021 x86_64 x86_64 x86_64 GNU/LinuxWhat steps will reproduce the bug?
I cannot reproduce it outside of our internal codebase yet.
How often does it reproduce? Is there a required condition?
It deterministically reproduces in the test I am running, but I cannot reproduce it outside of that yet
What do you see instead?
A seg fault:
Additional information
cc'ing @Qard because of the
SetPromiseHooksin the above segfault.Sorry I don't have any useful reproductions yet :-( will keep poking at it.
The text was updated successfully, but these errors were encountered: