1257335 - Replace some AutoSafeJSContext uses with AutoJSAPI or AutoJSContext

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We have some places where people should really not be using AutoSafeJSContext, but are.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Replace some AutoSafeJSContext uses with AutoJSAPI or AutoJSContext uses

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.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and I assume that comment is "that one fix" you meant?

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/476c67fdc36c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/476c67fdc36c

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: followup

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())) {

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: followup

wrong patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8732608 [details] [diff] [review]
followup

Sorry about that.  :(

Comment 10

[Gregor Wagner [:gwagner]]

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