684671 - Virgin America website won't load correctly : Illegal operation on WrappedNative prototype object due to setting XMLDocument.prototype.onreadystatechange

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Mathieu Marquer]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110904 Firefox/9.0a1
Build ID: 20110904030822

Steps to reproduce:

Opening www.virginamerica.com


Actual results:

Blank space where I usually find the booking part of the website. Also http://www.virginamerica.com/flights/ won't behave correctly (list options not dynamically changing, calendar not working). 

Error in error console :
Erreur : uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://www.virginamerica.com/scripts/GlobalScriptFile_08232011.js :: <TOP_LEVEL> :: line 1"  data: no]


Expected results:

Website behaving correctly.

Comment 1

[Alice0775 White]

Confirmed:

Error: Illegal operation on WrappedNative prototype object = NS_ERROR_XPC_BAD_OP_ON_WN_PROTO
Source file: http://www.virginamerica.com/flights/scripts/GlobalScriptFile_08232011.js
Line: 1

Error: document.layers is undefined
Source file: http://www.virginamerica.com/flights/scripts/GlobalScriptFile_08232011.js
Line: 1

Error: s is undefined
Source file: http://www.virginamerica.com/flights/
Line: 3353

Error: s is undefined
Source file: http://www.virginamerica.com/flights/
Line: 3412

Error: $ is not defined
Source file: http://www.virginamerica.com/flights/
Line: 4447



