XPCNativeWrapper.unwrap should work for arbitrary inputs

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bholley, Unassigned)

Tracking

({dev-doc-complete})

unspecified
mozilla23
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

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

Comment 1

6 years ago
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
Reporter

Comment 3

6 years ago
Looks green! Uploading patches and flagging for review.
Reporter

Comment 6

6 years ago
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.
Attachment #745915 - Flags: superreview?(mrbkap)
Attachment #745915 - Flags: review?(gkrizsanits)
Reporter

Comment 7

6 years ago
Posted patch Part 4 - Tests. v1 (obsolete) — Splinter Review
Attachment #745916 - Flags: review?(gkrizsanits)
Reporter

Comment 8

6 years ago
(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.
Reporter

Comment 9

6 years ago
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)
Reporter

Updated

6 years ago
Attachment #745916 - Attachment is obsolete: true
Attachment #745916 - Flags: review?(gkrizsanits)
Reporter

Updated

6 years ago
Attachment #745913 - Attachment is obsolete: true
Attachment #745913 - Flags: review?(gkrizsanits)
Reporter

Comment 11

6 years ago
This should be the API we promote going forward.
Attachment #745948 - Flags: superreview?(mrbkap)
Attachment #745948 - Flags: review?(gkrizsanits)
Reporter

Comment 12

6 years ago
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+
Reporter

Comment 16

6 years ago
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
Reporter

Comment 17

6 years ago
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?
Reporter

Comment 20

5 years ago
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)
You need to log in before you can comment on or make changes to this bug.