Regression in defining getters on prototypes in content script

VERIFIED FIXED

Status

()

Core
XPConnect
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: bz, Assigned: brendan)

Tracking

({fixed-aviary1.0.4, fixed1.7.8})

1.7 Branch
fixed-aviary1.0.4, fixed1.7.8
Points:
---
Bug Flags:
blocking1.7.8 +
blocking-aviary1.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

 
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
Created attachment 181011 [details]
Testcase
Flags: blocking-aviary1.0.4?
(Assignee)

Comment 3

12 years ago
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
(Assignee)

Comment 4

12 years ago
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
Attachment #181020 - Flags: superreview?(jst)
Attachment #181020 - Flags: review?(shaver)
(Assignee)

Comment 5

12 years ago
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)

Updated

12 years ago
Assignee: dbradley → brendan
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 6

12 years ago
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+

Comment 10

12 years ago
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
(Assignee)

Updated

12 years ago
Summary: Regression in setting getters on prototypes in content script → Regression in defining getters on prototypes in content script

Comment 11

12 years ago
(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?
(Assignee)

Updated

12 years ago
Flags: blocking1.7.8?
Flags: blocking1.7.8+
Flags: blocking-aviary1.0.4?
Flags: blocking-aviary1.0.4+
(Assignee)

Comment 12

12 years ago
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+
(Assignee)

Comment 13

12 years ago
Fixed on the branches.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

12 years ago
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!
(Assignee)

Comment 16

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

/be

Comment 17

12 years ago
*** Bug 291169 has been marked as a duplicate of this bug. ***
*** Bug 292625 has been marked as a duplicate of this bug. ***

Comment 19

12 years ago
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.
Keywords: fixed-aviary1.0.4, fixed1.7.8

Updated

12 years ago
Flags: blocking-aviary1.0.5+ → blocking-aviary1.0.4+

Updated

12 years ago
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!
(Assignee)

Comment 21

12 years ago
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

12 years ago
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

Comment 23

12 years ago
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.