Closed Bug 1111737 Opened 6 years ago Closed 6 years ago

crash nsScriptLoader not thread-safe nsScriptLoader.cpp:178


(Core :: DOM: Core & HTML, defect)

32 Branch
Not set



Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
firefox-esr31 35+ fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: mcmanus, Assigned: khuey)


(Keywords: sec-moderate, Whiteboard: [adv-main35+][adv-esr31.4+])


(1 file, 1 obsolete file)

during shutdown .. release off main thread

(gdb) bt
#0  0x00007f710150699d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f7101506834 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f7106f000f6 in ah_crap_handler (signum=11) at /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsSigHandlers.cpp:101
#3  0x00007f7106f001fb in child_ah_crap_handler (signum=11) at /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsSigHandlers.cpp:113
#4  0x00007f7107da51b9 in AsmJSFaultHandler (signum=11, info=0x7f70f5afc470, context=0x7f70f5afc340) at /home/mcmanus/src/mozilla2/wd/gecko-dev/js/src/asmjs/AsmJSSignalHandlers.cpp:912
#5  <signal handler called>
#6  nsScriptLoader::Release (this=0x7f70f10bc080) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:178
#7  0x00007f7104011cdd in nsRefPtr<nsScriptLoader>::~nsRefPtr (this=0x7f70ef090c30) at ../../dist/include/nsRefPtr.h:59
#8  0x00007f7104706e05 in (anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable::~NotifyOffThreadScriptLoadCompletedRunnable (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:788
#9  0x00007f7104706e49 in (anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable::~NotifyOffThreadScriptLoadCompletedRunnable (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:788
#10 0x00007f71030209e9 in nsRunnable::Release (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/glue/nsThreadUtils.cpp:32
#11 0x00007f71046f4944 in nsRefPtr<(anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable>::~nsRefPtr (this=0x7f70f5afca38) at ../../dist/include/nsRefPtr.h:59
#12 0x00007f71046f47bd in OffThreadScriptLoaderCallback (aToken=0x7f70eb5c3e60, aCallbackData=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:847
#13 0x00007f71085d9dd2 in js::HelperThread::handleParseWorkload (this=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:1182
#14 0x00007f71085d8cba in js::HelperThread::threadLoop (this=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:1367
#15 0x00007f71085d71b7 in js::HelperThread::ThreadMain (arg=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:977
#16 0x00007f710b7b298b in _pt_root (arg=0x7f70f9c5e800) at ../../../../../gecko-dev/nsprpub/pr/src/pthreads/ptthread.c:212
#17 0x00007f710b3c30a5 in start_thread (arg=0x7f70f5afd700) at pthread_create.c:309
#18 0x00007f710153f88d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
Er, yes.  This should proxy-release on the main thread, because there's no guarantee about which thread a runnable is destroyed on.
Looks like a standard "runnable that doesn't realize its dtor can run on either thread" bug.
Attached patch Patch (obsolete) — Splinter Review
Untested, but like so.
Assignee: nobody → khuey
Attachment #8536725 - Flags: review?(bzbarsky)
I suspect the reason we haven't seen this before is that processing the script takes a while.  At shutdown we probably skip that.  This could in theory be exploitable.
Group: core-security
Comment on attachment 8536725 [details] [diff] [review]

Please use forget() or swap()
Attachment #8536725 - Flags: review?(bzbarsky) → review+
Comment on attachment 8536725 [details] [diff] [review]

What if the runnable never runs?
During shutdown?  Just because the thread gets shut down before it gets a chance to?
More verbosely, NS_DispatchToMainThread works until after xpcom-shutdown-threads ... and I really hope the JS engine is joining threads that could call into Gecko by then ...
Attached patch PatchSplinter Review
Attachment #8536725 - Attachment is obsolete: true
I can't check this in because the tree has been closed all day.
Keywords: checkin-needed
Comment on attachment 8539637 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Bug 906371
[User impact if declined]: Possible crashes/security issues
[Describe test coverage new/current, TBPL]: This is the script loading path, it's pretty well tested.
[Risks and why]: Low risk.  This makes us explicitly release things on the thread we already virtually always release them on.
[String/UUID change made/needed]: None.
Attachment #8539637 - Flags: approval-mozilla-beta?
Attachment #8539637 - Flags: approval-mozilla-aurora?
Comment on attachment 8539637 [details] [diff] [review]

See above.
Attachment #8539637 - Flags: approval-mozilla-esr31?
Attachment #8539637 - Flags: approval-mozilla-b2g34?
Attachment #8539637 - Flags: approval-mozilla-b2g32?
Comment on attachment 8539637 [details] [diff] [review]

Approving now for Beta since we only have one beta this week and it would be best to have this in the build, out there getting tested by users.
Attachment #8539637 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8539637 [details] [diff] [review]

also approving for aurora, in irc we discussed and it's getting merged without issue.
Attachment #8539637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8539637 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [adv-main35+][adv-esr31.4+]
Attachment #8539637 - Flags: approval-mozilla-b2g34?
Attachment #8539637 - Flags: approval-mozilla-b2g34+
Attachment #8539637 - Flags: approval-mozilla-b2g32?
Attachment #8539637 - Flags: approval-mozilla-b2g32+
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.