WebCore::ResourceErrorBase::setType is crashing
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Sep 2016 16:37:26 +0000 (16:37 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Sep 2016 16:37:26 +0000 (16:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162484
<rdar://problem/28390828>

Patch by Youenn Fablet <youenn@apple.com> on 2016-09-28
Reviewed by Alex Christensen.

Source/WebCore:

Test: http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html

Behavior is slightly changed as we are no longer casting Timeout preflight errors as AccessControl errors.
This is more inline with fetch spec which prescribes to send back any error received by preflight as response error for fetch.

Ideally, we should not need to change errors received during preflight loads but the error type is important for some clients:
- EventSource may try to reconnect if error is not AccessControl
- XMLHttpRequest will send abort events in case of Cancellation errors and timeout events in case of Timeout errors

* loader/CrossOriginPreflightChecker.cpp:
(WebCore::CrossOriginPreflightChecker::notifyFinished): Setting error type to AccessControl except in case of Timeout.
(WebCore::CrossOriginPreflightChecker::doPreflight): Ditto.
* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::preflightFailure): Removing ASSERT since Timeout errors may be returned.
* platform/network/ResourceErrorBase.h:
(WebCore::ResourceErrorBase::isGeneral): New getter.

LayoutTests:

* http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt: Added.
* http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html: Added.
* tests-options.json: Marking test as slow.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html [new file with mode: 0644]
LayoutTests/tests-options.json
Source/WebCore/ChangeLog
Source/WebCore/loader/CrossOriginPreflightChecker.cpp
Source/WebCore/loader/DocumentThreadableLoader.cpp
Source/WebCore/platform/network/ResourceErrorBase.h

index 94653d5..4174350 100644 (file)
@@ -1,3 +1,15 @@
+2016-09-28  Youenn Fablet  <youenn@apple.com>
+
+        WebCore::ResourceErrorBase::setType is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=162484
+        <rdar://problem/28390828>
+
+        Reviewed by Alex Christensen.
+
+        * http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt: Added.
+        * http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html: Added.
+        * tests-options.json: Marking test as slow.
+
 2016-09-28  Jer Noble  <jer.noble@apple.com>
 
         [MSE][Mac] In SourceBufferPrivateAVFObjC::abort(), support reseting parser to the last appended initialization segment.
diff --git a/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt b/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt
new file mode 100644 (file)
index 0000000..a237142
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Testing timing out preflight 
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html b/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html
new file mode 100644 (file)
index 0000000..34e34f2
--- /dev/null
@@ -0,0 +1,30 @@
+<!doctype html>
+<html>
+  <head>
+    <title>XMLHttpRequest: A timeout network error during preflight should end up in a timeout event</title>
+    <script src="/js-test-resources/testharness.js"></script>
+    <script src="/js-test-resources/testharnessreport.js"></script>
+  </head>
+  <body>
+    <div id="log"></div>
+    <script>
+      var test = async_test("Testing timing out preflight")
+      test.step(function() {
+        var client = new XMLHttpRequest()
+        var url = "http://localhost:8000/misc/resources/delayed-log.php?delay=10000000"
+        client.open("GET", url, true)
+        client.setRequestHeader("X-Custom", "test")
+        client.hasTimedout = false;
+        client.ontimeout = test.step_func(function () {
+            client.hasTimedout = true
+        })
+        client.onloadend = test.step_func(function () {
+            assert_true(client.hasTimedout, "xhr should have timed out")
+            test.done()
+        })
+        client.send(null)
+      })
+    </script>
+  </body>
+</html>
+
index 58f8352..a425f97 100644 (file)
@@ -1,4 +1,7 @@
 {
+    "http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html": [
+        "slow"
+    ],
     "imported/w3c/syntax/parsing/html5lib_adoption01.html": [
         "slow"
     ],
     "imported/w3c/web-platform-tests/media-source/mediasource-redundant-seek.html": [
         "slow"
     ]
-}
\ No newline at end of file
+}
index 1327093..a4b59aa 100644 (file)
@@ -1,3 +1,28 @@
+2016-09-28  Youenn Fablet  <youenn@apple.com>
+
+        WebCore::ResourceErrorBase::setType is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=162484
+        <rdar://problem/28390828>
+
+        Reviewed by Alex Christensen.
+
+        Test: http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html
+
+        Behavior is slightly changed as we are no longer casting Timeout preflight errors as AccessControl errors.
+        This is more inline with fetch spec which prescribes to send back any error received by preflight as response error for fetch.
+
+        Ideally, we should not need to change errors received during preflight loads but the error type is important for some clients:
+        - EventSource may try to reconnect if error is not AccessControl
+        - XMLHttpRequest will send abort events in case of Cancellation errors and timeout events in case of Timeout errors
+
+        * loader/CrossOriginPreflightChecker.cpp:
+        (WebCore::CrossOriginPreflightChecker::notifyFinished): Setting error type to AccessControl except in case of Timeout.
+        (WebCore::CrossOriginPreflightChecker::doPreflight): Ditto.
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::preflightFailure): Removing ASSERT since Timeout errors may be returned.
+        * platform/network/ResourceErrorBase.h:
+        (WebCore::ResourceErrorBase::isGeneral): New getter.
+
 2016-09-28  Jer Noble  <jer.noble@apple.com>
 
         PiP shows incorrect state of play button.
