- Revision
- 206524
- Author
- [email protected]
- Date
- 2016-09-28 09:37:26 -0700 (Wed, 28 Sep 2016)
Log Message
WebCore::ResourceErrorBase::setType is crashing
https://bugs.webkit.org/show_bug.cgi?id=162484
<rdar://problem/28390828>
Patch by Youenn Fablet <[email protected]> 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (206523 => 206524)
--- trunk/LayoutTests/ChangeLog 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/LayoutTests/ChangeLog 2016-09-28 16:37:26 UTC (rev 206524)
@@ -1,3 +1,15 @@
+2016-09-28 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
[MSE][Mac] In SourceBufferPrivateAVFObjC::abort(), support reseting parser to the last appended initialization segment.
Added: trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt (0 => 206524)
--- trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight-expected.txt 2016-09-28 16:37:26 UTC (rev 206524)
@@ -0,0 +1,3 @@
+
+PASS Testing timing out preflight
+
Added: trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html (0 => 206524)
--- trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html (rev 0)
+++ trunk/LayoutTests/http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html 2016-09-28 16:37:26 UTC (rev 206524)
@@ -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=""
+ <script src=""
+ </head>
+ <body>
+ <div id="log"></div>
+ <script>
+ var test = async_test("Testing timing out preflight")
+ test.step(function() {
+ var client = new XMLHttpRequest()
+ var url = ""
+ 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>
+
Modified: trunk/LayoutTests/tests-options.json (206523 => 206524)
--- trunk/LayoutTests/tests-options.json 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/LayoutTests/tests-options.json 2016-09-28 16:37:26 UTC (rev 206524)
@@ -1,4 +1,7 @@
{
+ "http/tests/xmlhttprequest/on-network-timeout-error-during-preflight.html": [
+ "slow"
+ ],
"imported/w3c/syntax/parsing/html5lib_adoption01.html": [
"slow"
],
@@ -503,4 +506,4 @@
"imported/w3c/web-platform-tests/media-source/mediasource-redundant-seek.html": [
"slow"
]
-}
\ No newline at end of file
+}
Modified: trunk/Source/WebCore/ChangeLog (206523 => 206524)
--- trunk/Source/WebCore/ChangeLog 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/Source/WebCore/ChangeLog 2016-09-28 16:37:26 UTC (rev 206524)
@@ -1,3 +1,28 @@
+2016-09-28 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
PiP shows incorrect state of play button.
Modified: trunk/Source/WebCore/loader/CrossOriginPreflightChecker.cpp (206523 => 206524)
--- trunk/Source/WebCore/loader/CrossOriginPreflightChecker.cpp 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/Source/WebCore/loader/CrossOriginPreflightChecker.cpp 2016-09-28 16:37:26 UTC (rev 206524)
@@ -91,7 +91,11 @@
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 @@
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;
}
Modified: trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp (206523 => 206524)
--- trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp 2016-09-28 16:37:26 UTC (rev 206524)
@@ -341,7 +341,6 @@
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);
Modified: trunk/Source/WebCore/platform/network/ResourceErrorBase.h (206523 => 206524)
--- trunk/Source/WebCore/platform/network/ResourceErrorBase.h 2016-09-28 16:34:06 UTC (rev 206523)
+++ trunk/Source/WebCore/platform/network/ResourceErrorBase.h 2016-09-28 16:37:26 UTC (rev 206524)
@@ -53,6 +53,7 @@
};
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; }
