Closed
Bug 1341749
Opened 7 years ago
Closed 7 years ago
Segfault in wasm error throwing
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: azakai, Assigned: luke)
References
Details
Attachments
(3 files, 1 obsolete file)
The attached testcase segfaults an optimized build of spidermonkey. Looks like it happens when a promise has an error, (gdb) where #0 0x0000000000000000 in ?? () #1 0x000000000093c424 in js::HelperThread::handlePromiseTaskWorkload (this=this@entry=0x7ffff6b693c0, locked=...) at /home/alon/Dev/mozilla-central/js/src/vm/HelperThreads.cpp:1501 #2 0x000000000093d594 in js::HelperThread::threadLoop (this=0x7ffff6b693c0) at /home/alon/Dev/mozilla-central/js/src/vm/HelperThreads.cpp:1940 #3 0x000000000094bf92 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul> (this=0x7ffff6b161b0) at /home/alon/Dev/mozilla-central/js/src/threading/Thread.h:234 #4 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0x7ffff6b161b0) at /home/alon/Dev/mozilla-central/js/src/threading/Thread.h:227 #5 0x00007ffff7bc16ba in start_thread (arg=0x7ffff3020700) at pthread_create.c:333 #6 0x00007ffff6e5282d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Reporter | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Fortunately, this seems to just be a shell-only race between the main() thread quitting and tearing down the runtime while the async promise is still finishing up. Gecko does a final join and I thought that SM did too... Anyhow, this doesn't need to be s-s.
Assignee | ||
Comment 3•7 years ago
|
||
Updated•7 years ago
|
Group: core-security
Comment 4•7 years ago
|
||
Comment on attachment 8840127 [details] [diff] [review] drain-job-queue-before-quit Review of attachment 8840127 [details] [diff] [review]: ----------------------------------------------------------------- There's one call to DrainJobQueue in ReadEvalPrintLoop, which might be the one you were thinking of. Can you remove it there, maybe? Thanks for the patch.
Attachment #8840127 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #4) > There's one call to DrainJobQueue in ReadEvalPrintLoop, which might be the > one you were thinking of. Can you remove it there, maybe? Thanks for the > patch. Hmm, that one looks meaningful: it ensures that jobs are drained after each line is entered in the interactive command prompt.
Comment 6•7 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #5) > (In reply to Benjamin Bouvier [:bbouvier] from comment #4) > > There's one call to DrainJobQueue in ReadEvalPrintLoop, which might be the > > one you were thinking of. Can you remove it there, maybe? Thanks for the > > patch. > > Hmm, that one looks meaningful: it ensures that jobs are drained after each > line is entered in the interactive command prompt. OK, so to avoid repetition, maybe we can just move the check from ReadEvalPrintLoop() to the end of Process(), which handles all the cases?
Assignee | ||
Comment 7•7 years ago
|
||
Ok, so reading around more I realized there already was a DrainJobQueue() in ProcessArgs() and that the root bug here is that it isn't called on error (b/c we return false early). Calling this always with MakeScopeExit() doesn't work because DrainJobQueue() can run JS handlers and you can't do that with an exception pending. So the first patch was actually right to put it in Shell(), right after the AutoReportExceptions has cleared the pending exception. Now with test!
Attachment #8840127 -
Attachment is obsolete: true
Attachment #8840911 -
Flags: review?(bbouvier)
Comment 8•7 years ago
|
||
Comment on attachment 8840911 [details] [diff] [review] drain-job-queue-on-error Review of attachment 8840911 [details] [diff] [review]: ----------------------------------------------------------------- Wow, interesting.
Attachment #8840911 -
Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe85f9809f82 Drain job queue even after error (r=bbouvier)
Comment 10•7 years ago
|
||
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/525fc9bb16c7 Backout test (r=red)
Assignee | ||
Comment 11•7 years ago
|
||
So unfortunately, it seems like we don't have the ability to write tests with the `|jit-test| error:X` directive since, on platforms where !wasmIsSupported(), wasm.js will quit() (with a success message). Rather than scrambling some hack in the test harness that would likely break on some other platform, I just backed out this test for now.
Comment 12•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe85f9809f82 https://hg.mozilla.org/mozilla-central/rev/525fc9bb16c7 Landed 15 hours ago.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•