http/tests/security/xss-DENIED-xsl-document-redirect.xml fails with NetworkProcess
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 May 2014 03:59:11 +0000 (03:59 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 May 2014 03:59:11 +0000 (03:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132523

Patch by Myles C. Maxfield <mmaxfield@apple.com> on 2014-05-22
Reviewed by Alexey Proskuryakov.

Source/WebCore:
We should not manufacture a response in the event of an error during a
synchronous XHR. In addition, this test removes two places that are
sensitive to such a manufactured response.

Updates test expectations.

* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::loadRequest): Don't inspect a
loader response if there is an error
* platform/network/cf/ResourceHandleCFNet.cpp:
(WebCore::ResourceHandle::platformLoadResourceSynchronously): Do not
manufacture a response
* platform/network/mac/ResourceHandleMac.mm:
(WebCore::ResourceHandle::platformLoadResourceSynchronously): Do not
manufacture a response
* xml/XSLTProcessorLibxslt.cpp:
(WebCore::docLoaderFunc): Only use the response's URL if no error
occurred.

LayoutTests:
Two tests have been rebaselined, and re-enable test that now passes. Because of
r23889, synchronous XHRs for file: URLs purposely don't invoke error handlers.

* fast/xmlhttprequest/xmlhttprequest-nonexistent-file-expected.txt: Updated
incorrect test description
* fast/xmlhttprequest/resources/xmlhttprequest-nonexistent-file-real.html: Ditto
* platform/mac-wk2/TestExpectations: Re-enable test that now passes.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@169243 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-nonexistent-file-real.html
LayoutTests/fast/xmlhttprequest/xmlhttprequest-nonexistent-file-expected.txt
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentThreadableLoader.cpp
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp
Source/WebCore/platform/network/mac/ResourceHandleMac.mm
Source/WebCore/xml/XSLTProcessorLibxslt.cpp

index 35c881f..537e985 100644 (file)
@@ -1,3 +1,18 @@
+2014-05-22  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        http/tests/security/xss-DENIED-xsl-document-redirect.xml fails with NetworkProcess
+        https://bugs.webkit.org/show_bug.cgi?id=132523
+
+        Reviewed by Alexey Proskuryakov.
+
+        Two tests have been rebaselined, and re-enable test that now passes. Because of
+        r23889, synchronous XHRs for file: URLs purposely don't invoke error handlers.
+
+        * fast/xmlhttprequest/xmlhttprequest-nonexistent-file-expected.txt: Updated
+        incorrect test description
+        * fast/xmlhttprequest/resources/xmlhttprequest-nonexistent-file-real.html: Ditto
+        * platform/mac-wk2/TestExpectations: Re-enable test that now passes.
+
 2014-05-22  Simon Fraser  <simon.fraser@apple.com>
 
         Fix the  compositing/transitions/transform-on-large-layer.html test
index 86a9b17..1518ef4 100644 (file)
@@ -71,7 +71,7 @@
     </head>
     <body onload="testXHRNonExistentFile()">
         <p> Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=22475">22475</a>: REGRESSION: Async XMLHttpRequest never finishes on nonexistent files anymore </p>
-        <p> In both cases, readyState 4 should be reached, and error handler should be invoked. </p>
+        <p> In both cases, readyState 4 should be reached. Because one of the requests is synchronous and for a file: URL, only a single error handler should be invoked. </p>
         <div id="console"/>
     </body>
 </html>
index 28c03dc..4758c99 100644 (file)
@@ -1,7 +1,7 @@
 
 Bug 22475: REGRESSION: Async XMLHttpRequest never finishes on nonexistent files anymore
 
-In both cases, readyState 4 should be reached, and error handler should be invoked.
+In both cases, readyState 4 should be reached. Because one of the requests is synchronous and for a file: URL, only a single error handler should be invoked.
 
 Doing an XHR to a nonexistent file.
 ReadyState handler: readyState = 1
index 2c86e4e..b3ac417 100644 (file)
@@ -407,9 +407,6 @@ webkit.org/b/123431 [ Mavericks ] http/tests/css/link-css-disabled-value-with-sl
 webkit.org/b/123431 [ Mavericks ] http/tests/local/link-stylesheet-load-order-preload.html [ Failure ]
 webkit.org/b/123431 [ Mavericks ] http/tests/local/link-stylesheet-load-order.html [ Failure ]
 
-# passes but has different logging
-[ Mavericks ] http/tests/security/xss-DENIED-xsl-document-redirect.xml [ Failure ]
-
 # test results in DRT don't match in browser behavior, test seems broken
 fast/dom/Window/mozilla-focus-blur.html [ Failure ]
 
index 089e725..908abd1 100644 (file)
@@ -1,3 +1,29 @@
+2014-05-22  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        http/tests/security/xss-DENIED-xsl-document-redirect.xml fails with NetworkProcess
+        https://bugs.webkit.org/show_bug.cgi?id=132523
+
+        Reviewed by Alexey Proskuryakov.
+
+        We should not manufacture a response in the event of an error during a
+        synchronous XHR. In addition, this test removes two places that are
+        sensitive to such a manufactured response.
+
+        Updates test expectations.
+
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::loadRequest): Don't inspect a
+        loader response if there is an error
+        * platform/network/cf/ResourceHandleCFNet.cpp:
+        (WebCore::ResourceHandle::platformLoadResourceSynchronously): Do not
+        manufacture a response
+        * platform/network/mac/ResourceHandleMac.mm:
+        (WebCore::ResourceHandle::platformLoadResourceSynchronously): Do not
+        manufacture a response
+        * xml/XSLTProcessorLibxslt.cpp:
+        (WebCore::docLoaderFunc): Only use the response's URL if no error
+        occurred.
+
 2014-05-22  Myles C. Maxfield  <litherum@gmail.com>
 
         Unreviewed typo fix.
