1017030 - Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 5

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in ConsoleCallDataRunnable::RunConsole v1

I'm using similar reasoning to bug 988383 comment 27 here.
In that, any meaningful errors would have happened during the serialisation in PreDispatch.

I notice that the context passed to PreDispatch isn't pushed, but we're OK as that's guaranteed to be off main thread.
I also noticed that I think we could get rid of the |mWorkerPrivate->AssertIsOnWorkerThread();| in ConsoleRunnable::Dispatch as |WorkerPrivate::GetJSContext()| already calls it.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace AutoPushJSContext in ConsoleProfileRunnable::RunConsole v1

This is very similar to part 1, except for removing the way it used to get to the global for the JSAutoCompartment.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace AutoPushJSContext in PostMessageRunnable::Run v1

This one is even closer to bug 988383 part 4.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace AutoPushJSContext in nsJSContext::InitClasses v1

I'm fairly sure that because of what we are calling JS_DefineFunctions with here that it pretty much just drops down to the bottom of the function and calls DefineFunction and I don't think that cares what context it gets.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in Icc::NotifyStkEvent v1

Looks like the JSON parsing can report errors, although I'm not sure if aMessage might be guaranteed to be error free.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace AutoPushJSContext in nsDeviceStorage StringToJsval v1

As far as I can tell the only errors we could get from StringToJsval are OOM ones.

And this was the patch that prompted my question about JSObject*. :-)

Comment 7

[Bob Owen (:bobowen)]

Here's an old try push (I accidentally merged that last two patches):
https://tbpl.mozilla.org/?tree=Try&rev=9d6fd600eb5c

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8438517 [details] [diff] [review]
Part 3: Replace AutoPushJSContext in PostMessageRunnable::Run v1

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

::: dom/base/MessagePort.cpp
@@ +279,4 @@
>  
> +  AutoJSAPI jsapi;
> +  JSContext* cx = jsapi.cx();
> +  JSAutoCompartment ac(cx, globalObject->GetGlobalJSObject());

Note that in the non-window case, this switches us from using the SafeJSContext global to using the actual global. I think that's the right way to go, though.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8438535 [details] [diff] [review]
Part 6: Replace AutoPushJSContext in nsDeviceStorage StringToJsval v1

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1703,4 @@
>      return JSVAL_NULL;
>    }
>  
> +  AutoJSAPI jsapi;

The fact that the AutoJSAPI here exists between the naked JSObject* and the JSAutoCompartment might be flagged as a rooting hazard by the analysis (run browser rooting analysis in your trychooser syntax to see).

sfink was looking at this exact issue, so you should talk to him and see where he is about it. Just reordering and putting the AutoJSAPI at the top is probably enough to fix this though.

Comment 10

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 8438535 [details] [diff] [review]

> The fact that the AutoJSAPI here exists between the naked JSObject* and the
> JSAutoCompartment might be flagged as a rooting hazard by the analysis (run
> browser rooting analysis in your trychooser syntax to see).
> 
> sfink was looking at this exact issue, so you should talk to him and see
> where he is about it. Just reordering and putting the AutoJSAPI at the top
> is probably enough to fix this though.

Ah, OK, I'll kick one off once the trees are open.

Wouldn't it be nice if the Global JSObject had it's own derived type?
It might help in situations like this and maybe it would have other uses.

Thanks for all the reviews.
Might be tomorrow before I get those first fixed Bluetooth patches up for bug 1023969, as I can't test my fixes on try and I've never tried building B2G locally.

Comment 11

[Bob Owen (:bobowen)]

try push including the rooting analysis looks good:
https://tbpl.mozilla.org/?tree=Try&rev=dc2bc024f53d

Landing in part number order please.
Thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75855f82feae
https://hg.mozilla.org/integration/mozilla-inbound/rev/43e89a29e47b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8486fe4272ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed77ac7d2c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64357012f74
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d572d9513ce

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/75855f82feae
https://hg.mozilla.org/mozilla-central/rev/43e89a29e47b
https://hg.mozilla.org/mozilla-central/rev/8486fe4272ce
https://hg.mozilla.org/mozilla-central/rev/ed77ac7d2c62
https://hg.mozilla.org/mozilla-central/rev/b64357012f74
https://hg.mozilla.org/mozilla-central/rev/6d572d9513ce