Closed Bug 1341749 Opened 7 years ago Closed 7 years ago

Segfault in wasm error throwing

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: azakai, Assigned: luke)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file src.cpp.o.js
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
Attached file src.cpp.o.wasm
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.
Attached patch drain-job-queue-before-quit (obsolete) — Splinter Review
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8840127 - Flags: review?(bbouvier)
Group: core-security
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+
(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.
(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?
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 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)
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.
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.