At this point, we use the global of the "scope" to wrap if we use the scope at all. This means that the exact identity of the "scope" is not that important; what's important is its global. But we can get _a_ global from the cx that was passed in via js::CurrentGlobalOrNull. Some auditing and a bit of fatal assertions and try runs suggest that this would match our existing behavior, in that the cx is in the compartment of "scope". Patches coming up what remove the scope argument of *Binding::Wrap and WrapObject() methods, as well as of WrapNewBindingObject. They do _not_ remove the scope arg of WrapNewBindingNonWrapperCachedObject and WrapNewBindingNonWrapperCachedOwnedObject because that's used in a nontrivial way; in particular, if scope is a security wrapper there we enter the underlying object's compartment. We may be able to get rid of this bit if/when we move to using the callee Realm instead of the this object for wrapping purposes. Peter, are you ok with the general idea? If so, Bobby can review the mechanics.

This lets us preserve some invariants about our current compartment matching the scope we want to wrap into.

Oh, and I apologize in advance for the huge mass-changes. Hopefully the sed will make it easier to review.

Comment on attachment 8401356 [details] [diff] [review] part 1. Enter the compartment of the current wrapper before we try to reparent objects. Review of attachment 8401356 [details] [diff] [review]: ----------------------------------------------------------------- Can we assert this at the top of ReparentWrapper? r=me with that.

