1111737 - crash nsScriptLoader not thread-safe nsScriptLoader.cpp:178

Status: RESOLVED FIXED

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

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er, yes.  This should proxy-release on the main thread, because there's no guarantee about which thread a runnable is destroyed on.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Looks like a standard "runnable that doesn't realize its dtor can run on either thread" bug.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Untested, but like so.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8536725 [details] [diff] [review]
Patch

Please use forget() or swap()

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8536725 [details] [diff] [review]
Patch

What if the runnable never runs?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why would it never run?

Comment 8

[Boris Zbarsky [:bzbarsky]]

During shutdown?  Just because the thread gets shut down before it gets a chance to?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 ...

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I can't check this in because the tree has been closed all day.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8539637 [details] [diff] [review]
Patch

See above.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc84f5e110f

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8539637 [details] [diff] [review]
Patch

also approving for aurora, in irc we discussed and it's getting merged without issue.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6cfb21ae8405
https://hg.mozilla.org/releases/mozilla-beta/rev/9c908bced0db

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fc84f5e110f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/95e3f8386eb6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/74489778f365
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fc1373048808

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/fc1373048808