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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: khuey)

Tracking

(Blocks 1 bug, {sec-moderate})

Trunk
mozilla46
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox40 affected, firefox46 fixed, firefox-esr38 ?, firefox-esr45 wontfix)

Details

(Whiteboard: [post-critsmash-triage][adv-main46+])

Attachments

(1 attachment)

Reporter

Description

4 years ago
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

4 years ago
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.

Updated

4 years ago
Group: core-security → dom-core-security
Posted patch PatchSplinter Review
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #8702667 - Flags: review?(bugs)
(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.
There is an interesting question here of what happens if you don't JS::FinishOffThreadScript ... which this patch still doesn't do ...
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.
Attachment #8702667 - Flags: review?(bugs) → review+
Right, everything happens on the main thread so it's synchronized implicitly.
Wondering what bhackett thinks of comment 4 ...
Flags: needinfo?(bhackett1024)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(bhackett1024)
https://hg.mozilla.org/mozilla-central/rev/d33b8f5dfd87
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: dom-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.