1155328 - OffThreadScriptReceiverCallback releases objects off the main thread when NS_DispatchToMainThread() fails

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

I found this on crash-stats.  It is similar to bug 1117977.
https://crash-stats.mozilla.com/report/index/abc9524f-81ad-4985-b15e-029d22150412

I think what is happening is that we call OffThreadScriptReceiverCallback (in nsXULElement.cpp), which creates a new runnable that has a strong reference to a mainthread only object, then NS_DispatchToMainThread() fails because we're late in shutdown, so we exit and destroy the runnable, which destroys the mainthread object.

Comment 1

[Benjamin Smedberg]

Yeah wow, there shouldn't even *be* any other threads by the time we hit this point in shutdown.

FWIW, we shouldn't fix this by changing the refcounting; we should make sure that either this thread doesn't even exist by this point in shutdown, or that it doesn't call OffThreadScriptReceiverCallback after things like xpcom-shutdown or xpcom-shutdown-threads are finished.

Comment 2

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

Attachment: Patch

Comment 3

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Yeah wow, there shouldn't even *be* any other threads by the time we hit
> this point in shutdown.
> 
> FWIW, we shouldn't fix this by changing the refcounting; we should make sure
> that either this thread doesn't even exist by this point in shutdown, or
> that it doesn't call OffThreadScriptReceiverCallback after things like
> xpcom-shutdown or xpcom-shutdown-threads are finished.

I mostly disagree.  Fixing this without changing the reference counting would require JS engine shutdown to be a two stage process (first joining all helper threads, and then later shutting down the engine itself, since JS XPCOM still needs to work for a while during shutdown).  That may be desirable for other reasons (I would like NS_DispatchToMainThread to effectively be infallible one day) but we should not block this bug on that.

Comment 4

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

There is an interesting question here of what happens if you don't JS::FinishOffThreadScript ... which this patch still doesn't do ...

Comment 5

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

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

>+    static void NoteReceiver(nsIOffThreadScriptReceiver* aReceiver) {
{ to its own line

>@@ -2773,18 +2809,17 @@ nsXULPrototypeScript::Compile(JS::Source
> 
>     if (aOffThreadReceiver && JS::CanCompileOffThread(cx, options, aSrcBuf.length())) {
>         if (!JS::CompileOffThread(cx, options,
>                                   aSrcBuf.get(), aSrcBuf.length(),
>                                   OffThreadScriptReceiverCallback,
>                                   static_cast<void*>(aOffThreadReceiver))) {
>             return NS_ERROR_OUT_OF_MEMORY;
>         }
>-        // This reference will be consumed by the NotifyOffThreadScriptCompletedRunnable.
>-        NS_ADDREF(aOffThreadReceiver);
>+        NotifyOffThreadScriptCompletedRunnable::NoteReceiver(aOffThreadReceiver);
Ok, so it doesn't matter that script is already possibly compiled at this point since release anyway happens async in the main thread.

Comment 6

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

Right, everything happens on the main thread so it's synchronized implicitly.

Comment 7

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

Wondering what bhackett thinks of comment 4 ...

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> Wondering what bhackett thinks of comment 4 ...

If JS::FinishOffThreadScript isn't called then the GC things allocated by the parse task will never be collected (the GC never collects zones used by a parse thread) and we'll also never again be able to GC the atoms compartment.  If this only occurs during shutdown then I don't think that will be a problem, but someone on the GC team might know better.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d33b8f5dfd87

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d33b8f5dfd87