300325 - [FIX]Unable to set "expando" properties on an XPCNativeWrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Add the following code to the end of the initMenu method in browser.js:

        this.target.ownerDocument.documentElement.foo = 1;
        alert(this.target.ownerDocument.documentElement.foo);

2)  Build in browser/, start Firefox, open a context menu

ACTUAL RESULTS: Alert showing "undefined"

EXPECTED RESULTS: Alert showing "1"

I bet the problem is that the NewResolve hook for XPCNativeWrapper doesn't
define the property if it's not an XPConnectified one...  Is this really
something we want not working with XPCNativeWrapper, though?

In any case, this blocks bug 295937, since I can't test a patch to that bug
because I can't set a property on an XPCNativeWrapper at all.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that doesn't work

I thought this would work, but it didn't....

As a test, I tried changing the new code to:

	  var test = this.target.ownerDocument.documentElement;
	  test.foo = 1;
	  alert(test.foo);

but this has the same result -- "undefined"

Comment 2

[Brendan Eich [:brendan]]

<bz> brendan: is there anything special that XPCNativeWrapper needs to do to
allow random JS props to be set on it?
<brendan> bz: this must be in the should *not* bypass case?
<bz> brendan: yeah, this is the not bypass case
<bz> brendan: I'm just chrome setting random expandos on content
<brendan> in the !iface case
<bz> yeah
<brendan> and the !member case
<brendan> just leave *vp alone
<brendan> don't set it to JSVAL_VOID
<brendan> in both places
<brendan> easy peasy

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Per brendan's suggestion

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: And the right diff

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 188904 [details] [diff] [review]
And the right diff

What I said ;-).  Sorry I didn't spot this earlier.

/be

Comment 6

[Johnny Stenback (:jst)]

So um, why do we even want to fix this bug? I'd rather see us throw an exception
if code tries to set a non-existent property on a DOM node through a wrapper,
you simply shouldn't do that, IMO.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The reason this came up is that people were asking me about wrapper lifetime.  I
assume that means they want to set something on the wrappers themselves...  As
part of trying to define this lifetime, we need to resolve bug 295937 one way or
another, which leads us to this bug.

I guess what I'm thinking about is extensions trying to set their own stuff on
DOM nodes (eg what adblock does now unsafely) and being able to do so in a safe
way.  If we don't allow setting properties on an XPCNativeWrapper, they can't do
that...

irc conversation jst and I just had:

<jst_> bz: I just asked why we would want to support that at all
<jst_> IMO we shouldn't...
> because for implicit wrappers, imo, we want to be as much like actual nodes as
possible...
<jst_> IMO no
<jst_> even if they're implicit, they're still wrappers
<jst_> they should reflect the DOM, and people should know that
> I guess the question is what the pros and cons are....
> If extensions want to use expandos safely, at the moment they can't
<jst_> if we start supporting adding properties, then we're back in a world
where stuff can be hidden from our chrome, though not by content code, of course
> right
> But chrome hiding stuff from chrome might be needed...
> perhaps
<jst_> I'd want to see a strong demand for that before we'd support it
<jst_> since we're starting down the same slippery slope
<jst_> if we support that
<jst_> even if the slope isn't nearly as steep
> OK.
> Fair enough...
<jst_> so I'd say for now we should just throw an exception if code attempts to
do this so they don't get inconsistent behavior
<jst_> depending on when GC happens etc

Comment 8

[Brendan Eich [:brendan]]

Then I came on and argued a bit that in JS, most objects are expandable, so why
not XPCNativeWrappers.  Now we hear in bug 297543 from a real extension's
authors about cases where the alternatives if this bug isn't fixed range from
ugly to untenable.  So I say we should fix this.  The patch is trivial.

jst, bz, anyone else with XPCNW standing: what do you say?

/be

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 188904 [details] [diff] [review]
And the right diff

Fair enough. Though this is still pretty confusing to our developers, I bet
this will cause us to get bugs about content not seeing properties set by
chrome etc, more so than w/o this at least.

Please do write this up as part of our XPCNativeWrapper docs ASAP.

   if (!member) {
     // No member, no property.

-    *vp = JSVAL_VOID;
-

That comment is not really true any more now then, maybe make it say something
like "No member, no *IDL* property to expose."

r=jst

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 188904 [details] [diff] [review]
And the right diff

We need this for 1.8/Fx1.1.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

Made the comment changes, and checked in.  I've updated the documentation by
adding the section at
http://developer-test.mozilla.org/en/docs/XPCNativeWrapper#Setting_.22expando.22_properties_on_XPCNativeWrapper
-- please let me know what you think, ok?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed, even.