Closed Bug 1111737 Opened 6 years ago Closed 6 years ago
Script Loader not thread-safe ns Script Loader .cpp:178
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.
Untested, but like so.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
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.
Comment on attachment 8536725 [details] [diff] [review] Patch Please use forget() or swap()
Attachment #8536725 - Flags: review?(bzbarsky) → review+
Why would it never run?
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 ...
I can't check this in because the tree has been closed all day.
Comment on attachment 8539637 [details] [diff] [review] Patch 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.
Comment on attachment 8539637 [details] [diff] [review] Patch See above.
Comment on attachment 8539637 [details] [diff] [review] Patch 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] Patch also approving for aurora, in irc we discussed and it's getting merged without issue.
Attachment #8539637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8539637 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
You need to log in before you can comment on or make changes to this bug.