Closed Bug 1017030 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(6 files)

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
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
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)
This one is even closer to bug 988383 part 4.
Attachment #8438517 - Flags: review?(bobbyholley)
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)
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)
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.