Bug 16716 – XHR Handler may call undefined function

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Brent Fulgham]

The attached test case (from the Adobe AIR/APOLLO Team) demonstrates a bug in WebKit where the XHR 'onreadystatechange' handler executes before we have finished parsing the document.  This is a problem because (1) seemingly unrelated changes to the html content will cause the behaviour of the javascript to change, and (2) network latency could cause the javascript to change.

This is due to that fact that while we are parsing the document, we may not have seen all the javascript code in the document.  If an 'onreadystatechange' handler for an XHR fires before we have parsed all the javascript and the 'onreadystatechange' handler depends on the existance of all of the javascript in the document, then bad things might happen.

[Brent Fulgham]

Created attachment 18247 [details]
Test case for the XHR issue

Unzip the attached file for the test case.  See comments in the 'xhrAsync.html' file for details.

[Brent Fulgham]

See http://paste.lisp.org/display/53270 for a proposed correction.  Note that the use of the APOLLO-only m_finishLoadingTimer variable indicates some rework is needed.

[Mark Rowe (bdash)]

What makes this specific to XHR?  It seems as though similar behaviour could be triggered by other event handlers, such as an onmousemove, that are installed while the page is still loading.  I'm not really convinced that this is a bug.

[Brent Fulgham]

Note that the test case reports "FAIL" on a nightly build of WebKit 525+ (SVN 29107)(In reply to comment #3)
> What makes this specific to XHR?  It seems as though similar behaviour could be
> triggered by other event handlers, such as an onmousemove, that are installed
> while the page is still loading.  I'm not really convinced that this is a bug.
> 
I'm not sure.  Why would the fact that it might also be triggered by 'onmousemove' lead you to the conclusion that this isn't a bug?  Do you mean that other browsers probably have the same (perhaps wrong) behavior?

[Mark Rowe (bdash)]

It seems a stretch to call the current behaviour "wrong".  Calling a function before it is defined is not going to work elsewhere in JavaScript, why would an XHR event handler be any different?  In the synchronous case it is obvious that the "FAIL!!!!" case of your test is the right one to be hit.  Why is it "wrong" that it is a possible valid outcome in the asynchronous case?

[Maciej Stachowiak]

I think this bug is invalid. Reasons to believe the current behavior is not wrong:

1) Other browsers have the same behavior (making it highly unlikely this fix is required for web compatibility).
2) The XHR spec does not require anything to the contrary.
3) Synchronous XHR has the same behavior.
4) It's easy for authors to ensure an async XHR doesn't call an undefined function from its onreadystatechange handler, there are at least two very easy ways. 
    a) ensure that all functions you need are defined in the same script or earlier scripts as the one where you start the XHR - this works because <script> processing blocks the parser and is serialized.
    b) start your XHR from the document's "load" event or DOMContentLoaded event.
5) <img onload=""> can similarly fire before the document's "load" event; indeed, in this case it is required that the image's load event fire before the document's!

All of these arguments seriously undermine the claim that the current WebKit behavior is wrong.

Furthermore, delaying async XHR replies may be harmful to site loading performance. Many sites use "AJAX" techniques as part of the page load process but may also contain individual slow-loading resources, such as ads. Making this fix could cause display of useful site content to be delayed waiting for ads, in a way that can't happen in other browsers.

I suspect Adobe made this change because they created some buggy HTML/JS content as an AIR demo and mistakenly thought the bug was in WebKit rather than their code. I'm thinking something like this:

<script>
var xhr = new XMLHttpRequest();
xhr.onreadystatechange = handler;
xhr.open("GET", "something.xml");
xhr.send();
function handler() {
    someLibraryFunction();
}
</script>
<script src="external-lib.js"></script>

But that code is incorrect as written - the order of the two <script> elements should be reversed.

Therefore, closing this bug as INVALID unless there's a further argument as to why this behavior is wrong.

[Chris Brichford]

We changed the xhr code to fire onreadystatechange after the document is parsing mainly because of a security policy we had to implement.  The real issue for us is that while a document with a "app:" url scheme is being parsed we allow window.eval to work.  After a document with a "app:" url is finished parsing we then disable window.eval for that document.  We do this because we want to make it harder for an application developer to download data from the web using xhr and turn it into code that runs at application privilege level.

I do agree that the change probably should remain specific to PLATFORM(APOLLO) and should probably be tightened further to just documents with an "app:" url for the reasons given by Maciej.

[Stan Switzer]

Another possibly interesting observation on this:  Our test case included a document.write() in the XHR completion handler.  If the request completes while still parsing, I believe the write is injected into the document at that (more or less arbitrary) point in the parse, but if it completes after parsing, it will create a new document.  That seems quite odd to me.

Anyway, if this is the common behavior of browsers, I suppose that makes it a feature and not a bug.  I'm OK with that.