There are about 45 callers to audit/change here. Quite a lot, but fortunately most of them are in the "not a CCW" bucket. The plan (as before) is to introduce a new API that asserts non-CCW and then convert callers incrementally.

js::GetJSMEnvironmentOfScriptedCaller returns either nullptr or a NonSyntacticVariablesObject.

FWIW, at this point there are about 20 GetGlobalForObjectCrossCompartment callers left.

Comment on attachment 8989946 [details] [diff] [review] Part 1 - Use JS::GetNonCCWObjectGlobal in Codegen.py r=me

Comment on attachment 8989947 [details] [diff] [review] Part 2 - Use JS::GetNonCCWObjectGlobal in some functions where we unwrapped the object r=me

Comment on attachment 8989949 [details] [diff] [review] Part 3 - Remove some GetGlobalForObjectCrossCompartment calls on globals/WindowProxy >@@ -1174,25 +1174,16 @@ XPCOMObjectToJsval(JSContext* cx, JS::Ha Why is the assert here being removed? r=me with this assert left in.

Comment on attachment 8989967 [details] [diff] [review] Part 4 - Use JS::GetNonCCWObjectGlobal in PrefableDisablers::isEnabled r=me

Comment on attachment 8989978 [details] [diff] [review] Part 5 - Use JS::CurrentGlobalOrNull in JS shell clone() function r=me

Comment on attachment 8989980 [details] [diff] [review] Part 6 - Use JS::GetNonCCWObjectGlobal in subscript loader r=me

Comment on attachment 8989982 [details] [diff] [review] Part 7 - Use JS::GetNonCCWObjectGlobal in mozJSComponentLoader::FindTargetObject r=me

Comment on attachment 8989989 [details] [diff] [review] Part 8 - Use JS::GetNonCCWObjectGlobal in XrayAwareCalleeGlobal r=me

Comment on attachment 8989992 [details] [diff] [review] Part 9 - Use JS::GetNonCCWObjectGlobal in worker wrap-callback r=me

Thanks for all the reviews! (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13) > >@@ -1174,25 +1174,16 @@ XPCOMObjectToJsval(JSContext* cx, JS::Ha > > Why is the assert here being removed? > > r=me with this assert left in. What do you want this assert to look like? If I change: js::GetGlobalForObjectCrossCompartment(jsobj) == jsobj to: JS_IsGlobalObject(jsobj) Then the assert in the then-branch is always true because this is exactly the definition of JS_IsGlobalObject: NS_ASSERTION(js::GetObjectClass(jsobj)->flags & JSCLASS_IS_GLOBAL, That's why I removed it..

Hmm. I guess that used to assert things about js::GetObjectParent back when that was a thing. In that case, we can just make this function do: return NativeInterface2JSObjectAndThrowIfFailed(.....); right?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21) > In that case, we can just make this function do: > > return NativeInterface2JSObjectAndThrowIfFailed(.....); > > right? Works for me :)

Things are now becoming a bit harder (for me) to reason about. I'll probably deal with the remaining callers in follow-up bugs. The patches here are all green on Try btw.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c62e8a6080 part 1 - Use JS::GetNonCCWObjectGlobal in Codegen.py. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/7049feb994ee part 2 - Use JS::GetNonCCWObjectGlobal in some functions where we unwrapped the object. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcad8d82168 part 3 - Remove some GetGlobalForObjectCrossCompartment calls on globals/WindowProxy. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/aaaa74019db7 part 4 - Use JS::GetNonCCWObjectGlobal in PrefableDisablers::isEnabled. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/9d097737be52 part 5 - Use JS::CurrentGlobalOrNull in JS shell clone() function. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/21f5ed1760b9 part 6 - Use JS::GetNonCCWObjectGlobal in subscript loader. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/def39aedc2a6 part 7 - Use JS::GetNonCCWObjectGlobal in mozJSComponentLoader::FindTargetObject. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/6462a8c5ad53 part 8 - Use JS::GetNonCCWObjectGlobal in XrayAwareCalleeGlobal. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e17b68432e95 part 9 - Use JS::GetNonCCWObjectGlobal in worker wrap-callback. r=bz

Comment on attachment 8990261 [details] [diff] [review] Part 10 - Use JS::GetNonCCWObjectGlobal in DOMRequest::Then We should add a diagnostic assert to Promise::CreateFromExisting that aPromiseObj is JS::IsPromiseObject. I _think_ that's true, but am not 100% sure.

Comment on attachment 8990263 [details] [diff] [review] Part 11 - Use JS::GetNonCCWObjectGlobal in nsIDocument::SetScriptGlobalObject r=me

Comment on attachment 8990264 [details] [diff] [review] Part 12 - Use JS::GetNonCCWObjectGlobal in CheckForOutdatedParent r=me

Comment on attachment 8990265 [details] [diff] [review] Part 13 - Use JS::GetNonCCWObjectGlobal in BindingUtils >+ JS::GetNonCCWObjectGlobal(&aArgs.callee()); So I _think_ this is OK, because for Xrays we create a separate JSFunction, except for constructors, and those don't use this code... Pretty subtle, though. >@@ -1659,17 +1659,17 @@ FindAssociatedGlobal(JSContext* cx, T* p > JSObject* obj = WrapNativeHelper<T>::Wrap(cx, p, cache); This one is also subtle. It does always return a non-CCW object, but that object may not be in the current compartment of cx. Worth documenting. r=me

Comment on attachment 8990267 [details] [diff] [review] Part 14 - Use JS::GetNonCCWObjectGlobal in IndexedDatabaseManager::ExperimentalFeaturesEnabled IndexedDatabaseManager::ExperimentalFeaturesEnabled is only called from PrefableDisablers::isEnabled, so should only get globals passed to start with, right? We can probably add a diagnostic assert that aGlobal is in fact a global and then take out the "get a global" call. r=me with that.

Comment on attachment 8990268 [details] [diff] [review] Part 15 - Use JS::GetNonCCWObjectGlobal in SandboxCallableProxyHandler::call r=me

https://hg.mozilla.org/mozilla-central/rev/a8c62e8a6080 https://hg.mozilla.org/mozilla-central/rev/7049feb994ee https://hg.mozilla.org/mozilla-central/rev/5bcad8d82168 https://hg.mozilla.org/mozilla-central/rev/aaaa74019db7 https://hg.mozilla.org/mozilla-central/rev/9d097737be52 https://hg.mozilla.org/mozilla-central/rev/21f5ed1760b9 https://hg.mozilla.org/mozilla-central/rev/def39aedc2a6 https://hg.mozilla.org/mozilla-central/rev/6462a8c5ad53 https://hg.mozilla.org/mozilla-central/rev/e17b68432e95

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31) > We should add a diagnostic assert to Promise::CreateFromExisting that > aPromiseObj is JS::IsPromiseObject. I _think_ that's true, but am not 100% > sure. I get some orange on Try with the following stack: * mozilla::dom::Promise::CreateFromExisting * mozilla::dom::Promise::Resolve * mozilla::dom::TestFunctions_Binding::passThroughPromise ... I think JS::CallOriginalPromiseResolve can return a wrapped promise here: https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#2341-2349 For DOMRequest::Then, my justification was that we always create a new DOM + JS Promise here, without ever wrapping it AFAICS: https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/dom/base/DOMRequest.cpp#180

