Last Comment Bug 290777 - Regression in defining getters on prototypes in content script
: Regression in defining getters on prototypes in content script
Status: VERIFIED FIXED
: fixed-aviary1.0.4, fixed1.7.8
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 1.7 Branch
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Brendan Eich [:brendan]
: Phil Schwartau
Mentors:
: 290659 290660 291169 292625 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-17 21:31 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2006-03-12 18:27 PST (History)
23 users (show)
brendan: blocking1.7.8+
asa: blocking‑aviary1.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (728 bytes, text/html)
2005-04-17 21:33 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
fix (1.18 KB, patch)
2005-04-18 00:33 PDT, Brendan Eich [:brendan]
brendan: review+
jst: superreview+
asa: approval‑aviary1.0.4+
brendan: approval1.7.8+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2005-04-17 21:31:11 PDT
 
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2005-04-17 21:32:52 PDT
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 [:bz] (still a bit busy) 2005-04-17 21:33:56 PDT
Created attachment 181011 [details]
Testcase
Comment 3 Brendan Eich [:brendan] 2005-04-18 00:31:16 PDT
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] 2005-04-18 00:33:19 PDT
Created attachment 181020 [details] [diff] [review]
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] 2005-04-18 00:56:07 PDT
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] 2005-04-18 17:46:26 PDT
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: gavin@gavinsharp.com] 2005-04-18 17:59:01 PDT
*** Bug 290659 has been marked as a duplicate of this bug. ***
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-04-18 17:59:48 PDT
*** Bug 290660 has been marked as a duplicate of this bug. ***
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-18 22:19:01 PDT
Comment on attachment 181020 [details] [diff] [review]
fix

sr=jst
Comment 10 Erik Arvidsson 2005-04-19 06:13:09 PDT
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 2005-04-20 10:10:56 PDT
(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] 2005-04-20 16:48:41 PDT
Comment on attachment 181020 [details] [diff] [review]
fix

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

/be
Comment 13 Brendan Eich [:brendan] 2005-04-20 17:13:58 PDT
Fixed on the branches.

/be
Comment 14 Brendan Eich [:brendan] 2005-04-20 17:16:39 PDT
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) 2005-04-20 17:43:14 PDT
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] 2005-04-20 17:53:05 PDT
This code is not on the trunk, and if I can help it, never will be.  See bug 281988.

/be
Comment 17 José Jeria 2005-04-22 00:56:44 PDT
*** Bug 291169 has been marked as a duplicate of this bug. ***
Comment 18 Peter Van der Beken [:peterv] 2005-05-02 09:29:25 PDT
*** Bug 292625 has been marked as a duplicate of this bug. ***
Comment 19 Charles Eubanks 2005-05-02 11:14:27 PDT
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) 2005-05-09 14:24:10 PDT
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] 2005-05-09 14:27:50 PDT
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] 2005-05-09 18:24:20 PDT
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 2005-05-10 07:25:22 PDT
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.

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