290777 - Regression in defining getters on prototypes in content script

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bah.  Submit-on-enter madness.

This bug is based on a post to n.p.m.dom:

----------------------------------------------------------------------------
It has been pointed out to me that my DHTML scripts no longer work in 1.0.3. 
The error points to some code written by Erik at WebFX that I use for XML. 
The code is basically a getter:

Text.prototype.__defineGetter__( "text", function () {
      return this.nodeValue;
     }
);

Node.prototype.__defineGetter__( "text", function () {
     var cs = this.childNodes;
     var l = cs.length;
     var sb = new Array( l );
     for ( var i = 0; i < l; i++ )
         sb[i] = cs[i].text;
     return sb.join("");
     }
);

The error I am getting is:

Error: uncaught exception: [Exception... "Illegal operation on WrappedNative 
prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)" 
location: "JS frame :: URL_GOES_HERE :: anonymous :: line 789"  data: no]

The offending line is:

return this.nodeValue;

Any ideas of how to get around this? 
----------------------------------------------------------------------------

This shouldn't have broken for content script, so we're buggy somewhere. 
Testcase coming up.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 3

[Brendan Eich [:brendan]]

Great.  Law of Unintended Consequences strikes.  We didn't test.  The patch is a
one-liner.  I wonder if we should slip it into 1.0.3 with a silent respin....

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

I should have seen this one.  We must not bind the thunk to its parent, which
is the proto, or that will override the normal |this| binding, leaing straight
to XPC_BAD_OP_ON_WN_PROTO.

/be

Comment 5

[Brendan Eich [:brendan]]

I'm wondering whether we can't respin and update the releases just done,
somehow, without having to make a 1.0.4/1.7.7 version number jump.

Wish we had our QA together for DHTML apps last week.  Wish I had run down the
problem jst and I had with the thunk's parent, which should have clued us into
JSFUN_BOUND_METHOD (a flag I forgot jband used in xpconnect), while we were
developing the xpconnect patch.  Wishes aren't horses, but we should try to fix
this regression as simply and quickly as we can.

/be

Comment 6

[Brendan Eich [:brendan]]

Workaround: instead of just

var elementProto = Element.prototype;
elementProto.__defineGetter__('text',
                              function () {return this.firstChild.nodeValue;});

do this:

var elementProto = Element.prototype;
elementProto.__proto__ = {
  get text() { return this.firstChild.nodeValue; },
  __proto__: elementProto.__proto__
};

If Element.prototype has a text property already, and you want to override it,
then interpose the new object ahead of Element.prototype on instances' prototype
chains.

/be

Comment 7

[:Gavin Sharp [email: [e-mail]]]

*** Bug 290659 has been marked as a duplicate of this bug. ***

Comment 8

[:Gavin Sharp [email: [e-mail]]]

*** Bug 290660 has been marked as a duplicate of this bug. ***

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 181020 [details] [diff] [review]
fix

sr=jst

Comment 10

[Erik Arvidsson]

Not a good time to be away from work. At least I'm happy that other people found
and figured out what was causing this bug.

I take it on myself for not testing the RCs of 1.0.3

Comment 11

[Alexandre Peshansky]

(In reply to comment #10)
> Not a good time to be away from work. At least I'm happy that other people found
> and figured out what was causing this bug.
> 
> I take it on myself for not testing the RCs of 1.0.3

So when can we get a win32 build with this fix?

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 181020 [details] [diff] [review]
fix

shaver's on the road, marking + for him.

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed on the branches.

/be

Comment 14

[Brendan Eich [:brendan]]

So branch builds with this fix should appear under
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-aviary1.0.1/ tomorrow.

/be

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 181020 [details] [diff] [review]
fix

r=shaver indeed, but a comment explaining why we mask out JSFUN_BOUND_METHOD
would not go amiss on the trunk!

Comment 16

[Brendan Eich [:brendan]]

This code is not on the trunk, and if I can help it, never will be.  See bug 281988.

/be

Comment 17

[José Jeria]

*** Bug 291169 has been marked as a duplicate of this bug. ***

Comment 18

[Peter Van der Beken [:peterv]]

*** Bug 292625 has been marked as a duplicate of this bug. ***

Comment 19

[Charles Eubanks]

Hi, so this fix is going into, eventually, 1.0.4 right and NOT into a double 
secret 1.0.3 patch?  
I just downloaded 1.0.3 and nothing seems to have magically been fixed.  
I am trying to figure out what to put into our release notes and/or comments to 
QA.

Comment 20

[sairuh (rarely reading bugmail)]

other than the attached test case, are there other areas or things we should
test to ensure that this didn't regress anything? thanks!

Comment 21

[Brendan Eich [:brendan]]

See
http://developer-test.mozilla.org/docs/Working_around_the_Firefox_1.0.3_DHTML_regression
and look out for any DHTML package that does (for some DOM element type Element)
Element.prototype.__defineGetter__(...).

/be

Comment 22

[Jay Patel [:jay]]

Verified fixed with bz's testcase (JS executes as expected with no errors) using
latest Win32 Aviary build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7.8) Gecko/20050509 Firefox/1.0.4 

Comment 23

[Randy Peterman]

If you test it with http://webfx.eae.net/dhtml/ieemu/genmove.ieemu.html you will
see if you fixed another variation of the bug.