Bug 22475 – REGRESSION: Async XMLHttpRequest never finishes on nonexistent files anymore

• Component: XML
• Status: RESOLVED FIXED

[Francisco Tolmasky]

when requesting a local file that doesn't exist, the xmlhttprequest never gets beyond status "1".  In regular safari it gets all the way to 4.  We use this to determine when our users type in incorrect file names.

[Francisco Tolmasky]

Created attachment 25463 [details]
Reduction

Expected result: 1 then 2 then 4 (as in safari)
Actual result: 1

[Francisco Tolmasky]

Note: The attachment (test case) must be run locally

[Alexey Proskuryakov]

Confirmed as a regression with r38750.

[Mark Rowe (bdash)]

<rdar://problem/6400722>

[Julien Chaffraix]

Created attachment 27869 [details]
Proposed fix: fake the old behaviour when didFail is called

[Alexey Proskuryakov]

Comment on attachment 27869 [details]
Proposed fix: fake the old behaviour when didFail is called

> +        Safari 2 did not make a difference between loading nonexistent or existent files from an XHR point of view.

Should this mention Safari 3, as in this bug report?

> +    // For compatibility we fake a correct load for nonexistent local files (see bug 22475).
> +    // We do not dispatch a readyState event for LOADING to match the normal local file handling.
> +    if (m_url.isLocalFile() && !fileExists(m_url.string())) {

How does this fileExists() check change the behavior? Other common reasons for a local file to fail loading that come to mind are permissions issues and trying to load a directory instead of a file. It would be nice to have a test for the latter case at least, to verify that it doesn't need this special treatment.

> Index: LayoutTests/fast/loader/xmlhttprequest-nonexistent-file-expected.txt

It's funny that some non-HTTP XHR tests are in fast/loader, and others are in fast/dom.

[Julien Chaffraix]

> > +        Safari 2 did not make a difference between loading nonexistent or existent files from an XHR point of view.
> 
> Should this mention Safari 3, as in this bug report?

Oh, right, the current stable Safari version is 3.2.1. I will correct it.
 
> > +    // For compatibility we fake a correct load for nonexistent local files (see bug 22475).
> > +    // We do not dispatch a readyState event for LOADING to match the normal local file handling.
> > +    if (m_url.isLocalFile() && !fileExists(m_url.string())) {
> 
> How does this fileExists() check change the behavior? Other common reasons for
> a local file to fail loading that come to mind are permissions issues and
> trying to load a directory instead of a file. It would be nice to have a test
> for the latter case at least, to verify that it doesn't need this special
> treatment.

The idea was to make the changes as small as possible because
1) this area is not covered by the XHR spec.
2) browsers have crazy behaviour in such a corner case (I have filed a bug about FF throwing a cross-site exception!).

After some testing, I will side with you on this one as fileExist returns false in the 3 cases. Thus it will just hinder us (I think of windows that may have different rules).

> > Index: LayoutTests/fast/loader/xmlhttprequest-nonexistent-file-expected.txt
> 
> It's funny that some non-HTTP XHR tests are in fast/loader, and others are in
> fast/dom.

There was only 5 test cases so it was not worth having an xmlhttprequest directory. Maybe with 1 or 2 more it is time for a change :-)

[Julien Chaffraix]

Created attachment 27895 [details]
New version updated with ap's comments

[Alexey Proskuryakov]

Comment on attachment 27895 [details]
New version updated with ap's comments

Test results don't match expectations here: readyState 4 is only logged once. Also, I'm pretty sure that interleaving of the two requests' results is timing dependent and unreliable. You need waitUntilDone()/notifyDone(), and it would be better to start the second request once the first is done (or to normalize the results in some other way, e.g. by logging into separate DIVs).