You're right. We do apparently create dom::Promise around CCWs for promises... You're also right that for the DOMRequest case this can't happen.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5dded93548e6 part 10 - Use JS::GetNonCCWObjectGlobal in DOMRequest::Then. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e0247ffaa0e9 part 11 - Use JS::GetNonCCWObjectGlobal in nsIDocument::SetScriptGlobalObject. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0a0805ce4f part 12 - Use JS::GetNonCCWObjectGlobal in CheckForOutdatedParent. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7444af18cf part 13 - Use JS::GetNonCCWObjectGlobal in BindingUtils. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/314d501fe26f part 14 - Use JS::GetNonCCWObjectGlobal in IndexedDatabaseManager::ExperimentalFeaturesEnabled. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/c4dce7639098 part 15 - Use JS::GetNonCCWObjectGlobal in SandboxCallableProxyHandler::call. r=bz

https://hg.mozilla.org/mozilla-central/rev/5dded93548e6 https://hg.mozilla.org/mozilla-central/rev/e0247ffaa0e9 https://hg.mozilla.org/mozilla-central/rev/2a0a0805ce4f https://hg.mozilla.org/mozilla-central/rev/bf7444af18cf https://hg.mozilla.org/mozilla-central/rev/314d501fe26f https://hg.mozilla.org/mozilla-central/rev/c4dce7639098

Current status on m-i tip is there are 9 js::GetGlobalForObjectCrossCompartment calls left: * 1 in xpc::NativeGlobal. Bug 1474272 is dealing with this. * 2 in XPCWrappedJSClass. Can definitely be CCWs. I have a patch that's green on Try, but I need to double check the code. * 3 in dom/xbl. Some of these are likely on CCWs. I'll file a separate bug. * 1 related one in GetXBLScopeOrGlobal (this has 1 or 2 problematic callers I think). * 2 in XrayWrapper.cpp. These are definitely on wrappers. So the main ones are XBL and Xrays. I'll look into these today and file separate bugs.

When the dependent bugs land, there will be no uses left.

Comment on attachment 8996614 [details] [diff] [review] Part 16 - Remove js::GetGlobalForObjectCrossCompartment Review of attachment 8996614 [details] [diff] [review]: ----------------------------------------------------------------- \o/

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6b15672ed7 part 16 - Remove js::GetGlobalForObjectCrossCompartment. r=luke

This whole sub tree of bugs turned out to be bigger and more difficult than I expected. Really glad it's done :)