[BlackBerry] xhr request to non existent file response is 0 and not 404.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jason Liu]

Get 2 reponses captured instead of 4
We seem to be ignoring the response when an error code comes back. In this case
response should be 404 not 0

I'm seeing this on the cibc site. Possible root cause to the login issue


<script>
var d = (function () {  
  var xhr = XMLHttpRequest ? new XMLHttpRequest() : 
                             new ActiveXObject("Microsoft.XMLHTTP"); 
  xhr.open("HEAD", 'nothing.txt', true); 
  xhr.onreadystatechange = function(){ 

    console.log(xhr.responseText); 

    console.log(xhr, xhr.status); 

  }; 
  xhr.onerror = function () { 
    error(xhr, xhr.status); 
  }; 
  xhr.send(); 
});
d();
</script>

[Jason Liu]

Created attachment 141678 [details]
Patch

[George Staikos]

Comment on attachment 141678 [details]
Patch

1) Why are you changing a platform independent test and then a platform specific file?
2) Should this really be special cased to HEAD?  Seems like a hack around a deeper problem.  Is the issue really just that HEAD returns no data?  Is this a CURL quirk?

[Jason Liu]

(In reply to comment #2)
> (From update of attachment 141678 [details])
> 1) Why are you changing a platform independent test and then a platform specific file?

This test case is written by myself for "[BlackBerry] Missing readyState 2 when a XMLHttpRequest calls xmlhttp.open("HEAD","notExist.html",true).
    https://bugs.webkit.org/show_bug.cgi?id=83866"

And I think this bug is similar to 83866. Just another point to fix.

> 2) Should this really be special cased to HEAD?  Seems like a hack around a deeper problem.  Is the issue really just that HEAD returns no data?  Is this a CURL quirk?

If it is not HEAD, we can receive data and the response will be sent to webkit.

[George Staikos]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 141678 [details] [details])
> > 1) Why are you changing a platform independent test and then a platform specific file?
> 
> This test case is written by myself for "[BlackBerry] Missing readyState 2 when a XMLHttpRequest calls xmlhttp.open("HEAD","notExist.html",true).
>     https://bugs.webkit.org/show_bug.cgi?id=83866"
> 
> And I think this bug is similar to 83866. Just another point to fix.

  Ok, so the testcase was not quite correct to begin with.  You should maybe be more clear in your commit log.

> > 2) Should this really be special cased to HEAD?  Seems like a hack around a deeper problem.  Is the issue really just that HEAD returns no data?  Is this a CURL quirk?
> 
> If it is not HEAD, we can receive data and the response will be sent to webkit.

   Is HEAD the only method that will behave like this?  It still feels odd to me.  What if HTTP GET or POST truly returns an empty file?

[Jason Liu]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 141678 [details] [details] [details])
> > > 1) Why are you changing a platform independent test and then a platform specific file?
> > 
> > This test case is written by myself for "[BlackBerry] Missing readyState 2 when a XMLHttpRequest calls xmlhttp.open("HEAD","notExist.html",true).
> >     https://bugs.webkit.org/show_bug.cgi?id=83866"
> > 
> > And I think this bug is similar to 83866. Just another point to fix.
> 
>   Ok, so the testcase was not quite correct to begin with.  You should maybe be more clear in your commit log.
> 
> > > 2) Should this really be special cased to HEAD?  Seems like a hack around a deeper problem.  Is the issue really just that HEAD returns no data?  Is this a CURL quirk?
> > 
> > If it is not HEAD, we can receive data and the response will be sent to webkit.
> 
>    Is HEAD the only method that will behave like this?  It still feels odd to me.  What if HTTP GET or POST truly returns an empty file?
    If HTTP GET/POST returns an empty file, status code will be 2xx.
    And isError() will be false, So we can send response correctly.

[Joe Mason]

The updated LayoutTest looks right to me, but I don't like the webkit change.

If I understand you right:

For a non-existent file, curl is returning 404 for GET  but 0 for HEAD.  I don't see anything in the spec that says it should be doing this, so it sounds like a curl bug.

For a 0-length file, curl is returning 200 (with no body) for GET, and I can't tell from this writeup what it's doing for HEAD.  But I think it should be returning 200 here too. (Obviously with no body.)

So instead of changing NetworkJob, I think we should be converting the 0 to 404 in the platform layer, as soon as it's returned from libcurl.  And we should file a bug against libcurl.

