868675 - XPCNativeWrapper.unwrap should work for arbitrary inputs

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This defeats the whole point of XPCNativeWrapper being able to safely unwrap things that may or may not be an XrayWrapper.

Comment 1

[Bobby Holley (:bholley)]

Actually, let's expand this a bit. It would be great if XPCNativeWrapper.unwrap could work for pure JS objects (currently it only works for XrayWrappers).

And maybe we should even create an XrayWrapper object in addition and slowly transition over?

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9aaeb245d8b4

Comment 3

[Bobby Holley (:bholley)]

Looks green! Uploading patches and flagging for review.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper.unwrap. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Allow waiving on non-native objects. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a global XrayWrapper constructor in addition to XPCNativeWrapper. v1

PCNativeWrapper.unwrap is here to stay, and I cringe every time I tell people
to use it. The current name imposes a non-trivial cost of making all this stuff
seem more confusing and magical. Now that we're no longer defining this stuff in
content scopes, I think we can swallow the chrome scope pollution and just do this.
We can always back it out if need be.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 8

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #6)
> Created attachment 745915 [details] [diff] [review]
> Part 3 - Add a global XrayWrapper constructor in addition to
> XPCNativeWrapper. v1

FWIW, bz was in favor of this.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 745915 [details] [diff] [review]
Part 3 - Add a global XrayWrapper constructor in addition to XPCNativeWrapper. v1

gabor and I talked about this, and I've been convinced to put it on Cu, mostly because it gives us more descriptive and consistent naming (especially with the existing Cu.isXrayWrapper).

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper{,.unwrap}. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce a Cu API for waiving/unwaiving. v1

This should be the API we promote going forward.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 745948 [details] [diff] [review]
Part 3 - Introduce a Cu API for waiving/unwaiving. v1

Review of attachment 745948 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/idl/xpccomponents.idl
@@ +411,5 @@
> +    [implicit_jscontext]
> +    jsval waiveXrays(in jsval aVal);
> +
> +    /**
> +      * Strips off Xray waivers on a given value. Identity op for primitives.

Nit: Here and above, the second line of *s should line up with the first * on the first line.

@@ +417,5 @@
> +    [implicit_jscontext]
> +    jsval unwaiveXrays(in jsval aVal);
> +
> +    /**
> +      * Strips any Xray waivers on a given input.

This doesn't seem to be attached to anything.

::: js/xpconnect/src/XPCComponents.cpp
@@ +4616,5 @@
> +    if (!aVal.isObject()) {
> +        *aRetval = aVal;
> +        return NS_OK;
> +    }
> +    *aRetval = ObjectValue(*js::UncheckedUnwrap(&aVal.toObject()));

Nit: add a newline above this assignment.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 745948 [details] [diff] [review]
Part 3 - Introduce a Cu API for waiving/unwaiving. v1

Review of attachment 745948 [details] [diff] [review]:
-----------------------------------------------------------------

I would add only one more thing to the nits: make sure we update all the documentation to point to this new API. In the meanwhile we could also flag the old version as deprecated...

Comment 15

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8821628e2a68
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e690c384582a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/90a789485fbb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b440e4f094c

Comment 16

[Bobby Holley (:bholley)]

Flagging dev-doc-needed.

XPCNativeWrapper is an old, deprecated name for the concept that is now known as XrayWrapper. 

Historically, we've promoted the use of the XPCNativeWrapper function (in global scope) to unwaive Xray, and XPCNativeWrapper.unwrap to waive Xray. XPCNativeWrapper.unwrap is often preferable to .wrappedJSObject, because it works even if the object isn't an Xray waiver (the other patches in this bug make that behavior more robust as well).

We never renamed this function because of the compatibility pain of adding/removing functions in global scope. With this bug, we now have a more consistent API that lives on Components.utils in order to manage Xrays:

* Cu.isXrayWrapper(obj) - returns true if the object is an Xray wrapper.

* Cu.waiveXrays(obj) - identity operation for primitives, otherwise returns a wrapper that transitively waives Xray behavior on the underlying object and anything that comes off the object. Waiving Xrays gives you a transparent wrapper to the underlying JS object.

* Cu.unwaiveXrays(obj) - Strips off any waivers from obj, giving the caller its default wrapper for the object (which, in the case of native objects from unprivileged scopes, is an XrayWrapper).

This API is generally better, because making XPCNativeWrapper a global constructor is confusing, since it implies that Xrays are not the default (which they are).

Feel free to follow up with me in this bug or by email or by IRC if there are any further questions from the doc team.

Comment 17

[Bobby Holley (:bholley)]

I just realized that we probably can't fully deprecate XPCNativeWrapper{,.unwrap} because XBL needs it occasionally, and XBL can't see Cu.

Also, gabor, I'm realizing that jetpack content scripts are another major problem here, whether or not we eventually run them with nsEPs. Maybe we should make them a nice API that uses XPCNativeWrapper.unwrap internally?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8821628e2a68
https://hg.mozilla.org/mozilla-central/rev/e690c384582a
https://hg.mozilla.org/mozilla-central/rev/90a789485fbb
https://hg.mozilla.org/mozilla-central/rev/7b440e4f094c

Comment 19

[Will Bamberg [:wbamberg]]

I've added pages for this API:
https://developer.mozilla.org/en-US/docs/Components.utils.isXrayWrapper
https://developer.mozilla.org/en-US/docs/Components.utils.waiveXrays
https://developer.mozilla.org/en-US/docs/Components.utils.unwaiveXrays

Let me know if this is OK.

Comment 20

[Bobby Holley (:bholley)]

Looks good. I would mention that Cu.waiveXrays is like .wrappedJSObject (which many people know about), but more useful, because it passes through non-Xray objects and primitives rather than throwing.

> Just like waiveXrays, unwaiveXrays is transitive: it unwaives Xrays for all properties of the object (and > their properties, and so on).

This is a tiny bit confusing - it's not so much that the unwaiving is transitive itself, but just that it undoes a transitively-extended operation, and returns things to the default behavior. Not a big deal though.