Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 8 years ago

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.

	Bobby Holley (:bholley)
	Comment 2 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 8 years ago

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

	Pulsebot
	Comment 5 • 8 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 8 years ago
bugherder

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

	Gregor Wagner [:gwagner]
	Comment 7 • 8 years ago

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

	Gregor Wagner [:gwagner]
	Comment 8 • 8 years ago

wrong patch

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 8 years ago

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

Sorry about that.  :(

	Gregor Wagner [:gwagner]
	Comment 10 • 8 years ago

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