Closed Bug 290777 Opened 19 years ago Closed 19 years ago

Regression in defining getters on prototypes in content script

Categories

(Core :: XPConnect, defect)

1.7 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: brendan)

References

Details

(Keywords: fixed-aviary1.0.4, fixed1.7.8)

Attachments

(2 files)

 
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.
Assignee: general → dbradley
Component: DOM → XPConnect
QA Contact: ian → pschwartau
Attached file Testcase
Flags: blocking-aviary1.0.4?
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
Status: NEW → ASSIGNED
Attached patch fixSplinter 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
to XPC_BAD_OP_ON_WN_PROTO.

/be
Attachment #181020 - Flags: superreview?(jst)
Attachment #181020 - Flags: review?(shaver)
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
Flags: blocking1.7.8?
Assignee: dbradley → brendan
Status: ASSIGNED → NEW
OS: Linux → All
Hardware: PC → All
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
Status: NEW → ASSIGNED
*** Bug 290659 has been marked as a duplicate of this bug. ***
*** Bug 290660 has been marked as a duplicate of this bug. ***
Comment on attachment 181020 [details] [diff] [review]
fix

sr=jst
Attachment #181020 - Flags: superreview?(jst) → superreview+
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
Summary: Regression in setting getters on prototypes in content script → Regression in defining getters on prototypes in content script
(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?
Flags: blocking1.7.8?
Flags: blocking1.7.8+
Flags: blocking-aviary1.0.4?
Flags: blocking-aviary1.0.4+
Comment on attachment 181020 [details] [diff] [review]
fix

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

/be
Attachment #181020 - Flags: review?(shaver)
Attachment #181020 - Flags: review+
Attachment #181020 - Flags: approval1.7.8+
Attachment #181020 - Flags: approval-aviary1.0.4+
Fixed on the branches.

/be
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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 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!
This code is not on the trunk, and if I can help it, never will be.  See bug 281988.

/be
*** Bug 291169 has been marked as a duplicate of this bug. ***
*** Bug 292625 has been marked as a duplicate of this bug. ***
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.
Flags: blocking-aviary1.0.5+ → blocking-aviary1.0.4+
Attachment #181020 - Flags: approval-aviary1.0.5+ → approval-aviary1.0.4+
other than the attached test case, are there other areas or things we should
test to ensure that this didn't regress anything? thanks!
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
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 
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: