Last Comment Bug 868675 - XPCNativeWrapper.unwrap should work for arbitrary inputs
: XPCNativeWrapper.unwrap should work for arbitrary inputs
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla23
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-03 17:34 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2014-12-04 09:24 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper.unwrap. v1 (1.10 KB, patch)
2013-05-06 09:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 2 - Allow waiving on non-native objects. v1 (1022 bytes, patch)
2013-05-06 09:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 3 - Add a global XrayWrapper constructor in addition to XPCNativeWrapper. v1 (1.96 KB, patch)
2013-05-06 09:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 4 - Tests. v1 (1.62 KB, patch)
2013-05-06 09:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper{,.unwrap}. v2 (1.94 KB, patch)
2013-05-06 10:40 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 3 - Introduce a Cu API for waiving/unwaiving. v1 (3.32 KB, patch)
2013-05-06 10:41 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
mrbkap: superreview+
Details | Diff | Splinter Review
Part 4 - Tests. v1 (1.89 KB, patch)
2013-05-06 10:41 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2013-05-03 17:34:14 PDT
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) (busy with Stylo) 2013-05-03 17:49:27 PDT
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) (busy with Stylo) 2013-05-05 22:30:06 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9aaeb245d8b4
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:36:32 PDT
Looks green! Uploading patches and flagging for review.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:43:03 PDT
Created attachment 745913 [details] [diff] [review]
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper.unwrap. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:43:14 PDT
Created attachment 745914 [details] [diff] [review]
Part 2 - Allow waiving on non-native objects. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:43:39 PDT
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.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:43:52 PDT
Created attachment 745916 [details] [diff] [review]
Part 4 - Tests. v1
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 09:44:17 PDT
(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) (busy with Stylo) 2013-05-06 09:53:11 PDT
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) (busy with Stylo) 2013-05-06 10:40:58 PDT
Created attachment 745947 [details] [diff] [review]
Part 1 - Don't throw for primitive-valued inputs to XPCNativeWrapper{,.unwrap}. v2
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 10:41:25 PDT
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.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 10:41:48 PDT
Created attachment 745949 [details] [diff] [review]
Part 4 - Tests. v1
Comment 13 Blake Kaplan (:mrbkap) 2013-05-06 16:42:47 PDT
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 [:krizsa :gabor] 2013-05-07 06:23:22 PDT
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 16 Bobby Holley (:bholley) (busy with Stylo) 2013-05-07 10:16:38 PDT
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) (busy with Stylo) 2013-05-07 16:44:22 PDT
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 20 Bobby Holley (:bholley) (busy with Stylo) 2014-12-02 21:44:23 PST
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.

Note You need to log in before you can comment on or make changes to this bug.