Replace some AutoSafeJSContext uses with AutoJSAPI or AutoJSContext

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

We have some places where people should really not be using AutoSafeJSContext, but are.
In general, using an AutoJSAPI inited with an object is NOT the same as using
AutoSafeJSContext (or AutoJSAPI inited without an object) and then entering the
compartment of the object: the former will report exceptions to the global of
the object as it comes off the stack, while the latter will not.  This only
really matters if we have an object from a window or worker global and hence
might fire error events, or report internal stuff to the web console.

The changes to initing with an object made in this bug are OK for the following
reasons:

1) dom/base/Console.cpp: Always clears its exception before coming off the stack.
2) dom/base/nsDOMClassInfo.cpp: Inits with a non-web global.
3) dom/base/nsFrameMessageManager.cpp: Inits with a non-web global.
4) dom/media/MediaPermissionGonk.cpp: We probably want the caller to notice if
   anything here throws.
5) dom/xbl/nsXBLPrototypeBinding.cpp: Inits with a non-web global.
6) dom/xul/nsXULElement.cpp: Inits with a non-web global.
7) extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp: Inits with a non-web global.
8) ipc/testshell/XPCShellEnvironment.cpp: Inits with a non-web global.
Attachment #8731951 - Flags: review?(bobbyholley)
Comment on attachment 8731951 [details] [diff] [review]
Replace some AutoSafeJSContext uses with AutoJSAPI or AutoJSContext uses

r=me with that one fix.

Splinter is broken for this patch for some reason. :-)

>-    // Note: this unroots mScript so that it is available to be collected by the
>-    // JS GC. The receiver needs to root the script before performing a call that
>-    // could GC.
>-    JSScript *script;
>+    JS::Rooted<JSScript*> script(nsContentUtils::RootingCx());
>     {
>-        AutoSafeJSContext cx;
>-        JSAutoCompartment ac(cx, xpc::CompilationScope());
>+        AutoJSAPI jsapi;
>+        if (!jsapi.Init(xpc::CompilationScope())) {
>+            // Now what?  I guess we just leak... this should probably never
>+            // happen.
>+            return NS_ERROR_UNEXPECTED;
>+        }
>+        JSContext* cx = jsapi.cx();

I don't fully understand the caller, but the comment indicates that the lack of rooting might be intentional. Please get to the bottom of what it's talking about and figure out whether the Rooted<JSScript> is ok.
Attachment #8731951 - Flags: review?(bobbyholley) → review+
> but the comment indicates that the lack of rooting might be intentional.

The comment is about the mScript member that we have not had in a good long while.  At the time, mScript was a raw pointer rooted by the offthread stuff and the Finish* call unrooted it.  Now we have the mToken thing, which is what we pass to the Finish* call and it gives us a JSScript.
Oh, and I assume that comment is "that one fix" you meant?

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/476c67fdc36c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Posted patch followup (obsolete) — Splinter Review
Quick followup for 
13:48:23     INFO -  Unified_cpp_dom_media0.o
13:48:35     INFO -  ../../../gecko/dom/media/MediaPermissionGonk.cpp: In member function 'virtual nsresult mozilla::{anonymous}::MediaPermissionRequest::Allow(JS::HandleValue)':
13:48:35    ERROR -  ../../../gecko/dom/media/MediaPermissionGonk.cpp:247:14: error: 'class mozilla::dom::AutoJSAPI' has no member named 'init'
13:48:35     INFO -     if (!jsapi.init(&aChoices.toObject())) {
Attachment #8732607 - Flags: review?(bzbarsky)
Posted patch followupSplinter Review
wrong patch
Attachment #8732607 - Attachment is obsolete: true
Attachment #8732607 - Flags: review?(bzbarsky)
Attachment #8732608 - Flags: review?(bzbarsky)
Blocks: 1245091
Comment on attachment 8732608 [details] [diff] [review]
followup

Sorry about that.  :(
Attachment #8732608 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8732608 [details] [diff] [review]
> followup
> 
> Sorry about that.  :(

No problem!
Fabrice fixed it in bug 1258034 as well. Nothing else left to do here.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.