404748 - __defineSetter__ regression in FF3: no longer works with event properties when used on prototype

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ish]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: 

The following code works well in FF2 and earlier:

HTMLDivElement.prototype.__defineSetter__("onclick",function () { alert("setter called"); });
divRef.onclick=function () {};

In FF3b the setter is never called. It is called fine for non-event properties, but all event properties stopped working.

This regression is critical for us and will require going through many changes in order to workaround. Can this be fixed for FF3?

Reproducible: Always

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Could you attach a minimal testcase using "Add an attachment"?
And finding the regression range would be great http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Comment 2

[Ish]

Attachment: Minimal testcase demonstrating the regression

Added requested testcase.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Regression somewhere between 2006-09-01 - 2006-11-15

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

2006-10-06-04-trunk works, 2006-10-07-04-trunk doesn't.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-10-06+01%3A00%3A00&maxdate=2006-10-07+07%3A00%3A00&cvsroot=%2Fcvsroot
I'd guess bug 349465.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Or some JS engine change? Not sure.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ah, didn't notice bug 159849.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Indeed, regression from bug 159849. Tested backing out locally.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't see a way to make both |with| (which requires that the property resolve somewhere on the prototype chain of the object) and the defineSetter thing here work short of always making sure to define the property on an ancestor of the prototype defineSetter was called on.  Of course people can call it at arbitrary places in the prototype chain (Object.prototype, anyone)?  And we don't really want to be putting on* properties on Object.prototype.

Put another way, we could perhaps with a lot of pain put the properties on HTMLDivElement.prototype, but then someone calling HTMLElement.prototype.defineSetter will run into this exact same bug.

Unless I'm seriusly missing something about how the JS engine works, I think our only reasonable options are that we ship either this bug or bug 159849.

Comment 10

[Ish]

Boris, I understand the point you make about the impossibility of supporting people trying to call it at arbitrary places in the chain. However, I believe that the main issue here is to keep supporting code that was already working, and not necessarily try to accommodate any conceivable new situation. 

HTMLSomeElement.prototype.__defineSetter__ on event properties worked up to and including FF2. The patch to bug 159849, which broke it, was never included in any official release, as far as I'm aware. So basically the choice is between keeping backward compatibility, and introducing new functionality.

I may not be entirely objective, but I'd argue that the new functionality offered by the 159849 patch is coming to support a feature that is both generally discouraged for performance and ambiguity reasons (FF 1.5 even had a strict warning about usage of 'with') and is not helping support more websites, as the syntax choice is very rare.

I'd definitely go with keeping backward compatibility here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> the main issue here is to keep supporting code that was already working

Yes, but the question is what this set of code should be.

> So basically the choice is between keeping backward compatibility, and
> introducing new functionality.

It's a choice between keeping compatibility and making some sites that used to not work work...

> as the syntax choice is very rare.

Sadly, it's not.  There's a lot of "with" usage out there, esp. in some of the JS libraries floating around (Dreamweaver stuff comes to mind).

I do agree that it's not clear which way the tradeoff should go here; we need data...  Plus I'd like to hear what Brendan has to say, if anything.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Possible fix.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Possible fix (diff -w)

This reverts us to work as we used to in the case described here. bz, what do you think about this approach?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I guess that works.  It makes the behavior ordering-dependent (so if you assign on an object once before the defineSetter call, the setter will never get called for future sets of that property on that object), but if we assume the defineSetter happens early enough it would work.

I'm a little confused by two parts of the patch:

1)  When doing an assignment, why are we now doing a RegisterCompileHandler if
    there is a setter up the proto chain?
2)  How can we hit the !ok case in the innermost |if| there?  The previous
    condition was "!proto || (ok && hasProp)", right?

Comment 15

[Johnny Stenback (:jst)]

(In reply to comment #14)
> I guess that works.  It makes the behavior ordering-dependent (so if you assign
> on an object once before the defineSetter call, the setter will never get
> called for future sets of that property on that object), but if we assume the
> defineSetter happens early enough it would work.

Yeah, not sure we can do much about that...

> I'm a little confused by two parts of the patch:
> 
> 1)  When doing an assignment, why are we now doing a RegisterCompileHandler if
>     there is a setter up the proto chain?
> 2)  How can we hit the !ok case in the innermost |if| there?  The previous
>     condition was "!proto || (ok && hasProp)", right?

Both of those are screwups when I refactored some of that code from an earlier version... New patch coming up. 

Comment 16

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 289735 [details] [diff] [review]
Updated fix.

This, I buy.

Please check in some tests to go with the fix?

Comment 18

[Johnny Stenback (:jst)]

I'd love to see someone step up and help write testcases for this, and to get this landed. This isn't a high priority blocker, if a blocker at all, so I'd love to not spend more time on this now unless I have to.

Comment 19

[Mike Schroepfer]

Comment on attachment 289735 [details] [diff] [review]
Updated fix.

would be great to get some tests with this...

Comment 20

[Reed Loden [:reed]]

Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.492; previous revision: 1.491
done

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in a mochitest for this.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #14)
> I guess that works.  It makes the behavior ordering-dependent (so if you assign
> on an object once before the defineSetter call, the setter will never get
> called for future sets of that property on that object), but if we assume the
> defineSetter happens early enough it would work.

That's life on the prototype chain, though. Sorry I didn't reply to my summons earlier -- was traveling last week -- glad to see you guys sorted it out.

/be

Comment 23

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task