Last Comment Bug 290777 - Regression in defining getters on prototypes in content script
: Regression in defining getters on prototypes in content script
: 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
: Andrew Overholt [:overholt]
: 290659 290660 291169 292625 (view as bug list)
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Comment 4 Brendan Eich [:brendan] 2005-04-18 00:33:19 PDT
Created attachment 181020 [details] [diff] [review]

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

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.

Comment 6 Brendan Eich [:brendan] 2005-04-18 17:46:26 PDT
Workaround: instead of just

var elementProto = Element.prototype;
                              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

Comment 7 :Gavin Sharp [email:] 2005-04-18 17:59:01 PDT
*** Bug 290659 has been marked as a duplicate of this bug. ***
Comment 8 :Gavin Sharp [email:] 2005-04-18 17:59:48 PDT
*** Bug 290660 has been marked as a duplicate of this bug. ***
Comment 9 Johnny Stenback (:jst, 2005-04-18 22:19:01 PDT
Comment on attachment 181020 [details] [diff] [review]

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]

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

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

Comment 14 Brendan Eich [:brendan] 2005-04-20 17:16:39 PDT
So branch builds with this fix should appear under tomorrow.

Comment 15 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-20 17:43:14 PDT
Comment on attachment 181020 [details] [diff] [review]

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.

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 
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
and look out for any DHTML package that does (for some DOM element type Element)

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