533596 - Add XPCNativeWrapper.unwrap

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Attachment: 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]]

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) (inactive)]

Comment on attachment 416826 [details] [diff] [review]
Patch

I guess this changes an interface.

Comment 4

[Nickolay_Ponomarev]

> 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) (inactive)]

(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) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ce295c1a1fe1

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Comment on attachment 416826 [details] [diff] [review]
Patch

Except the patch needs tweaking to apply to older branches.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For 1.9.2

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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]]

a=beltzner for 192 branch

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69285201ec18

Comment 13

[Daniel Veditz [:dveditz]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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]]

(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]]

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 [:bzbarsky]]

Yes, we made that change on 1.9.2 on the 19th.  See comment 12.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6b2dcb647ca

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

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]]

Documented here:

https://developer.mozilla.org/en/XPCNativeWrapper#Unwrapping_an_object

Comment 22

[Boris Zbarsky [:bzbarsky]]

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]]

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 [:bzbarsky]]

> 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]]

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)]

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?