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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
Created attachment 8438508 [details] [diff] [review]
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.
Attachment #8438508 - Flags: review?(bobbyholley)
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Created attachment 8438511 [details] [diff] [review]
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.
Attachment #8438511 - Flags: review?(bobbyholley)
Created attachment 8438517 [details] [diff] [review]
Part 3: Replace AutoPushJSContext in PostMessageRunnable::Run v1

This one is even closer to bug 988383 part 4.
Attachment #8438517 - Flags: review?(bobbyholley)
Created attachment 8438527 [details] [diff] [review]
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.
Attachment #8438527 - Flags: review?(bobbyholley)
Created attachment 8438532 [details] [diff] [review]
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.
Attachment #8438532 - Flags: review?(bobbyholley)
Created attachment 8438535 [details] [diff] [review]
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*. :-)
Attachment #8438535 - Flags: review?(bobbyholley)
Here's an old try push (I accidentally merged that last two patches):
https://tbpl.mozilla.org/?tree=Try&rev=9d6fd600eb5c
Attachment #8438508 - Flags: review?(bobbyholley) → review+
Attachment #8438511 - Flags: review?(bobbyholley) → review+
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.
Attachment #8438517 - Flags: review?(bobbyholley) → review+
Attachment #8438527 - Flags: review?(bobbyholley) → review+
Attachment #8438532 - Flags: review?(bobbyholley) → review+
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.
Attachment #8438535 - Flags: review?(bobbyholley) → review+
(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.
try push including the rooting analysis looks good:
https://tbpl.mozilla.org/?tree=Try&rev=dc2bc024f53d

Landing in part number order please.
Thanks.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.