Last Comment Bug 533596 - Add XPCNativeWrapper.unwrap
: Add XPCNativeWrapper.unwrap
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 541742
Blocks: 547176 blazinglyfastthis
  Show dependency treegraph
 
Reported: 2009-12-08 16:48 PST by Blake Kaplan (:mrbkap)
Modified: 2011-06-27 05:21 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wanted


Attachments
Patch (10.44 KB, patch)
2009-12-09 16:04 PST, Blake Kaplan (:mrbkap)
peterv: review+
jst: superreview+
Details | Diff | Splinter Review
For 1.9.2 (11.53 KB, patch)
2009-12-22 17:19 PST, Blake Kaplan (:mrbkap)
mbeltzner: approval1.9.2.2-
Details | Diff | Splinter Review
For 1.9.1 (10.41 KB, patch)
2010-01-13 11:22 PST, Blake Kaplan (:mrbkap)
jst: review+
bzbarsky: superreview-
dveditz: approval1.9.1.8-
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2009-12-08 16:48:37 PST
This is meant to take the place of the incorrect construction in extensions:
  if (obj.wrappedJSObject)
     obj = obj.wrappedJSObject;
to try to unwrap an XPCNativeWrapper.

The proposed usage is:
  obj = XPCNativeWrapper.unwrap(obj);
Comment 1 Blake Kaplan (:mrbkap) 2009-12-09 16:04:34 PST
Created attachment 416826 [details] [diff] [review]
Patch

The size of this patch is rather significantly increased by the testing stuff that I needed to add.
Comment 2 Peter Van der Beken [:peterv] 2009-12-11 17:31:09 PST
Comment on attachment 416826 [details] [diff] [review]
Patch

>+nsDOMWindowUtils::GetClassName(char **aName)
>+  // first and second params must be JSObjects

There's only one param?
Comment 3 Blake Kaplan (:mrbkap) 2009-12-14 14:30:36 PST
Comment on attachment 416826 [details] [diff] [review]
Patch

I guess this changes an interface.
Comment 4 Nickolay_Ponomarev 2009-12-22 07:35:11 PST
> incorrect construction in extensions
In what sense is it incorrect? Because having wrappedJSObject does not mean the object is XPCNativeWrapper-wrapped and because it can point to a different object (in the case of JS-implemented XPCOM components, XPCWrappedNative around XPCWrappedJS, right)? Any other reasons?