Regression window(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/b354d9b3e9e1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824095659
Fails:
http://hg.mozilla.org/mozilla-central/rev/e58e98a89827
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110825 Firefox/9.0a1 ID:20110825030823
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b354d9b3e9e1&tochange=e58e98a89827

Regression window(cached m-i hourly),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7254c4f4a805
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110823 Firefox/9.0a1 ID:20110824124236
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3fbd9eaf9deb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824125354
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7254c4f4a805&tochange=3fbd9eaf9deb

Triggered by
Bug 659350 - don't rely on slots to hold event handlers, IDL-ify event handlers

Comment 2

[Boris Zbarsky [:bzbarsky]]

The site does this:

        if (window.XMLDocument) {
            XMLDocument.prototype.onreadystatechange = null;
            XMLDocument.prototype.parseError = 0;

and that first assignment throws the exception listed above.  Worse yet, this is not one-off code, but part of the Sarissa library.  So evangelism is not likely to help globally here.  We might be able to get at least this site to fix the broken code, of course.

I believe per WebIDL throwing there is in fact the right thing as long as XMLDocument inherits from Document, because the setter on Document.prototype should throw when invoked with a |this| that's not a Document object.

So either we need some pretty serious spec changes here in terms of how onreadystatechange works, or the XMLDocument thing needs to go away or it needs to stop inheriting from Document.  The second of these is what seems to be the case in IE, though making that change may add dependencies on the exact behavior of document.load() in IE, complete with whatever readystate stuff it needs to do.  The L&S codepath does |Document.prototype.onreadystatechange = null;| so would _really_ fail, but luckily I think no one implements L&S.

I just tested, and WebKit, Gecko, and Presto all have an XMLDocument, so take that codepath.  None do the "XMLDocumentLoader" thing specified at http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#xmldocumentloader as far as I can tell.

In WebKit, it appears that XMLDocument.prototype's proto is Node.prototype, not Document.prototype.  But in Presto, XMLDocument.prototype's proto is Document.prototype, as in Gecko.  I haven't looked in detail into what the Presto behavior is for the onreadystatechange setter and why the site works there.

Now this particular site does NOT depend on XMLDocument.prototype.load actually doing or being anything useful, I would guess (since in fact in WebKit it doesn't exist).  So maybe one option would be to make XMLDocument not inherit from Document and hence not have any inline event listeners on it and move load() to a different interface altogether.

Other possible options?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I should note that the library code is clearly buggy; setting onreadystatechange on the _prototype_ makes absolutely no sense.

Comment 4

[Boris Zbarsky [:bzbarsky]]

ccing ms2ger too, since http://html5.org/specs/dom-parsing.html#xmldocument specifies that XMLDocument inherits from Document.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh "serious spec changes" above means "make the onreadystatechange setter on Document.prototype a silent no-op when |this| is not a document", which is not something WebIDL supports doing at the moment.

Comment 6

[Cameron McCormack (:heycam)]

It looks like Presto doesn't do onreadystatechange on XMLDocument (or at least, the property doesn't exist on a newly created XMLDocument object).  Also, since they still have IDL attributes be own properties on the instance rather than accessor properties on the prototype, they would avoid the exception even if the property did exist.

Adding a [NoOpIfWrongThis] or similar seems a little distasteful, but I am getting quite used to eating foul web content. ;)  I don't think we should change this in the general case -- I think throwing is the right thing to do when `this` is wrong.

I have no idea what Document members are required on XMLDocument to judge whether it makes sense to just make XMLDocument inherit from Node and copy load() and a couple of other things over to it.

Or even if need to keep XMLDocument and not just make DOMParser etc. return a plain Document.  That might be the neatest solution, if we can get away with it.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we add a one-off thing to the Gecko code here and warn in the developer console. It would seem that at *some* point in the future we would be able to remove that, and in the meantime we won't have to dig ourselves a deeper hole in terms of complexity of the web platform.

Of course, I don't know what that means in terms of what various specs should state...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Could we add a one-off thing to the Gecko code here

The one-off being to allow a bogus |this| in this case?  Or something else?

If you mean allowing the bogus |this|, we could try, but that involves changing either the XPConnect unwrapping behavior or the quickstubs generated unwrapping code.  It's quite a bit of work either way, though probably easier with quickstubs.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Then again, maybe it wouldn't be that bad for quickstubs.  I can try to hack up a patch for that just so we have it as an option.  Of course once we ship such a patch we may place further constraints on any possible solutions here (e.g. we may entrench the ability to set onreadystatechange on Document.prototype and have it be a no-op instead of throwing in the web platform).

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good.

Though it occurred to me, why does onreadystatechange show up on XMLDocument.prototype? Shouldn't it *just* be on Document.prototype?

Surely XMLDocument.prototype.__proto__ != Document.prototype

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Surely XMLDocument.prototype.__proto__ != Document.prototype

Surely it is!

[13:02:00.329] XMLDocument.prototype.__proto__ === Document.prototype
[13:02:00.332] true

That's what it means for XMLDocument to inherit from Document in IDL, after all.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, to be clear, I don't really care what the one-off is. I'd prefer to optimize first of affecting as few other properties in the DOM as possible, optimize second for what's easy to implement, and only third optimize for sensible behavior when the property is being set.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> [13:02:00.329] XMLDocument.prototype.__proto__ === Document.prototype
> [13:02:00.332] true

Doh! Pardon my brainfart

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: hackery: allow onreadystatechange getter/setter to not throw even if its |this| is bogus.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 558872 [details] [diff] [review]
hackery: allow onreadystatechange getter/setter to not throw even if its |this| is bogus.

This does fix the site for me.  Still waiting on spec feedback, but it would be good to give this a once-over in the meantime.

I _think_ after inlining the compiler should be able to nix the new conditionals in the common "fatal" case. I certainly tried very hard to make it so....

Comment 16

[Boris Zbarsky [:bzbarsky]]

Actually, that patch is definitely wrong, because elements implement nsIInlineEventHandlers via a tearoff and hence can never be unwrapped to it in the quickstub.

And because Window also implements this interface, I can't just unwrap to nsINode (which both elements and documents could do).

Peter, any idea how I can make this work?  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow the onreadystatechange getter/setter on Document.prototype to not throw even if its |this| is bogus.

This is ugly, but I think it works.  Note that for this site we really only need the setter to work, and only when null is being set... if you would prefer that we restrict the hackery accordingly, just let me know!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 559303 [details] [diff] [review]
Allow the onreadystatechange getter/setter on Document.prototype to not throw even if its |this| is bogus.

The new fix for bug 682554 will make this simpler...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow the onreadystatechange getter/setter on Document.prototype to not throw even if its |this| is bogus.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 559661 [details] [diff] [review]
Allow the onreadystatechange getter/setter on Document.prototype to not throw even if its |this| is bogus.

Review of attachment 559661 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but definitely want someone that knows qs better than me to look at it too.

::: js/src/xpconnect/src/dom_quickstubs.qsconf
@@ +910,5 @@
>                  '    rv = self->CreateTextNode(arg0, getter_AddRefs(result));'
>      },
> +    'nsIDOMDocument_GetOnreadystatechange' : {
> +        'thisType' : 'nsDocument',
> +        'unwrapThisFailureFatal' : False

shouldn't that be lower-case <false>? Same below.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> but definitely want someone that knows qs better than me to look at it too.

That's where Peter comes in.

> shouldn't that be lower-case <false>? 

Nope, the python keyword (which is what we want here) is spelled with a capital 'F'.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, one thing I forgot is that it'd be great if you could use nsIDocument::WarnOnceAbout to warn for each document-global which relies on this hack.

Comment 23

[Boris Zbarsky [:bzbarsky]]

That's actually a huge pain in this case....  I could try to do it if you really want me to, I guess, but I'd really prefer a separate bug.  Let me know?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Separate bug is of course ok. But I do think the only chance we have of seeing this hack removed is if we add that warning.

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 559661 [details] [diff] [review]
Allow the onreadystatechange getter/setter on Document.prototype to not throw even if its |this| is bogus.

Review of attachment 559661 [details] [diff] [review]:
-----------------------------------------------------------------

The change in xpcquickstubs.h looks wrong I think.

::: content/base/test/test_bug684671.html
@@ +37,5 @@
> +  canSetReadyStateChange = true;
> +} catch (e) {
> +  canSetReadyStateChange = false;
> +}
> +is(canSetReadyStateChange, true, "This may break web pages");

