Closed Bug 533596 Opened 10 years ago Closed 10 years ago

Add XPCNativeWrapper.unwrap

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.1 --- wanted

People

(Reporter: mrbkap, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

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);
Attached patch PatchSplinter Review
The size of this patch is rather significantly increased by the testing stuff that I needed to add.
Attachment #416826 - Flags: review?(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?
Attachment #416826 - Flags: review?(peterv) → review+
Comment on attachment 416826 [details] [diff] [review]
Patch

I guess this changes an interface.
Attachment #416826 - Flags: superreview?(jst)
Attachment #416826 - Flags: superreview?(jst) → superreview+
> 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).
Keywords: dev-doc-needed
(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().
http://hg.mozilla.org/mozilla-central/rev/ce295c1a1fe1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 416826 [details] [diff] [review]
Patch

It would be good to get this in on older branches too, I think.
Attachment #416826 - Flags: approval1.9.2.1?
Attachment #416826 - Flags: approval1.9.1.8?
Comment on attachment 416826 [details] [diff] [review]
Patch

Except the patch needs tweaking to apply to older branches.
Attachment #416826 - Flags: approval1.9.2.1?
Attachment #416826 - Flags: approval1.9.1.8?
Attached patch For 1.9.2Splinter Review
Attachment #418946 - Flags: approval1.9.2.1?
Attachment #418946 - Attachment is patch: true
Attachment #418946 - Attachment mime type: application/octet-stream → text/plain
Attached patch For 1.9.1Splinter Review
I think we should get this new API in on as many branches as possible.
Attachment #421487 - Flags: review?(jst)
Attachment #421487 - Flags: approval1.9.1.8?
Attachment #421487 - Flags: review?(jst) → review+
a=beltzner for 192 branch
Whiteboard: [needs 1.9.2 bake before 1.9.1]
Attachment #418946 - Flags: approval1.9.2.1? → approval1.9.2.1+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69285201ec18
Whiteboard: [needs 1.9.2 bake before 1.9.1] → [fixed1.9.2.1]
Whiteboard: [fixed1.9.2.1]
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.
Attachment #421487 - Flags: superreview?(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 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.
Attachment #421487 - Flags: superreview?(bzbarsky) → superreview-
Attachment #421487 - Flags: approval1.9.1.8? → approval1.9.1.8-
(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?
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. :(
Yes, we made that change on 1.9.2 on the 19th.  See comment 12.
Comment on attachment 418946 [details] [diff] [review]
For 1.9.2

I think we will want a branch safe patch for this, yes.
Attachment #418946 - Flags: approval1.9.2.1+ → approval1.9.2.1-
Blocks: 547176
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.
OS: Linux → All
Hardware: x86 → All
The RESOLVED FIXED status of this bug is misleading if it's been backed out, fwiw. Documentation has had that bit deleted now.
> 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... ;)
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.
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?
You need to log in before you can comment on or make changes to this bug.