index 0f6c129..ea28d43 100644 (file)
@@ -91,7 +91,11 @@ void CrossOriginPreflightChecker::notifyFinished(CachedResource* resource)
     ASSERT_UNUSED(resource, resource == m_resource);
     if (m_resource->loadFailedOrCanceled()) {
         ResourceError preflightError = m_resource->resourceError();
-        preflightError.setType(ResourceError::Type::AccessControl);
+        // If the preflight was cancelled by underlying code, it probably means the request was blocked due to some access control policy.
+        // FIXME:: According fetch, we should just pass the error to the layer above. But this may impact some clients like XHR or EventSource.
+        if (preflightError.isNull() || preflightError.isCancellation() || preflightError.isGeneral())
+            preflightError.setType(ResourceError::Type::AccessControl);
+
         m_loader.preflightFailure(m_resource->identifier(), preflightError);
         return;
     }
@@ -133,9 +137,11 @@ void CrossOriginPreflightChecker::doPreflight(DocumentThreadableLoader& loader,
 
     unsigned identifier = loader.document().frame()->loader().loadResourceSynchronously(preflightRequest, DoNotAllowStoredCredentials, ClientCredentialPolicy::CannotAskClientForCredentials, error, response, data);
 
-    // FIXME: Investigate why checking for response httpStatusCode here. In particular, can we have a not-null error and a 2XX response.
-    if (!error.isNull() && response.httpStatusCode() <= 0) {
-        error.setType(ResourceError::Type::AccessControl);
+    if (!error.isNull()) {
+        // If the preflight was cancelled by underlying code, it probably means the request was blocked due to some access control policy.
+        // FIXME:: According fetch, we should just pass the error to the layer above. But this may impact some clients like XHR or EventSource.
+        if (error.isCancellation() || error.isGeneral())
+            error.setType(ResourceError::Type::AccessControl);
         loader.preflightFailure(identifier, error);
         return;
     }
index 7ac25f4..3f6cbaa 100644 (file)
@@ -341,7 +341,6 @@ void DocumentThreadableLoader::preflightSuccess(ResourceRequest&& request)
 
 void DocumentThreadableLoader::preflightFailure(unsigned long identifier, const ResourceError& error)
 {
-    ASSERT(error.isAccessControl());
     m_preflightChecker = Nullopt;
 
     InspectorInstrumentation::didFailLoading(m_document.frame(), m_document.frame()->loader().documentLoader(), identifier, error);
index c0c31df..658c9ca 100644 (file)
@@ -53,6 +53,7 @@ public:
     };
 
     bool isNull() const { return m_type == Type::Null; }
+    bool isGeneral() const { return m_type == Type::General; }
     bool isAccessControl() const { return m_type == Type::AccessControl; }
     bool isCancellation() const { return m_type == Type::Cancellation; }
     bool isTimeout() const { return m_type == Type::Timeout; }