691059 - "Illegal operation on WrappedNative prototype object" on store.sony.ca's prototype.js

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[(no longer active)]

[11:12:24.575] uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://store.sony.ca/wcsstore/SonyStyleCanadaStorefront/javascript/prototype.js :: copy :: line 2634"  data: no]

This causes all pages on store.sony.ca to fail to load completely.

This site loads fine in Aurora.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Ehsan Akhgari [:ehsan] from comment #0)

> This site loads fine in Aurora.

That would imply that the regression window is sometime in the last 5 days, no?

bz, blake, peterv - any idea what might have caused this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Not yet.  Got a one-day regression range?

Comment 3

[Dave Townsend [:mossop]]

This site does not load fine for me in aurora. Seems ok in beta.

Comment 4

[(no longer active)]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=06445f55f009&tochange=5319b0100025

Nothing in the range jumps at me.  CCing all of the people in this range.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is a "regression" from bug 432698.

I put that in quotes, because our behavior is required by the spec, but maybe we need to change the spec here....

The exception is thrown in this function:

  function copy(methods, destination, onlyIfAbsent) {
    onlyIfAbsent = onlyIfAbsent || false;
    for (var property in methods) {
      var value = methods[property];
      if (!Object.isFunction(value)) continue;
      if (!onlyIfAbsent || !(property in destination))
        destination[property] = value.methodize();
    }
  }

Line 2634 is that innermost assignment.

The relevant call to copy(), I bet is this:

    copy(Element.Methods, HTMLElement.prototype);

(not lack of third argument, which makes onlyIfAbsent false!) because earlier we have:

  Object.extend(Element.Methods, methods || { });

