Closed Bug 979951 Opened 6 years ago Closed 6 years ago

Worker Runnables can end up running and creating objects in the SafeJSContext global

Categories

(Core :: DOM: Workers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 + fixed
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: bholley, Assigned: khuey)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

In bug 979481, I'm changing the SafeJSContext such that pushing it resets its compartment to null, rather than to the compartment of the dummy SafeJSContext global. There were two oranges - one of them the Settings API, the other one in Workers, via browser/devtools/shared/test/browser_telemetry_button_tilt.js.

The basic issue is that the TILT_CRAFTER chrome worker is parented to a main thread global that is not an nsIDOMWindow, so mWindow and mScriptContext are null. So we end up using the SafeJSContext.

In WorkerRunnable.cpp, we try to find a default compartment object to enter, using GetWrapper(). But this can return null if the wrapper has been GCed:

http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerRunnable.cpp#300

Before bug 979481, the SafeJSContext defaults to the SafeJSContext compartment, so we silently do the wrong thing. Afterwards, its defaults compartment is null, so we assert.

It seems like we should replace (or augment) mWindow with some sort of |nsIGlobalObject* mMainThreadGlobal|, which would always be non-null. Kyle and Ben didn't have an immediate consensus on what this should look like, so I'm punting this to Kyle and taking myself out of the loop.
This is the stack where we crash:

https://pastebin.mozilla.org/4480803
Flags: needinfo?(khuey)
Attached patch Patch (obsolete) — Splinter Review
I think we should land this for now.  We're seeing this pattern where we want to be able to recreate a wrapper at will from C++ appear a lot.  I think we need a more generic solution here in nsWrapperCache or something.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #8386457 - Flags: review?(bent.mozilla)
Flags: needinfo?(khuey)
Comment on attachment 8386457 [details] [diff] [review]
Patch

Review of attachment 8386457 [details] [diff] [review]:
-----------------------------------------------------------------

This should be ok for beta as long as these are addressed:

::: dom/workers/WorkerPrivate.cpp
@@ +2142,5 @@
>  
>    AssertIsOnParentThread();
>  
> +  JSObject* wrapper =
> +    WorkerBinding::Wrap(aCx, aScope, ParentAsWorkerPrivate());

Can you call TryPreserveWrapper if this returns null? It's probably safer to nullcheck right?

@@ +2143,5 @@
>    AssertIsOnParentThread();
>  
> +  JSObject* wrapper =
> +    WorkerBinding::Wrap(aCx, aScope, ParentAsWorkerPrivate());
> +  TryPreserveWrapper(wrapper);

If I'm reading the comments right this should be MOZ_ALWAYS_TRUE(Try...)
Attachment #8386457 - Flags: review?(bent.mozilla) → review+
Also, looks like this had a rooting hazard: https://tbpl.mozilla.org/?tree=Try&rev=55d3b0ada798
I had to add a Rooted to make the analysis happy.
https://hg.mozilla.org/mozilla-central/rev/b077aaea25e7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attached patch PatchSplinter Review
If it's not too late I'd like to slip this into beta to avoid shipping a release where web workers may not work if the GC runs at exactly the right time.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 928312
User impact if declined: If the website uses web workers in odd ways, they may not work properly.
Testing completed (on m-c, etc.): On m-c, trivial patch.
Risk to taking this patch (and alternatives if risky): Very low risk.  This essentially restores the behavior we have had in Gecko 27 and lower.
String or IDL/UUID changes made by this patch: N/A
Attachment #8386457 - Attachment is obsolete: true
Attachment #8386831 - Flags: review+
Attachment #8386831 - Flags: approval-mozilla-beta?
Attachment #8386831 - Flags: approval-mozilla-aurora?
Attachment #8386831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is a regression from 27 and would cause website breakage in ways we can't tell the impact of at this late stage - will approve the uplifts in a sec.
Attachment #8386831 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.