Bug 86344

Created attachment 141678 [details] Patch

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?

(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.

(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?

(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.

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.

(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).

Created attachment 141904 [details] Patch

Created attachment 141905 [details] Patch

> 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.

(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?

Created attachment 142172 [details] Patch

Comment on attachment 142172 [details] Patch Much better

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

Comment on attachment 142172 [details] Patch Clearing flags on attachment: 142172 Committed r117246: <http://trac.webkit.org/changeset/117246>

All reviewed patches have been landed. Closing bug.