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

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: smaug)

Tracking

({regression})

Trunk
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(firefox8 unaffected, firefox9+ affected, firefox10-)

Details

(Whiteboard: [qa+], URL)

Attachments

(2 attachments, 3 obsolete attachments)

[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.
(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?
Not yet.  Got a one-day regression range?
This site does not load fine for me in aurora. Seems ok in beta.
Keywords: regressionwindow-wanted
status-firefox8: --- → unaffected
status-firefox9: --- → affected
tracking-firefox9: --- → ?
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.
Keywords: regressionwindow-wanted
Keywords: testcase-wanted
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.  :(
Assignee: nobody → Olli.Pettay
Blocks: 432698
Keywords: testcase-wanted
And ehsan, thanks for the regression range!
(Assignee)

Comment 7

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

Comment 9

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

Comment 11

6 years ago
Looks like Opera just sets the property to the prototype object.

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

Comment 12

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

Comment 13

6 years ago
Chrome seems to work like Opera, it just adds property to the prototype.
(Assignee)

Comment 14

6 years ago
(Tested with onclick, since webkit doesn't support mouseenter/leave )
(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?
> 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.
(Assignee)

Comment 17

6 years ago
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";
> 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?
(Assignee)

Comment 19

6 years ago
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() {}
(Assignee)

Comment 20

6 years ago
The website does ofc have <meta http-equiv="X-UA-Compatible" content="IE=8" /> :/
> 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.
(Assignee)

Comment 22

6 years ago
Running the page in IE9 mode does give similar exceptions.
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....
(Assignee)

Comment 24

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

Comment 25

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

Comment 27

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

Comment 28

6 years ago
Created attachment 565756 [details] [diff] [review]
qs hack
(Assignee)

Comment 29

6 years ago
Oops, the patch is missing something...
(Assignee)

Comment 30

6 years ago
Created attachment 565762 [details] [diff] [review]
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.
Attachment #565756 - Attachment is obsolete: true
(Assignee)

Comment 31

6 years ago
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-37995d0e12c4/
(Assignee)

Comment 32

6 years ago
Created attachment 565787 [details] [diff] [review]
for aurora
(Assignee)

Comment 33

6 years ago
(Aurora https://tbpl.mozilla.org/?tree=Try&rev=24c15b3e9895)
(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.
(Assignee)

Updated

6 years ago
Attachment #565762 - Flags: review?(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...
(Assignee)

Comment 36

6 years ago
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 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.
Attachment #565762 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 38

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

Comment 39

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

Comment 40

6 years ago
http://pastebin.mozilla.org/1352003 fixes the problem
> 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?
(Assignee)

Comment 42

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

Comment 43

6 years ago
Created attachment 565954 [details] [diff] [review]
patch, includes fix for xpc_qsUnwrapThis<nsGenericElement>
Attachment #565762 - Attachment is obsolete: true
Attachment #565954 - Flags: review?(bzbarsky)
Comment on attachment 565954 [details] [diff] [review]
patch, includes fix for xpc_qsUnwrapThis<nsGenericElement>

r=me.  Good catch on the unwrap stuff!
Attachment #565954 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 45

6 years ago
Created attachment 565963 [details] [diff] [review]
for aurora
Attachment #565787 - Attachment is obsolete: true
Attachment #565963 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 46

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

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 47

6 years ago
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14428
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.
(Assignee)

Comment 49

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

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

Updated

6 years ago
tracking-firefox10: ? → +
tracking-firefox9: ? → +

Updated

6 years ago
Attachment #565963 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Assignee)

Comment 51

6 years ago
(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

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

Comment 53

6 years ago
(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.
Whiteboard: [qa+]
FF10 does not appear to be affected from my testing of store.sony.ca. Untracking for FF10.
tracking-firefox10: + → -
You need to log in before you can comment on or make changes to this bug.