Bug 1374943 - Fetch needs to handle redirect=error not resulting in NS_FAILED. r=bkelly draft fetch-stop-crash
authorAndrew Sutherland <asutherland@asutherland.org>
Wed, 21 Jun 2017 08:28:34 -0400
changeset 403797 ceb73ff36cd7fe18c4eb8c1f34c217ec4813950a
parent 403796 464b2a3c25aa1065760d9ecbb0870bca4a66c62e
push id4
push userbugmail@asutherland.org
push dateMon, 26 Jun 2017 06:55:04 +0000
reviewersbkelly
bugs1374943
milestone56.0a1
Bug 1374943 - Fetch needs to handle redirect=error not resulting in NS_FAILED. r=bkelly The included test crashes without the included fix if run with --disable-e10s. e10s doesn't crash because HttpChannelChild examines the return value of the call to the listener's OnStartRequest method and invokes Cancel() if it NS_FAILED. This is a divergence between e10s and non-e10s. See the bug for more details and discussion.
dom/fetch/FetchDriver.cpp
dom/tests/mochitest/fetch/file_fetch_cached_redirect.html
dom/tests/mochitest/fetch/file_fetch_cached_redirect.html^headers^
dom/tests/mochitest/fetch/mochitest.ini
dom/tests/mochitest/fetch/test_fetch_cached_redirect.html
dom/tests/mochitest/fetch/test_fetch_cached_redirect.js
--- a/dom/fetch/FetchDriver.cpp
+++ b/dom/fetch/FetchDriver.cpp
@@ -752,17 +752,21 @@ FetchDriver::OnDataAvailable(nsIRequest*
 }
 
 NS_IMETHODIMP
 FetchDriver::OnStopRequest(nsIRequest* aRequest,
                            nsISupports* aContext,
                            nsresult aStatusCode)
 {
   workers::AssertIsOnMainThread();
-  if (NS_FAILED(aStatusCode)) {
+
+  // We need to check mObserver, which is nulled by FailWithNetworkError(),
+  // because in the case of "error" redirect mode, aStatusCode may be NS_OK but
+  // mResponse will definitely be null so we must not take the else branch.
+  if (NS_FAILED(aStatusCode) || !mObserver) {
     nsCOMPtr<nsIAsyncOutputStream> outputStream = do_QueryInterface(mPipeOutputStream);
     if (outputStream) {
       outputStream->CloseWithStatus(NS_BINDING_FAILED);
     }
 
     // We proceed as usual here, since we've already created a successful response
     // from OnStartRequest.
   } else {
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/fetch/file_fetch_cached_redirect.html
@@ -0,0 +1,1 @@
+<html><body>My contents don't matter.  Only my header matters!</body></html>
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/fetch/file_fetch_cached_redirect.html^headers^
@@ -0,0 +1,3 @@
+HTTP 302 Redirect
+Location: //example.org/target_does_not_matter.html
+Cache-Control: max-age=10
--- a/dom/tests/mochitest/fetch/mochitest.ini
+++ b/dom/tests/mochitest/fetch/mochitest.ini
@@ -1,16 +1,19 @@
 [DEFAULT]
 support-files =
   fetch_test_framework.js
   file_fetch_controller.html
+  file_fetch_cached_redirect.html
+  file_fetch_cached_redirect.html^headers^
   file_fetch_csp_block_frame.html
   file_fetch_csp_block_frame.html^headers^
   test_fetch_basic.js
   test_fetch_basic_http.js
+  test_fetch_cached_redirect.js
   test_fetch_cors.js
   file_fetch_observer.html
   test_formdataparsing.js
   test_headers_common.js
   test_request.js
   test_response.js
   utils.js
   nested_worker_wrapper.js
@@ -45,16 +48,17 @@ support-files =
 [test_headers_sw_reroute.html]
 [test_headers_mainthread.html]
 [test_fetch_basic.html]
 [test_fetch_basic_sw_reroute.html]
 [test_fetch_basic_sw_empty_reroute.html]
 [test_fetch_basic_http.html]
 [test_fetch_basic_http_sw_reroute.html]
 [test_fetch_basic_http_sw_empty_reroute.html]
+[test_fetch_cached_redirect.html]
 [test_fetch_controller.html]
 [test_fetch_cors.html]
 skip-if = toolkit == 'android' && debug # Bug 1210282
 [test_fetch_cors_sw_reroute.html]
 skip-if = toolkit == 'android' && debug # Bug 1210282
 [test_fetch_cors_sw_empty_reroute.html]
 skip-if = toolkit == 'android' && debug # Bug 1210282
 [test_fetch_csp_block.html]
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/fetch/test_fetch_cached_redirect.html
@@ -0,0 +1,22 @@
+<!--
+  Any copyright is dedicated to the Public Domain.
+  http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Bug 1374943 - Test fetch cached redirects</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+<script type="text/javascript" src="utils.js"> </script>
+<script type="text/javascript" src="fetch_test_framework.js"> </script>
+<script class="testbody" type="text/javascript">
+testScript("test_fetch_cached_redirect.js");
+</script>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/fetch/test_fetch_cached_redirect.js
@@ -0,0 +1,21 @@
+async function testCachedRedirectErrorMode() {
+  // This is a file that returns a 302 to someplace else and will be cached.
+  const REDIRECTING_URL = "file_fetch_cached_redirect.html";
+
+  let firstResponse = await(fetch(REDIRECTING_URL, { redirect: "manual" }));
+  // okay, now it should be in the cahce.
+  try {
+    let secondResponse = await(fetch(REDIRECTING_URL, { redirect: "error" }));
+  } catch(ex) {
+
+  }
+
+  ok(true, "didn't crash");
+}
+
+function runTest() {
+
+  return Promise.resolve()
+    .then(testCachedRedirectErrorMode)
+    // Put more promise based tests here.
+}