991742 - Remove the need for a "scope" argument when wrapping objects

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Enter the compartment of the current wrapper before we try to reparent objects.

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the "scope" argument of WrapNativeParentHelper/WrapNativeParentFallback/WrapNativeISupportsParent.

Comment 4

[Bobby Holley (:bholley)]

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 5

[Bobby Holley (:bholley)]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the "scope" argument of WrapNativeParent().

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove the "scope" argument of WrapCallThisObject().

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove use of the "aScope" argument in binding Wrap() methods.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6a. Remove the "aScope" argument of binding Wrap() methods, the non-mechanical parts.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6b. Remove the "aScope" argument of binding Wrap() methods, the mechanical parts.

This will be folded with part 6a before pushing

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7a. Remove the "aScope" argument of WrapNode() methods, the mechanical parts.

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7b. Remove the "aScope" argument of WrapNode() methods, the non-mechanical parts.

This will be folded with part 7a before pushing

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8a. Remove the "aScope" argument of WebIDL/nsWrapperCache WrapObject() methods, the mechanical parts.

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8b. Remove the "aScope" argument of WrapObject() methods, the non-mechanical parts.

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove the "scope" argument of WrapNewBindingObject.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove the "aScope" argument from the Promise ArgumentToJSValue() methods.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[Bobby Holley (:bholley)]

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 20

[Bobby Holley (:bholley)]

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 21

[Bobby Holley (:bholley)]

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 22

[Bobby Holley (:bholley)]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> 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

Comment 24

[Bobby Holley (:bholley)]

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

Comment 25

[Peter Van der Beken [:peterv]]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Carsten Book [:Tomcat]]

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