Closed Bug 290777 Opened 15 years ago Closed 15 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: 15 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.