index f15fc18..83009fb 100644 (file)
@@ -410,9 +410,14 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Secur
 
     InspectorInstrumentation::documentThreadableLoaderStartedLoadingForClient(&m_document, identifier, m_client);
 
-    // No exception for file:/// resources, see <rdar://problem/4962298>.
-    // Also, if we have an HTTP response, then it wasn't a network error in fact.
-    if (!error.isNull() && !requestURL.isLocalFile() && response.httpStatusCode() <= 0) {
+    if (!error.isNull() && response.httpStatusCode() <= 0) {
+        if (requestURL.isLocalFile()) {
+            // We don't want XMLHttpRequest to raise an exception for file:// resources, see <rdar://problem/4962298>.
+            // FIXME: XMLHttpRequest quirks should be in XMLHttpRequest code, not in DocumentThreadableLoader.cpp.
+            didReceiveResponse(identifier, response);
+            didFinishLoading(identifier, 0.0);
+            return;
+        }
         m_client->didFail(error);
         return;
     }
index c0b6122..909e122 100644 (file)
@@ -512,14 +512,6 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
 
     if (error.isNull())
         response = client->response();
-    else {
-        response = ResourceResponse(request.url(), String(), 0, String(), String());
-        // FIXME: ResourceHandleMac also handles authentication errors by setting code to 401. CFNet version should probably do the same.
-        if (error.domain() == String(kCFErrorDomainCFNetwork))
-            response.setHTTPStatusCode(error.errorCode());
-        else
-            response.setHTTPStatusCode(404);
-    }
 
     data.swap(client->mutableData());
 }
index 556ebab..8466706 100644 (file)
@@ -426,25 +426,6 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
 
     if (error.isNull())
         response = client->response();
-    else {
-        // FIXME: We might not ever need to manufacture a response: This might all be dead code.
-        // When exploring removal of this code, we should substitute appropriate ASSERTs.
-        response = ResourceResponse(request.url(), String(), 0, String(), String());
-        if (error.domain() == String(NSURLErrorDomain))
-            switch (error.errorCode()) {
-            case NSURLErrorUserCancelledAuthentication:
-                // FIXME: We don't need to manufacture a 401 response if we say continueWithoutCredentialForAuthenticationChallenge:
-                // in which case we'll get the real failure response. A reading of SynchronousLoaderClient.mm suggests we already do this.
-                response.setHTTPStatusCode(401);
-                break;
-            default:
-                response.setHTTPStatusCode(error.errorCode());
-            }
-        else {
-            // FIXME: This is wrong. We shouldn't need to ever make up a 404.
-            response.setHTTPStatusCode(404);
-        }
-     }
 
     data.swap(client->mutableData());
 }
index ac25f39..cda2b47 100644 (file)
@@ -128,7 +128,10 @@ static xmlDocPtr docLoaderFunc(const xmlChar* uri,
         bool requestAllowed = globalCachedResourceLoader->frame() && globalCachedResourceLoader->document()->securityOrigin()->canRequest(url);
         if (requestAllowed) {
             globalCachedResourceLoader->frame()->loader().loadResourceSynchronously(url, AllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, error, response, data);
-            requestAllowed = globalCachedResourceLoader->document()->securityOrigin()->canRequest(response.url());
+            if (error.isNull())
+                requestAllowed = globalCachedResourceLoader->document()->securityOrigin()->canRequest(response.url());
+            else
+                data.clear();
         }
         if (!requestAllowed) {
             data.clear();