When documenting this it would be helpful to answer when one might need this (e.g. should I use unwrap() instead of just accessing .wrappedJSObject when I know I'm dealing with XPCNW?), and what to expect of the call (the stuff in the tests).
Comment 5 Blake Kaplan (:mrbkap) 2009-12-22 16:49:44 PST
(In reply to comment #4)
> In what sense is it incorrect? Because having wrappedJSObject does not mean the
> object is XPCNativeWrapper-wrapped and because it can point to a different
> object (in the case of JS-implemented XPCOM components, XPCWrappedNative around
> XPCWrappedJS, right)? Any other reasons?

Also, if you have a SJOW already (e.g. you already unwrapped your object), then getting the .wrappedJSObject could get an entirely unrelated object. This might not be a problem right now, but with the new wrappers (that I'll write a blog post about), if you expose a COW (chrome object wrapper) to content and it sticks a property on your underlying object (which you have to explicitly allow) that is itself an object, there is currently no reliable way to test to see if that object is an XPCNativeWrapper or SJOW.

> When documenting this it would be helpful to answer when one might need this
> (e.g. should I use unwrap() instead of just accessing .wrappedJSObject when I
> know I'm dealing with XPCNW?), and what to expect of the call (the stuff in the
> tests).

Yeah, it really comes down to whether or not you know that you have an XPCNativeWrapper. If you do, then you don't have to use unwrap().
Comment 6 Blake Kaplan (:mrbkap) 2009-12-22 16:58:05 PST
http://hg.mozilla.org/mozilla-central/rev/ce295c1a1fe1
Comment 7 Blake Kaplan (:mrbkap) 2009-12-22 17:04:34 PST
Comment on attachment 416826 [details] [diff] [review]
Patch

It would be good to get this in on older branches too, I think.
Comment 8 Blake Kaplan (:mrbkap) 2009-12-22 17:19:08 PST
Comment on attachment 416826 [details] [diff] [review]
Patch

Except the patch needs tweaking to apply to older branches.
Comment 9 Blake Kaplan (:mrbkap) 2009-12-22 17:19:33 PST
Created attachment 418946 [details] [diff] [review]
For 1.9.2
Comment 10 Blake Kaplan (:mrbkap) 2010-01-13 11:22:22 PST
Created attachment 421487 [details] [diff] [review]
For 1.9.1

I think we should get this new API in on as many branches as possible.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-19 10:18:11 PST
a=beltzner for 192 branch
Comment 12 Blake Kaplan (:mrbkap) 2010-01-19 15:43:50 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69285201ec18
Comment 13 Daniel Veditz [:dveditz] 2010-01-22 13:33:05 PST
Comment on attachment 421487 [details] [diff] [review]
For 1.9.1

>-[scriptable, uuid(a2d8d4f8-6082-4653-b91d-f958518b6ada)]
>+[scriptable, uuid(3c155d09-efc1-4fb5-ba54-834fcb09a2c5)]
> interface nsIDOMWindowUtils : nsISupports {

Can we change this interface on a stable branch? Don't we need a nsIDOMWindowUtils_MOZILLA_1_9_1_BRANCH ?

requesting additional sr=bzbarsky to get an opinion on the above.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2010-01-22 19:46:05 PST
So...  How is this supposed to work, exactly?  It creates an XPCWrappedJS around an XPCNativeWrapper, not an XPCWrappedJS around the underlying unsafe object, unless I'm seriously missing something.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-01-22 19:53:06 PST
Comment on attachment 421487 [details] [diff] [review]
For 1.9.1

Yeah, we need a new interface.  And we need to get whether it works sorted out.
Comment 16 Daniel Veditz [:dveditz] 2010-01-25 10:35:12 PST
(In reply to comment #15)
> Yeah, we need a new interface. 

Wait, we made the same change on the 1.9.2 branch after we shipped. So do we need to backout and make an interface there as well?
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-25 10:35:50 PST
Uhhh, crap. Did we change the interface on mozilla-1.9.2, as well, which is now a stable branch? If so, we should back that out and quickly, before Fennec spins an RC. This may have been my fault. :(
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2010-01-25 10:54:04 PST
Yes, we made that change on 1.9.2 on the 19th.  See comment 12.
Comment 19 Blake Kaplan (:mrbkap) 2010-01-25 13:39:32 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6b2dcb647ca
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-26 09:55:25 PST
Comment on attachment 418946 [details] [diff] [review]
For 1.9.2

I think we will want a branch safe patch for this, yes.
Comment 21 Eric Shepherd [:sheppy] 2010-08-23 13:19:06 PDT
Documented here:

https://developer.mozilla.org/en/XPCNativeWrapper#Unwrapping_an_object
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2010-11-27 19:10:59 PST
The documentation is wrong; this was backed out from 1.9.2, so isn't present there.  It was never landed in 1.9.1.
Comment 23 Eric Shepherd [:sheppy] 2010-11-28 11:20:49 PST
The RESOLVED FIXED status of this bug is misleading if it's been backed out, fwiw. Documentation has had that bit deleted now.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2010-11-28 12:04:22 PST
> The RESOLVED FIXED status of this bug is misleading if it's been backed out,

It's been backed out from the 1.9.2 branch; note the lack of a "fixed on branch" flag.

This is fixed on trunk, and we still need documentation about this being the right way to unwrap on trunk (and in Firefox 4.0).

One of these days our bug system will make it clear what's fixed where... ;)
Comment 25 Eric Shepherd [:sheppy] 2010-11-29 07:57:46 PST
No wonder I was confused! I've put the text back, and changed the "Introduced in Gecko 1.9.2.2" to say Gecko 2.0 (Firefox 4) instead.
Comment 26 Henri Sivonen (:hsivonen) 2011-06-27 05:21:24 PDT
My about:blank patch in attachment 542132 [details] [diff] [review] break the mochitest here by making the "must see expandos for a chrome sandbox" asserts fail. It seems that at the time of failure, the right XUL doc is in the docshell (based on document.documentURI).

Where should I look? Could the window object of the initial about:blank get reused with the wrong kinds of security properties or something?

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