Comment on attachment 8401384 [details] [diff] [review] part 2. Remove the "scope" argument of WrapNativeParentHelper/WrapNativeParentFallback/WrapNativeISupportsParent. Review of attachment 8401384 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +1357,5 @@ > static inline JSObject* > WrapNativeParent(JSContext* cx, JS::Handle<JSObject*> scope, T* p, > nsWrapperCache* cache, bool useXBLScope = false) > { > + MOZ_ASSERT(js::GetObjectCompartment(scope) == js::GetContextCompartment(cx)); js::IsObjectInContextCompartment. Or, if you want this to be turned on in the nightlies, AssertSameCompartment(cx, scope).

This will be folded with part 6a before pushing

This patch was generated with the following command: find . -name "*.h" -o -name "*.cpp" | xargs sed -e '/WrapNode(JSContext/ {; N; s/\(WrapNode(JSContext *\* *a\{0,1\}[Cc]x\),\n\{0,1\} *JS::Handle<JSObject\*> a\{0,1\}[sS]cope/\1/ ; }' -i ""

This will be folded with part 7a before pushing

This patch was generated with the following command: find . -name "*.h" -o -name "*.cpp" | xargs sed -e '/WrapObject(JSContext/ {; N; s/\(WrapObject(JSContext *\* *a\{0,1\}[Cc]x\),\n\{0,1\} *JS::Handle<JSObject\*> a\{0,1\}[sS]cope/\1/ ; }' -i "" and then reverting the changes that made to dom/bindings/BindingUtils.h, since those WrapObject methods are not the ones we're trying to change here.

Will be folded with part 8a. I really wish I could somehow keep them separate when pushing without breaking bisects... :(

> Can we assert this at the top of ReparentWrapper? Yes, done. > Or, if you want this to be turned on in the nightlies, AssertSameCompartment(cx, scope). I would love to turn these asserts on in nightlies, but AssertSameCompartment takes a perf hit for the function call even in non-nightlies. And even if I fixed that it would still make the benchmark numbers in nightlies suspect. The good-ish news is that many of these asserts are gone by the end of the patch stack, since the argument they are asserting about is gone. ;) I'll use AssertSameCompartment where perf is not a worry, and IsObjectInContextCompartment where it is.

> many of these asserts are gone by the end of the patch stack Oh, and the point is that I did try pushes of every stage in the stack, to make sure to run through our tests with the asserts in place.

Comment on attachment 8401394 [details] [diff] [review] part 3. Remove the "scope" argument of WrapNativeParent(). Review of attachment 8401394 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +1413,5 @@ > struct GetParentObject > { > static JSObject* Get(JSContext* cx, JS::Handle<JSObject*> obj) > { > + MOZ_ASSERT(js::GetContextCompartment(cx) == js::GetObjectCompartment(obj)); Here too. I don't really have a strong opinion about it though, so I'll stop commenting on it and let you do what you like. ::: dom/bindings/Codegen.py @@ +2565,5 @@ > ' MOZ_ASSERT(ToSupportsIsOnPrimaryInheritanceChain(aObject, aCache),\n' > ' "nsISupports must be on our primary inheritance chain");\n') > return """%s > %s > + MOZ_ASSERT(js::GetContextCompartment(aCx) == js::GetObjectCompartment(aScope)); IsObjectInContextCompartment

Comment on attachment 8401406 [details] [diff] [review] part 8b. Remove the "aScope" argument of WrapObject() methods, the non-mechanical parts. Review of attachment 8401406 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/MessagePort.cpp @@ +125,5 @@ > MessagePort* port; > if (JS_ReadBytes(reader, &port, sizeof(port))) { > JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx)); > if (global) { > + // XXXbz Can global be null? If it was, we would have crashed in the old world, right? But note that these callbacks are invoked by the JS engine, which will definitely be in a compartment. So you can remove this XXX comment. ::: dom/base/nsGlobalWindow.cpp @@ +7700,5 @@ > MessagePortBase* port; > if (JS_ReadBytes(reader, &port, sizeof(port))) { > JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx)); > if (global) { > + // XXXbz can global be null here? And this. ::: dom/base/nsJSEnvironment.cpp @@ +2761,5 @@ > dataArray.toObject()); > // Wrap it in a JS::Value. > JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx)); > if (!global) { > + // XXXbz Can this happen? It cannot. ::: dom/indexedDB/IDBObjectStore.cpp @@ +779,5 @@ > aData.name, aData.type, fileInfo.forget()); > > JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx)); > if (!global) { > + // XXXbz can this happen? It cannot. Though it is concerning that everyone seems to be checking this in structured clone callbacks. Maybe it's just cargo cult... ::: dom/workers/WorkerPrivate.cpp @@ +369,5 @@ > dataArray.toObject()); > // Wrap it in a JS::Value. > JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx)); > if (!global) { > + // XXXbz Can this actually happen? Same.

Comment on attachment 8401407 [details] [diff] [review] part 9. Remove the "scope" argument of WrapNewBindingObject. Review of attachment 8401407 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Exceptions.cpp @@ +89,5 @@ > } > > JS::Rooted<JSObject*> glob(aCx, JS::CurrentGlobalOrNull(aCx)); > if (!glob) { > + // XXXbz Can this actually be null here? That I can't tell offhand.

Comment on attachment 8401408 [details] [diff] [review] part 10. Remove the "aScope" argument from the Promise ArgumentToJSValue() methods. Review of attachment 8401408 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley modulo that. ::: dom/bindings/TypedArray.h @@ +215,2 @@ > { > + return TypedArrayType::Create(aCx, JS::NullPtr(), mArray.Length(), I can't figure out where this is defined. What's with the NullPtr() argument? Can we fix this one as well?

> IsObjectInContextCompartment To be clear, I locally changed all of those cases to use either that or AssertSameCompartment. So you can in fact stop commenting on it. > If it was, we would have crashed in the old world, right? No, would have just returned null and failed the structured clone. > Maybe it's just cargo cult... That's my impression. Mind if I just remove all those null-checks in structured cloning? > I can't figure out where this is defined. http://mxr.mozilla.org/mozilla-central/source/dom/bindings/TypedArray.h?rev=d5ac3e3f4596&force=1#138 Basically, you can pass in an object to create in that object's compartment; otherwise it creates in the context compartment. > Can we fix this one as well? Only by moving the JSAutoCompartment into the callers. Some of these are nontrivial to analyze. e.g. I think ImageData::Constructor is OK because WebIDL constructors actually enter the content compartment at the moment, but that's a very non-local property... That said, I expect we have very few consumers of this API, so it seems like it should be ok to just hoist the autocompartment to them. Most callers pass in an nsWrapperCache, in which case we _do_ want to enter the compartment of the wrapper in that cache, as we do at http://mxr.mozilla.org/mozilla-central/source/dom/bindings/TypedArray.h?rev=d5ac3e3f4596&mark=132-132&force=1#126

(In reply to Boris Zbarsky [:bz] from comment #23) > That's my impression. Mind if I just remove all those null-checks in > structured cloning? Please do. > > I can't figure out where this is defined. > > http://mxr.mozilla.org/mozilla-central/source/dom/bindings/TypedArray. > h?rev=d5ac3e3f4596&force=1#138 > > Basically, you can pass in an object to create in that object's compartment; > otherwise it creates in the context compartment. > > > Can we fix this one as well? > > Only by moving the JSAutoCompartment into the callers. Some of these are > nontrivial to analyze. e.g. I think ImageData::Constructor is OK because > WebIDL constructors actually enter the content compartment at the moment, > but that's a very non-local property... > > That said, I expect we have very few consumers of this API, so it seems like > it should be ok to just hoist the autocompartment to them. Most callers > pass in an nsWrapperCache, in which case we _do_ want to enter the > compartment of the wrapper in that cache, as we do at > http://mxr.mozilla.org/mozilla-central/source/dom/bindings/TypedArray. > h?rev=d5ac3e3f4596&mark=132-132&force=1#126 Feel free to land regardless of whatever you do here. I trust your judgement.

(In reply to Boris Zbarsky [:bz] from comment #0) > Peter, are you ok with the general idea? If so, Bobby can review the > mechanics. Yeah, afaict this is fine.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdaf616d4a60 https://hg.mozilla.org/integration/mozilla-inbound/rev/34f9e715f424 https://hg.mozilla.org/integration/mozilla-inbound/rev/37213b9688f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/c86f1317b99c https://hg.mozilla.org/integration/mozilla-inbound/rev/b94df8924248 https://hg.mozilla.org/integration/mozilla-inbound/rev/c438f7b1d1b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/56f352a5c023 https://hg.mozilla.org/integration/mozilla-inbound/rev/de7487db16d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/c51fa6ea5011 https://hg.mozilla.org/integration/mozilla-inbound/rev/2247454df4e5

https://hg.mozilla.org/mozilla-central/rev/fdaf616d4a60 https://hg.mozilla.org/mozilla-central/rev/34f9e715f424 https://hg.mozilla.org/mozilla-central/rev/37213b9688f4 https://hg.mozilla.org/mozilla-central/rev/c86f1317b99c https://hg.mozilla.org/mozilla-central/rev/b94df8924248 https://hg.mozilla.org/mozilla-central/rev/c438f7b1d1b5 https://hg.mozilla.org/mozilla-central/rev/56f352a5c023 https://hg.mozilla.org/mozilla-central/rev/de7487db16d9 https://hg.mozilla.org/mozilla-central/rev/c51fa6ea5011 https://hg.mozilla.org/mozilla-central/rev/2247454df4e5