where |methods| is the argument passed to Element.addMethods, and this is being called like so:

  Element.addMethods({
      onmouseenter: function(element,observer) {
  .....

The upshot is that this code tries to directly set HTMLElement.prototype.onmouseenter (and similar for onmouseleave), which throws.

The site is using Prototype 1.6.0.2 from back in 2008.

Prototype 1.7 doesn't have the onmouseenter addMethods stuff going on.  So sites using that shouldn't have a problem here.

In any case, maybe we need to make this these props not throw just like we did for onreadystatechange....  On the other hand, this code expects the props to actually get set on the proto, so silently doing nothing won't be quite right either.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

And ehsan, thanks for the regression range!

Comment 7

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

I'm not at all that familiar with our prototype handling...
But I guess it is good time to know some more about it :/

(I don't quite understand why setting interface.prototype.property needs to throw)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Our DOM property setters for interface X throw if the |this| object is not actually an X.

And HTMLElement.prototype is not an actual HTMLElement...  In particular, it has no event listener manager, etc.  

One option would be to have the set silently define the property on |this| instead of throwing.  We could even try doing that globally for DOM properties.....

Comment 9

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

I was wondering *why* setting .prototype.property throws...

Need to test what Opera does here. It support mouseenter/leave, yet I didn't
see any exceptions in the error console.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I was wondering *why* setting .prototype.property throws...

Because there's really no sane action it can take.  What would you expect it to do if it did not throw?

Comment 11

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

Looks like Opera just sets the property to the prototype object.

HTMLElement.prototype.onmouseenter = function foo() { return "hello"}; HTMLElement.prototype.onmouseenter() == "hello";
-> true

Comment 12

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

(In reply to Boris Zbarsky (:bz) from comment #10) 
> Because there's really no sane action it can take.  What would you expect it
> to do if it did not throw?

I would expect the property just be there, like in Opera. 
(That is at least my initial reaction to this all.)

Comment 13

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

Chrome seems to work like Opera, it just adds property to the prototype.

Comment 14

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

(Tested with onclick, since webkit doesn't support mouseenter/leave )

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Olli Pettay [:smaug] from comment #13)
> Chrome seems to work like Opera, it just adds property to the prototype.

Does that mean that those properties live on the objects themselves?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I was wondering *why* setting .prototype.property throws...

Because there's really no sane action it can take.  What would you expect it to do if it did not throw?

> Looks like Opera just sets the property to the prototype object.

Ah, but does it have the property there _before_ that?  WebKit does not, for example.

Comment 17

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

Ah, Opera doesn't have onfoo properties in HTMLElement.prototype.

> Because there's really no sane action it can take.  What would you expect it
> to do if it did not throw?

It would just set a property in the prototype. Just like var o = {}; o.foo = "helloworld";

Comment 18

[Boris Zbarsky [:bzbarsky]]

> It would just set a property in the prototype.

But the point is that the prototype _already_ has a property called "onmouseenter".  It has a getter and a setter.  When you do |HTMLElement.prototype.onmouseenter = function() {}|, the setter is invoked with the function as the argument and HTMLElement.prototype as |this|.  Now what is this setter supposed to do?

To do the thing you suggested would involve the setter reconfiguring the prototype to have a data property instead of an acessor property.  That's what I suggested in comment 8 near the end.  The side effect, of course, would be that onmouseenter sets on particular elements would no longer register event listeners after such a set on the prototype.

Opera and WebKit not having on* properties on the prototype certainly explains their behavior.

What about IE9/IE10?  If you pick an on* property it supports and try assigning it on the prototype, what happens?

Comment 19

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

Yes, the prototype has already the property. The setter would store the value somewhere so that
the getter would return it.
(This is all still just my initial reaction to this all, and I can be very wrong.)

It does feel a bit awkward to me that one can set function property to a new value
(HTMLElement.prototype.lookupPrefix = function() { return "hello"}), but not an
attribute.

I was told that IE9 throws "Invalid calling object" for HTMLElement.prototype.onclick = function() {}

Comment 20

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

The website does ofc have <meta http-equiv="X-UA-Compatible" content="IE=8" /> :/

Comment 21

[Boris Zbarsky [:bzbarsky]]

> The setter would store the value somewhere so that the getter would return it.

We could try doing that; should that be just for on* properties or all properties?

Also, does this script expect to get .onmouseenter on individual elements and get the thing it stored on the prototype?  If it does, that approach won't work (nor will silently ignoring the set).

> It does feel a bit awkward to me that one can set function property

The important part there is that 'lookupPrefix' a data property that doesn't have a setter.

In particular, if you set lookupPrefix on an individual node, it will simply shadow the prototype and create an own property on the node, which is distinctly not what happens with accessor properties.

Comment 22

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

Running the page in IE9 mode does give similar exceptions.

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK.  So if you move the two new props from nsIInlineEventHandlers to Element locally, and then quickstub them with the same magic as we did for onreadystatechange on document, does this page still work?  Not sure how to exercise the onmouseenter stuff on this page....

Comment 24

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

Btw, I don't yet know what doesn't work in store.sony.ca.
I do get the errors, but pages seem to load just fine here.

Comment 25

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

http://www.prototypejs.org/assets/2008/1/25/prototype-1.6.0.2.js doesn't have that buggy
mouseenter/leave stuff. So the site uses modified version of the script.

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #24)
> Btw, I don't yet know what doesn't work in store.sony.ca.
> I do get the errors, but pages seem to load just fine here.

For me, the page does not display all of the images it should in Nightly.  Under "Shop Special Offers" there is just a giant void, whereas in Chrome there is an image.  Additionally, while at the top of the page it first shows an image, it seems to rotate through them, and subsequent images are not displayed, so again you get a blank space at the top of the page.

Comment 27

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

(In reply to Boris Zbarsky (:bz) from comment #23)
> OK.  So if you move the two new props from nsIInlineEventHandlers to Element
> locally, and then quickstub them with the same magic as we did for
> onreadystatechange on document, does this page still work?
No :(
Things seem to work even worse that way, and I get the following exception.

JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert JavaScript argument"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: http://store.sony.ca/wcsstore/SonyStyleCanadaStorefront/javascript/prototype.js :: copy :: line 2634"  data: no]

Comment 28

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

Attachment: qs hack

Comment 29

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

Oops, the patch is missing something...

Comment 30

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

Attachment: patch

This seems to actually work, at least I don't see any js exceptions, and the page
looks ok here.
This does the same thing for onmouseenter/leave what was done for onreadystatechange.

Uploaded to try (37995d0e12c4). Please test.

Comment 31

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

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-37995d0e12c4/

Comment 32

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

Attachment: for aurora

Comment 33

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

(Aurora https://tbpl.mozilla.org/?tree=Try&rev=24c15b3e9895)

Comment 34

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #31)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.
> com-37995d0e12c4/

This try server build works fine for me.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Olli, how much pain is it for you to produce separate diffs for the changes that actually matter and the iid rev?  That would make this _way_ easier to review.

Also, please don't forget to raise a spec issue on this...

Comment 36

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

Just look at the patch from start to +++ b/dom/interfaces/html/nsIDOMHTMLAnchorElement.idl
and from the end to +++ b/js/src/xpconnect/src/dom_quickstubs.qsconf.

And yes, I'll file a spec issue.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 565762 [details] [diff] [review]
patch

Ah, ok.  So for the moment, as long as those properties aren't on Node, the thisType for document and element should be nsDocument and nsGenericElement respectively, no?  Otherwise you can end up with the document setter applied to an element, which should not work per spec but will work in our implementation.  That should incidentally not require you to make the nsINode methods public; I'd really like to leave them protected if at all possible.

r=me with that fixed.

Comment 38

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

(In reply to Boris Zbarsky (:bz) from comment #37)
> That should incidentally not require you to make the
> nsINode methods public;
Well, the methods are still protected to qs code can't access them.

Comment 39

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

(In reply to Boris Zbarsky (:bz) from comment #37)
> Ah, ok.  So for the moment, as long as those properties aren't on Node, the
> thisType for document and element should be nsDocument and nsGenericElement
> respectively, no?
This change causes qs code to crash with HTMLElement.prototype.onmouseenter = function() {}
but interestingly not with HTMLDocument.prototype.onmouenter = function()  {}

I guess there is something wrong with  xpc_qsUnwrapThis<nsGenericElement>

Comment 40

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

http://pastebin.mozilla.org/1352003 fixes the problem

Comment 41

[Boris Zbarsky [:bzbarsky]]

> Well, the methods are still protected to qs code can't access them.

But those methods are present and public on nsDocument at least, right?  Are they not on nsGenericElement?

For that last bit, are we landing in a case where !failureFatal and ok but *ppThis didn't get set or something?

Comment 42

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

(In reply to Boris Zbarsky (:bz) from comment #41)
> > Well, the methods are still protected to qs code can't access them.
> 
> But those methods are present and public on nsDocument at least, right?  Are
> they not on nsGenericElement?
They are public in nsDocument because it inherits nsIInlineEventHandlers.
 
> For that last bit, are we landing in a case where !failureFatal and ok but
> *ppThis didn't get set or something?
Yes. If !failure, ok is true, and nothing sets *ppThis.
Note, xpc_qsUnwrapThis<nsIContent> sets the value of content to null, not *ppThis

But I can simplify the change a bit.

Comment 43

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

Attachment: patch, includes fix for xpc_qsUnwrapThis<nsGenericElement>

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 565954 [details] [diff] [review]
patch, includes fix for xpc_qsUnwrapThis<nsGenericElement>

r=me.  Good catch on the unwrap stuff!

Comment 45

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

Attachment: for aurora

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/c80e4acec9f5
https://hg.mozilla.org/mozilla-central/rev/98baa291a949
https://hg.mozilla.org/mozilla-central/rev/50f9a6fb25db

(The patch ended up being in 3 different changeset in my tree)

Comment 47

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

http://www.w3.org/Bugs/Public/show_bug.cgi?id=14428

Comment 48

[Johnny Stenback (:jst)]

Olli, this is a pretty big patch to take on aurora. We're weighing the cost vs. benefit of taking this patch vs backing out the patch that caused it. I don't think the backout will be easy at all, but please share your thoughts on the aurora patch vs. a backout. Thanks.

Comment 49

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

The patch is basically just using the hack from bug 684671 to onmouseenter/leave.
Size of the patch comes from the uuid updates.

Comment 50

[LegNeato]

We've actually decided to back out bug 432698 for aurora. Reasoning:

* Not a standard
* Lived without it this long
* According to http://www.quirksmode.org/blog/archives/2011/09/event_compatibi_1.html both Chrome and Safari don't support this

We think it can wait for the next migration (assuming the problems get fixed on Nightly)

Comment 51

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

(In reply to Christian Legnitto [:LegNeato] from comment #50)
> We've actually decided to back out bug 432698 for aurora. Reasoning:
> 
> * Not a standard
What is not a standard? mouseenter/leave is, .prototype.onfoo isn't

> * Lived without it this long
sure

> * According to
> http://www.quirksmode.org/blog/archives/2011/09/event_compatibi_1.html both
> Chrome and Safari don't support this
That sure shouldn't be a real reason

> We think it can wait for the next migration (assuming the problems get fixed
> on Nightly)
Sure.

Comment 52

[LegNeato]

(In reply to Olli Pettay [:smaug] from comment #51)
> (In reply to Christian Legnitto [:LegNeato] from comment #50)
> > We've actually decided to back out bug 432698 for aurora. Reasoning:
> > 
> > * Not a standard
> What is not a standard? mouseenter/leave is, .prototype.onfoo isn't

It was our understanding from reading bug 432698 that these are "not W3C events". if they are in fact or defined somewhere else that would definitely be good to know.

> 
> > * Lived without it this long
> sure

This was the main reasoning + the uuid changes.

> 
> > * According to
> > http://www.quirksmode.org/blog/archives/2011/09/event_compatibi_1.html both
> > Chrome and Safari don't support this
> That sure shouldn't be a real reason

No, but if we were the odd browser out it would push us a bit to get it in as early as possible.

> 
> > We think it can wait for the next migration (assuming the problems get fixed
> > on Nightly)
> Sure.

Great, thanks so much for the quick backout patch!

Comment 53

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

(In reply to Christian Legnitto [:LegNeato] from comment #52)
> It was our understanding from reading bug 432698 that these are "not W3C
> events". if they are in fact or defined somewhere else that would definitely
> be good to know.

mouseenter/leave are in DOM 3 Events.

Comment 54

[Alex Keybl [:akeybl]]

FF10 does not appear to be affected from my testing of store.sony.ca. Untracking for FF10.