[Joe Mason]

(In reply to comment #6)
> If I understand you right:
> 
> For a non-existent file, curl is returning 404 for GET  but 0 for HEAD.  I don't see anything in the spec that says it should be doing this, so it sounds like a curl bug.

Nevermind, I'm not understanding right.

The problem is in handleNotifyClose:

sendResponseIfNeeded();
if (isClientAvailable()) {
    RecursionGuard guard(m_callingClient);

    if (isError(m_extendedStatusCode) && !m_dataReceived && m_handle->firstRequest().httpMethod() != "HEAD") {
        String domain = m_extendedStatusCode < 0 ? ResourceError::platformErrorDomain : ResourceError::httpErrorDomain;
        ResourceError error(domain, m_extendedStatusCode, m_response.url().string(), m_response.httpStatusText());
        m_handle->client()->didFail(m_handle.get(), error);
    } else
        m_handle->client()->didFinishLoading(m_handle.get(), 0);
}

I'm not sure when that "HEAD" check got added, but it needs to match the check in sendResponseIfNeeded, since that gets called immediately beforehand.

The intent is that on error we either call "didFail" with the error message, if an error code was returned but no data, or "didReceiveResponse" followed by "didFinishLoading", if an error code was returned but there was data to display (such as a custom 404 page).  didReceiveResponse is sent by sendResponseIfNeeded.

So on a HEAD request, which never has data, if a 404 is received we return from sendResponseIfNeeded without calling didReceiveResponse, but then call didFinishLoading.

I guess the HEAD check was added so that didFail is never called on HEAD - it should call didReceiveResponse followed by didFinishLoading even without any data.

And we don't want to get rid of the didFail branch entirely, because for top-level requests that's where we format our custom error page.

So the code change looks good to me.  But I think "isError(m_extendedStatusCode) && !m_dataReceived && m_handle->firstRequest().httpMethod() != "HEAD"" should be put into a helper function so that if it's changed again it only needs to be changed in one place.

Also, are we sure that an XHR "GET" for to a server that returns just 404 with no data doesn't have the same problem?  Maybe we should also change the "HEAD" check to a check for TargetIsMainFrame || TargetIsSubframe (since we only ever need to call didFail and create an error page for errors that will actually be displayed in a frame), or a check for TargetIsXHR (if didFail has side effects that we should be triggering everywhere expect in an XHR).

[Jason Liu]

Created attachment 141904 [details]
Patch

[Jason Liu]

Created attachment 141905 [details]
Patch

[Jason Liu]

> So the code change looks good to me.  But I think "isError(m_extendedStatusCode) && !m_dataReceived && m_handle->firstRequest().httpMethod() != "HEAD"" should be put into a helper function so that if it's changed again it only needs to be changed in one place.
> 
> Also, are we sure that an XHR "GET" for to a server that returns just 404 with no data doesn't have the same problem?  Maybe we should also change the "HEAD" check to a check for TargetIsMainFrame || TargetIsSubframe (since we only ever need to call didFail and create an error page for errors that will actually be displayed in a frame), or a check for TargetIsXHR (if didFail has side effects that we should be triggering everywhere expect in an XHR).

hi,Joe
This is a good suggestion. 
I've written function shouldNotifyClientFailed() for this "if" condition sentence. and a new patch attached.

I tested with google's chrome and Safari.
They don't treat 404 for "GET" without content as a failure, but we do.
So I think we should only treat (status<0) as a failure to XMLHttpRequest instead of adding "HEAD" check.

Do you think so?

Thanks for your help.

[Joe Mason]

(In reply to comment #10)
> I tested with google's chrome and Safari.
> They don't treat 404 for "GET" without content as a failure, but we do.
> So I think we should only treat (status<0) as a failure to XMLHttpRequest instead of adding "HEAD" check.
> 
> Do you think so?

I agree!

Thanks for checking.  Can you add a cross-platform layout test for that too?

[Jason Liu]

Created attachment 142172 [details]
Patch

[George Staikos]

Comment on attachment 142172 [details]
Patch

Much better

[Jason Liu]

(In reply to comment #13)
> (From update of attachment 142172 [details])
> Much better

Thank you.

[WebKit Review Bot]

Comment on attachment 142172 [details]
Patch

Clearing flags on attachment: 142172

Committed r117246: <http://trac.webkit.org/changeset/117246>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.