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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

This is the stack where we crash:

https://pastebin.mozilla.org/4480803

Comment 2

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

Attachment: Patch

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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/78c1e9b63491

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a944b1d3110b (along with bug 619487and bug 968836) for m-bc bustage on at least OSX: https://tbpl.mozilla.org/php/getParsedLog.php?id=35695094&tree=Mozilla-Inbound

Comment 6

[Bobby Holley (:bholley)]

Also, looks like this had a rooting hazard: https://tbpl.mozilla.org/?tree=Try&rev=55d3b0ada798

Comment 7

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

I had to add a Rooted to make the analysis happy.

Comment 8

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

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

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

Attachment: Patch

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

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8883bce299f
https://hg.mozilla.org/releases/mozilla-beta/rev/bef53d213a54

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/bef53d213a54