This could be an ok(...).

::: js/src/xpconnect/src/nsDOMQS.h
@@ +122,5 @@
>      if(ok)
>      {
> +        if(failureFatal || content)
> +          ok = castToElement(content, val, ppThis, pThisVal);
> +        if(failureFatal && !ok)

Could we make this:

    if(ok && content)
    {
        ok = castToElement(content, val, ppThis, pThisVal);
        if(failureFatal && !ok)

::: js/src/xpconnect/src/xpcquickstubs.h
@@ +548,5 @@
>  
> +    if (failureFatal)
> +        return NS_SUCCEEDED(rv) || xpc_qsThrow(cx, rv);
> +
> +    *ppThis = nsnull;

Doesn't this overwrite the result of castNative if it succeeded?

Comment 26

[Peter Van der Beken [:peterv]]

Oh, and are we evangelizing Sarissa? Getting them to fix this might avoid others copying it at least.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> This could be an ok(...).

Yeah, ok.  Will do.

> Could we make this:

No, because then if !content && failureFatal we wouldn't actually throw the exception we should throw.  Unless I'm completely misunderstanding your proposed change?

> Doesn't this overwrite the result of castNative if it succeeded?

Er, yes.  That needs to check for NS_FAILED(rv), of course.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments, except the one I don't think is right

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Oh, and are we evangelizing Sarissa?

I reported a bug to them.  But as far as I can tell it's sort of abandonware...

Comment 30

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #27)
> No, because then if !content && failureFatal we wouldn't actually throw the
> exception we should throw.

Didn't xpc_qsUnwrapThis<nsIContent> throw already in that case?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Didn't xpc_qsUnwrapThis<nsIContent> throw already in that case?

Ah, good point.  OK, let me rewrite that to be clearer, with more comments.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Oh, I recall now why I wrote it that way.  It was because I wanted the compiler to be able to use the statically-known (after inlining) value of failureFatal to generate exactly the same code as before on the common (failureFatal == true) path.  In particular, if failureFatal, then 

  if(failureFatal || content)

optimizes to |if (1)| (which is fine, because of failureFatal then !!content == !!ok) and 

  if(failureFatal && !ok)

optimizes to |if (!ok)|.

If you think the other is clearer and worth the extra branch in the common case, I can do it, of course.  Or I can clearly document why the code is the way it is.

Comment 33

[Peter Van der Beken [:peterv]]

Comment on attachment 561371 [details] [diff] [review]
Updated to comments, except the one I don't think is right

Let's hope the compiler figures it out ;-).

Comment 34

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/21f5c38e33ec

Comment 35

[Boris Zbarsky [:bzbarsky]]

Filed bug 688691 on adding a warning.

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21f5c38e33ec

Comment 37

[Johnny Stenback (:jst)]

This made 9, not tracking at this point.