> +    // For compatibility we fake a correct load for local files in error (see bug 22475).
> +    // We do not dispatch a readyState event for LOADING to match the normal local file handling.
> +    if (m_url.isLocalFile()) {

Should this say "file: URLs" instead of local files? For one thing, these can be directories, and they can be on remote server volumes that are mounted locally. And yes, I'm unhappy about the existing method name, too.

I've performed some testing now, and I wonder why this is limited to local files at all. I've tried loading an http resource from a non-existent domain (to make sure I get a network error, not a 404 response), and both Safari 3 and Firefox 3 dispatched readystate events for states 2 and 4 - and ToT WebKit didn't. Unless I did something wrong in my test, this fix should not be limited to local files.

[Alexey Proskuryakov]

(I didn't test IE or check what the spec says in HTTP case though).

[Julien Chaffraix]

(In reply to comment #9)
> (From update of attachment 27895 [details] [review])
> Test results don't match expectations here: readyState 4 is only logged once.
> Also, I'm pretty sure that interleaving of the two requests' results is timing
> dependent and unreliable. You need waitUntilDone()/notifyDone(), and it would
> be better to start the second request once the first is done (or to normalize
> the results in some other way, e.g. by logging into separate DIVs).

My apologies, I was working on another test case that used synchronous XHR and I have mixed the two in my mind: for me we were doing sync loads (which would have worked nicely). I should have checked the new output instead of blindly seeing the two requests been done and assuming it was working.
Of course, this test needs to serialize the 2 requests and the waitUntilDone()/notifyFinished() as you were pointing out.

> > +    // For compatibility we fake a correct load for local files in error (see bug 22475).
> > +    // We do not dispatch a readyState event for LOADING to match the normal local file handling.
> > +    if (m_url.isLocalFile()) {
> 
> Should this say "file: URLs" instead of local files? For one thing, these can
> be directories, and they can be on remote server volumes that are mounted
> locally. And yes, I'm unhappy about the existing method name, too.

Even though I understand your point and agree with your rationale to some extend (such distinctions are done at the OS level and we cannot say where a file in the filesystem hierarchy is located), I think it is better to stick with the current naming scheme.

> I've performed some testing now, and I wonder why this is limited to local
> files at all. I've tried loading an http resource from a non-existent domain
> (to make sure I get a network error, not a 404 response), and both Safari 3 and
> Firefox 3 dispatched readystate events for states 2 and 4 - and ToT WebKit
> didn't. Unless I did something wrong in my test, this fix should not be limited
> to local files.

I think you should have a look at http://trac.webkit.org/browser/trunk/WebCore/xml/XMLHttpRequest.cpp#L925. The XHR says that we should go from OPENED(1) to DONE(4) directly without dispatching HEADERS_RECEIVED(2) but we are not following it to match Firefox. If FF has changed how it handles network errors, then it would be better to solve this in a separate bug.
I will test it too and file a bug if needed.

[Alexey Proskuryakov]

> I think it is better to stick with the current naming scheme.

OK, but please fix the comment.

> I will test it too and file a bug if needed.

I'm not convinced that these cases should be handled in separate bugs. If other browsers handle local and non-local errors identically, introducing code that makes them different in WebKit looks like a bad move to me.

[Alexey Proskuryakov]

Please note that some of this code is also touched in bug 24089, I'm worried that these patches may clash in subtle or not so subtle ways.

[Julien Chaffraix]

> I've performed some testing now, and I wonder why this is limited to local
> files at all. I've tried loading an http resource from a non-existent domain
> (to make sure I get a network error, not a 404 response), and both Safari 3 and
> Firefox 3 dispatched readystate events for states 2 and 4 - and ToT WebKit
> didn't. Unless I did something wrong in my test, this fix should not be limited
> to local files.

I have done some more testing but I have different results:

- FF 3 and Safari 3 raised a cross-site exception on open() and did not dispatch any readyState event when requesting a non-existent HTTP domain both from file:// or HTTP page
- Opera raised it on send() and dispatched a readyState event for OPEN and HEADERS_RECEIVED
- ToT matches Safari 3 except for the file:// page where it continues the load as if no error happened.


>> I think it is better to stick with the current naming scheme.

> OK, but please fix the comment.

What I meant what that modifying just the comments while leaving the rest untouched will lead to a discrepancy in the naming scheme (the method is still named isLocalFile). As you seem to really what this change, I will make the changes in the XHR class.

> > I will test it too and file a bug if needed.

> I'm not convinced that these cases should be handled in separate bugs. If other
> browsers handle local and non-local errors identically, introducing code that
> makes them different in WebKit looks like a bad move to me.

If they are treated the same, I agree that it should be done in this bug. However during my testing, I would say that they are usually treated differently. That's why I wanted to keep this change as simple as necessary to avoid further regressions and move the other (related) changes to another bug.

[Alexey Proskuryakov]

Created attachment 28385 [details]
test for non-existent domain (run from local file)

(In reply to comment #14)
> I have done some more testing but I have different results:

Attaching my test.

> If they are treated the same, I agree that it should be done in this bug.
> However during my testing, I would say that they are usually treated
> differently.

OK, I'm glad we don't disagree here.

> What I meant what that modifying just the comments while leaving the rest
> untouched will lead to a discrepancy in the naming scheme (the method is still
> named isLocalFile). As you seem to really what this change, I will make the
> changes in the XHR class.

I'm not sure if we are on the same page here. isLocalFile() is not a perfectly descriptive name, but stuffing all fine details in its name seems impractical to me. Since fine details turn out to be so important in XHR case, I think that we should document what we expect from this function precisely.

[Julien Chaffraix]

(In reply to comment #15)
> Created an attachment (id=28385) [review]
> test for non-existent domain (run from local file)

Now I see what is different with my test. You are asking Gecko to authorize XMLHttpRequest from file:// to HTTP. If you forget that, it will reject the request without even looking at the domain.

Looking at the result for Safari 3, it seems that we have changed how we handle this case: we used to match FF by raising an exception but now we continue the load and call the error handler. I remember seeing such a change but I could not pinpoint it. Do you remember what the rationale was?

> > What I meant what that modifying just the comments while leaving the rest
> > untouched will lead to a discrepancy in the naming scheme (the method is still
> > named isLocalFile). As you seem to really what this change, I will make the
> > changes in the XHR class.
> 
> I'm not sure if we are on the same page here. isLocalFile() is not a perfectly
> descriptive name, but stuffing all fine details in its name seems impractical
> to me. Since fine details turn out to be so important in XHR case, I think that
> we should document what we expect from this function precisely.

That's what I understood. What I find strange is changing only a part of the issue, that is not changing the name (to something like isFileURI()) and just refining our expectation on XHR (not even in the KURL class). If this distinction is important for XHR, it may be of interest to other part of the engine.

[Julien Chaffraix]

We have discussed this a bit further off the bug. I have summed up our conclusions along with the original comment.

> Looking at the result for Safari 3, it seems that we have changed how we handle
> this case: we used to match FF by raising an exception but now we continue the
> load and call the error handler. I remember seeing such a change but I could
> not pinpoint it. Do you remember what the rationale was?

file:// URL always had universal access and I was just thinking of some old related change which is irrelevant here.
Also my results were wrong but we could not find the cause (lI was likely confused by some results). I have tested again and now I agree with Alexey.
It means that the change should not be limited to file:// URL only. Leaving the bug summary as-is but I will add a comment in the ChangeLog to cover this discussion and explain the broader fix.

> That's what I understood. What I find strange is changing only a part of the
> issue, that is not changing the name (to something like isFileURI()) and just
> refining our expectation on XHR (not even in the KURL class). If this
> distinction is important for XHR, it may be of interest to other part of the
> engine.

About this part, we have decided to change the name to isFileURL() as it is clearer (also URI would sound strange in KURL and URI is not used much inside WebCore). I have filed bug 24543 to handle this renaming.

[Julien Chaffraix]

Created attachment 28839 [details]
Updated version: Made the test more reliable, fake the load for file:// URL to cover all cases

[Alexey Proskuryakov]

Comment on attachment 28839 [details]
Updated version: Made the test more reliable, fake the load for file:// URL to cover all cases

> +    // For compatibility we fake a correct load when the XHR is issued from a file:// URL in error (see bug 22475).
> +    if (scriptExecutionContext()->url().isLocalFile()) {

I don't think that this is right. We should do this when the request is sent from an HTTP URL, too. In Safari 3, this immediately raised a permission denied exception, but now we implement cross-origin XHR, so it should work just like in Firefox 3.1 beta.

I tested, and a request from http to a non-existent domain also results in reaching the final state in Firefox.

Also, note this comment in genericError():

    // The spec says we should "Synchronously switch the state to DONE." and then "Synchronously dispatch a readystatechange event on the object"
    // but this does not match Firefox.

What exactly about this behavior doesn't match Firefox? Maybe the comment is wrong, and the new code should be in genericError(), and not in didFail()?

+                xhr.open("GET", "http://non.existent.domain.in.the.web.org", true);

I usually send requests to 127.0.0.1:7 to simulate a host that doesn't have an HTTP server in such tests - this avoids the need to hit the network. Another way to simulate a network error is via a redirect to self.

[Alexey Proskuryakov]

See also: bug 24886.

[Alexey Proskuryakov]

*** Bug 24886 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Created attachment 29157 [details]
proposed fix

This patch fixes both issues without special casing file:// requests. I didn't find any regressions in changed test results.

[David Levin]

Thx! One minor issue:
  sp in WebCore/ChangeLog: "readyststechange"->"readystatechange"

fwiw, other than that it looks good to me.  

[Darin Adler]

Comment on attachment 29157 [details]
proposed fix

r=me

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/42164>.