Goal is to remove it from dom::WrapObject.

Comment on attachment 8403821 [details] [diff] [review] part 1. Remove aScope from nsContentUtils::WrapObject. Review of attachment 8403821 [details] [diff] [review]: ----------------------------------------------------------------- This patch is misnamed. r=bholley with comments addressed. ::: dom/base/nsJSEnvironment.cpp @@ +1277,5 @@ > AutoFree iidGuard(iid); // Free iid upon destruction. > > JS::Rooted<JSObject*> global(cx, GetWindowProxy()); > JS::Rooted<JS::Value> v(cx); > + JSAutoCompartment ac(cx, global); Pre-existing, but this isn't actually a global at all. ::: dom/bluetooth/BluetoothManager.cpp @@ +76,5 @@ > > AutoPushJSContext cx(sc->GetNativeContext()); > > JS::Rooted<JSObject*> global(cx, sc->GetWindowProxy()); > + JSAutoCompartment ac(cx, global); Same here. ::: dom/file/ArchiveRequest.cpp @@ +195,5 @@ > NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY); > > + if (NS_FAILED(rv) || > + !JS_DefineElement(aCx, array, i, JS::StringValue(str), nullptr, nullptr, > + JSPROP_ENUMERATE)) { I don't understand why you're making this change, here and below. ::: dom/indexedDB/AsyncConnectionHelper.cpp @@ +124,5 @@ > nsRefPtr<IDBWrapperCache> wrapper = static_cast<IDBWrapperCache*>(mRequest); > JS::Rooted<JSObject*> global(aCx, wrapper->GetParentObject()); > NS_ASSERTION(global, "This should never be null!"); > > + JSAutoCompartment ac(aCx, global); Given that aResult is immediately propagated out of scope, I don't think we need the previous 4 lines at all here. ::: dom/xbl/nsXBLProtoImplMethod.cpp @@ +297,5 @@ > > JS::Rooted<JS::Value> v(cx); > + { > + // Scope for entering the compartment of globalObject > + JSAutoCompartment ac(cx, globalObject); Why do we need this scope?

Comment on attachment 8403823 [details] [diff] [review] part 3. Remove the scope argument of the classinfo WrapNative methods. Review of attachment 8403823 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/base/nsDOMClassInfo.cpp @@ +2009,5 @@ > } > > nsCOMPtr<nsIDOMWindow> currentWin(do_GetInterface(currentInner)); > + { // scope for JSAutoCompartment: we want to wrap the window > + // in the scope of obj, not of thisObject How would it make any difference at all? @@ +3060,5 @@ > } else { > scope = aWin->GetGlobalJSObject(); > } > > + JSAutoCompartment ac(cx, scope); Same here. Basically, we detect in WrapperFactory whether PreCreate actually knows what its scope is. If it doesn't, we create an entirely new WN in the wrap callback. So we should never need to enter a compartment just to wrap an nsISupports, at least for XPCWNs. Does that ever happen for the WebIDL case? @@ +3292,2 @@ > JS::Rooted<JS::Value> v(cx); > + { // Scope for the JSAutoCompartment Same here. Location definitely knows its scope.

Comment on attachment 8403826 [details] [diff] [review] part 5. Remove the "creator" argument of the version of TypedArray::Create that takes a JSObject* creator. Review of attachment 8403826 [details] [diff] [review]: ----------------------------------------------------------------- Not that it affects this patch too much, but this is probably one of the places where we want a stronger story for Xrays (c/f bug 991661).

> Pre-existing, but this isn't actually a global at all. I'll rename to "scope". > I don't understand why you're making this change, here and below. Because I couldn't deal with the obviously buggy code that was there before.... It's not strictly needed for this bug. I can pull it out into a separate bug if you'd prefer. > Given that aResult is immediately propagated out of scope, I don't think we need the > previous 4 lines at all here. I'm not following what you mean here. > Why do we need this scope? We could have two different JSAutoCompartment on the stack in this method instead (ac and ac2 or something), but this seemed cleaner to me: it scopes entering the compartment of globalObject to just the wrapping. Or is the point that we should just move "scopeObject" and its JSAutoCompartment higher up and wrap directly into that compartment? I could buy that. > How would it make any difference at all? Sounds like it doesn't, good. I'll remove that JSAutoCompartment and the JS_WrapValue bit there. > Does that ever happen for the WebIDL case? WebIDL things generally return something useful from GetParentObject(), so know their own scope.

Comment on attachment 8403827 [details] [diff] [review] part 6. Remove the "scope" argument of dom::WrapObject methods. Review of attachment 8403827 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/document/src/nsHTMLDocument.cpp @@ +2258,5 @@ > > JS::Rooted<JS::Value> val(cx); > { // Scope for auto-compartment > JS::Rooted<JSObject*> wrapper(cx, GetWrapper()); > JSAutoCompartment ac(cx, wrapper); We probably don't need this AutoCompartment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea41acd470b https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2585d38bc0 https://hg.mozilla.org/integration/mozilla-inbound/rev/07db28f7c6a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc114a391a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb2d9a7e383 https://hg.mozilla.org/integration/mozilla-inbound/rev/50bb1cfc2352 with the comments addressed.

https://hg.mozilla.org/mozilla-central/rev/9ea41acd470b https://hg.mozilla.org/mozilla-central/rev/ab2585d38bc0 https://hg.mozilla.org/mozilla-central/rev/07db28f7c6a5 https://hg.mozilla.org/mozilla-central/rev/9dc114a391a2 https://hg.mozilla.org/mozilla-central/rev/0cb2d9a7e383 https://hg.mozilla.org/mozilla-central/rev/50bb1cfc2352

Comment on attachment 8403822 [details] [diff] [review] part 2. Remove the duplicated WrapNative methods in HTMLAllCollection. Review of attachment 8403822 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this; I meant to get back and remove those much sooner.