The default bug view has changed. See this FAQ.

XPCNativeWrapper.unwrap should work for arbitrary inputs

RESOLVED FIXED in mozilla23

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Unassigned)

Tracking

({dev-doc-complete})

unspecified
mozilla23
x86
Mac OS X
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

This defeats the whole point of XPCNativeWrapper being able to safely unwrap things that may or may not be an XrayWrapper.
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?
Summary: XPCNativeWrapper.unwrap shouldn't throw for primitive values → XPCNativeWrapper.unwrap should work for arbitrary inputs
https://tbpl.mozilla.org/?tree=Try&rev=9aaeb245d8b4
Looks green! Uploading patches and flagging for review.
Created attachment 745913 [details] [diff] [review]
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper.unwrap. v1
Attachment #745913 - Flags: review?(gkrizsanits)
Created attachment 745914 [details] [diff] [review]
Part 2 - Allow waiving on non-native objects. v1
Attachment #745914 - Flags: review?(gkrizsanits)
Created attachment 745915 [details] [diff] [review]
Part 3 - Add a global XrayWrapper constructor in addition to XPCNativeWrapper. v1

XPCNativeWrapper.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.
Attachment #745915 - Flags: superreview?(mrbkap)
Attachment #745915 - Flags: review?(gkrizsanits)
Created attachment 745916 [details] [diff] [review]
Part 4 - Tests. v1
Attachment #745916 - Flags: review?(gkrizsanits)
(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 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).
Attachment #745915 - Attachment is obsolete: true
Attachment #745915 - Flags: superreview?(mrbkap)
Attachment #745915 - Flags: review?(gkrizsanits)
Attachment #745916 - Attachment is obsolete: true
Attachment #745916 - Flags: review?(gkrizsanits)
Attachment #745913 - Attachment is obsolete: true
Attachment #745913 - Flags: review?(gkrizsanits)
Created attachment 745947 [details] [diff] [review]
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper{,.unwrap}. v2
Attachment #745947 - Flags: review?(gkrizsanits)
Created attachment 745948 [details] [diff] [review]
Part 3 - Introduce a Cu API for waiving/unwaiving. v1

This should be the API we promote going forward.
Attachment #745948 - Flags: superreview?(mrbkap)
Attachment #745948 - Flags: review?(gkrizsanits)
Created attachment 745949 [details] [diff] [review]
Part 4 - Tests. v1
Attachment #745949 - Flags: review?(gkrizsanits)
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.
Attachment #745948 - Flags: superreview?(mrbkap) → superreview+
Attachment #745947 - Flags: review?(gkrizsanits) → review+
Attachment #745914 - Flags: review?(gkrizsanits) → review+
Attachment #745949 - Flags: review?(gkrizsanits) → review+
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...
Attachment #745948 - Flags: review?(gkrizsanits) → review+
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
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.
Keywords: dev-doc-needed
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?
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.