DevTools: make network interception work when underlying request failed

This triggers Fetch.requestPaused if we get a network error through
URLLoader::OnComplete() instead of network response.

Bug: b/170901677
Change-Id: Ieb1a7beafa627dd17b130494c6daba47948d57c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2478143Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819041}

content/browser/devtools/devtools_url_loader_interceptor.cc

@@ -317,6 +317,7 @@ class InterceptionJob : public network::mojom::URLLoaderClient,
 
   void StartRequest();
   void CancelRequest();
+  void CompleteRequest(const network::URLLoaderCompletionStatus& status);
   void Shutdown();
 
   std::unique_ptr<InterceptedRequestInfo> BuildRequestInfo(
@@ -854,9 +855,10 @@ void InterceptionJob::Detach() {
 
 Response InterceptionJob::InnerContinueRequest(
     std::unique_ptr<Modifications> modifications) {
-  if (!waiting_for_resolution_)
+  if (!waiting_for_resolution_) {
     return Response::ServerError(
         "Invalid state for continueInterceptedRequest");
+  }
   waiting_for_resolution_ = false;
 
   if (state_ == State::kAuthRequired) {
@@ -880,15 +882,15 @@ Response InterceptionJob::InnerContinueRequest(
       status.extended_error_code =
           static_cast<int>(blink::ResourceRequestBlockedReason::kInspector);
     }
-    client_->OnComplete(status);
-    Shutdown();
+    CompleteRequest(status);
     return Response::Success();
   }
 
-  if (modifications->response_headers || modifications->response_body)
+  if (modifications->response_headers || modifications->response_body) {
     return ProcessResponseOverride(std::move(modifications->response_headers),
                                    std::move(modifications->response_body),
                                    modifications->body_offset);
+  }
 
   if (state_ == State::kFollowRedirect) {
     if (modifications->modified_url.isJust()) {
@@ -934,6 +936,10 @@ Response InterceptionJob::InnerContinueRequest(
           "Unable to continue request as is after body is taken");
     }
     // TODO(caseq): report error if other modifications are present.
+    if (response_metadata_->status.error_code) {
+      CompleteRequest(response_metadata_->status);
+      return Response::Success();
+    }
     DCHECK_EQ(State::kResponseReceived, state_);
     DCHECK(!body_reader_);
     client_->OnReceiveResponse(std::move(response_metadata_->head));
@@ -1173,8 +1179,7 @@ void InterceptionJob::SendResponse(scoped_refptr<base::RefCountedMemory> body,
   }
   if (response_metadata_->transfer_size)
     client_->OnTransferSizeUpdated(response_metadata_->transfer_size);
-  client_->OnComplete(response_metadata_->status);
-  Shutdown();
+  CompleteRequest(response_metadata_->status);
 }
 
 void InterceptionJob::ResponseBodyComplete() {
@@ -1267,6 +1272,7 @@ void InterceptionJob::FetchCookies(
 
 void InterceptionJob::NotifyClient(
     std::unique_ptr<InterceptedRequestInfo> request_info) {
+  DCHECK(!waiting_for_resolution_);
   FetchCookies(base::BindOnce(&InterceptionJob::NotifyClientWithCookies,
                               base::Unretained(this), std::move(request_info)));
 }
@@ -1290,6 +1296,12 @@ void InterceptionJob::NotifyClientWithCookies(
   interceptor_->request_intercepted_callback_.Run(std::move(request_info));
 }
 
+void InterceptionJob::CompleteRequest(
+    const network::URLLoaderCompletionStatus& status) {
+  client_->OnComplete(status);
+  Shutdown();
+}
+
 void InterceptionJob::Shutdown() {
   if (interceptor_)
     interceptor_->RemoveJob(current_id_);
@@ -1456,18 +1468,32 @@ void InterceptionJob::OnStartLoadingResponseBody(
 
 void InterceptionJob::OnComplete(
     const network::URLLoaderCompletionStatus& status) {
-  // Essentially ShouldBypassForResponse(), but skip DCHECKs
-  // since this may be called in any state during shutdown.
-  if (!response_metadata_) {
-    client_->OnComplete(status);
-    Shutdown();
-    return;
-  }
-  response_metadata_->status = status;
   // No need to listen to the channel any more, so just reset it, so if the pipe
   // is closed by the other end, |shutdown| isn't run.
   client_receiver_.reset();
   loader_.reset();
+
+  if (!response_metadata_) {
+    // If we haven't seen response and get an error completion,
+    // treat it as a response and intercept (provided response are
+    // being intercepted).
+    if (!(stage_ & InterceptionStage::RESPONSE) || !status.error_code) {
+      CompleteRequest(status);
+      return;
+    }
+    response_metadata_ = std::make_unique<ResponseMetadata>();
+    response_metadata_->status = status;
+    auto request_info = BuildRequestInfo(nullptr);
+    request_info->response_error_code = status.error_code;
+    NotifyClient(std::move(request_info));
+    return;
+  }
+  // Since we're not forwarding OnComplete right now, make sure
+  // we're in the proper state. The completion is due upon client response.
+  DCHECK(state_ == State::kResponseReceived || state_ == State::kResponseTaken);
+  DCHECK(waiting_for_resolution_);
+
+  response_metadata_->status = status;
 }
 
 void InterceptionJob::OnAuthRequest(

third_party/blink/web_tests/http/tests/inspector-protocol/fetch/intercept-after-failure-expected.txt

+Tests that we intercept errors reported instead of response when intercepting responses.
+[mock fetcher] Request to http://127.0.0.1:8000/inspector-protocol/resources/inspector-protocol-page.html, type: Document
+Testing continuing failed request...
+[mock fetcher] Request to http://127.0.0.1:8000/devtools/network/resources/resource-deny.php, type: XHR
+Intercepted GET http://127.0.0.1:8000/devtools/network/resources/resource-deny.php at response, error: AccessDenied
+Fetched responsed with: Error: {}
+Testing failing failed request...
+[mock fetcher] Request to http://127.0.0.1:8000/devtools/network/resources/resource-deny.php, type: XHR
+Intercepted GET http://127.0.0.1:8000/devtools/network/resources/resource-deny.php at response, error: AccessDenied
+Fetched responsed with: Error: {}
+Testing fulfilling failed request...
+[mock fetcher] Request to http://127.0.0.1:8000/devtools/network/resources/resource-deny.php, type: XHR
+Intercepted GET http://127.0.0.1:8000/devtools/network/resources/resource-deny.php at response, error: AccessDenied
+Fetched responsed with: overriden response body
+Testing we're not pausing on errors when only intercepting requests...
+[mock fetcher] Request to http://127.0.0.1:8000/devtools/network/resources/resource-deny.php, type: XHR
+Fetched responsed with: Error: {}
+

third_party/blink/web_tests/http/tests/inspector-protocol/fetch/intercept-after-failure.js

+(async function(testRunner) {
+  const {page, session, dp} = await testRunner.startBlank(
+      `Tests that we intercept errors reported instead of response when intercepting responses.`);
+
+  function isResponse(params) {
+    return "responseErrorReason" in params || "responseStatusCode" in params;
+  }
+
+  async function requestAndDump(expectResponse) {
+    const url = '/devtools/network/resources/resource-deny.php';
+    const responsePromise = session.evaluateAsync(`
+        fetch("${url}").then(r => r.text()).
+                        catch(error => 'Error: ' + JSON.stringify(error))
+    `).then(response => testRunner.log(`Fetched responsed with: ${response}`));
+
+    const requestParams = (await dp.Fetch.onceRequestPaused()).params;
+    if (!requestParams.request.url.endsWith(url)) {
+      testRunner.fail(`Paused at wrong request, got ${requestParams.request.url}, expected ${url}`);
+      return;
+    }
+    if (isResponse(requestParams)) {
+      testRunner.fail(`Paused at wrong phase, expected request, got response`);
+      return;
+    }
+    dp.Fetch.continueRequest({requestId: requestParams.requestId});
+
+    if (expectResponse) {
+      const responseParams = (await dp.Fetch.onceRequestPaused()).params;
+      const phase = isResponse(responseParams) ? "response" : "request";
+      const message = `Intercepted ${responseParams.request.method} ${responseParams.request.url} at ${phase}`;
+      const maybeError = responseParams.responseErrorReason ? `, error: ${responseParams.responseErrorReason}` : '';
+      testRunner.log(message + maybeError);
+    }
+    return {requestId: requestParams.requestId, responsePromise};
+  }
+
+  // This browser-level FetchHelper is only for mocking error responses to the
+  // Fetch handler under test, which is on the renderer target (and gets
+  // inserted on top of the browser one).
+  const FetchHelper = await testRunner.loadScript('resources/fetch-test.js');
+  const helper = new FetchHelper(testRunner, testRunner.browserP());
+  await helper.enable();
+  helper.onceRequest().continueRequest();
+  helper.setLogPrefix("[mock fetcher] ");
+
+  dp.Page.enable();
+  dp.Page.reload();
+  await dp.Page.onceLoadEventFired();
+
+  helper.onRequest(/resource-deny/).fail({
+    errorReason: 'AccessDenied'
+  });
+
+  await dp.Fetch.enable({patterns: [{requestStage: 'Request'}, {requestStage: 'Response'}]});
+
+  {
+    testRunner.log(`Testing continuing failed request...`);
+    const {requestId, responsePromise} = await requestAndDump(true);
+    dp.Fetch.continueRequest({requestId});
+    await responsePromise;
+  }
+
+  {
+    testRunner.log(`Testing failing failed request...`);
+    const {requestId, responsePromise} = await requestAndDump(true);
+    dp.Fetch.failRequest({requestId, errorReason: 'Aborted'});
+    await responsePromise;
+  }
+
+  {
+    testRunner.log(`Testing fulfilling failed request...`);
+    const {requestId, responsePromise} = await requestAndDump(true);
+    dp.Fetch.fulfillRequest({
+      requestId,
+      responseCode: 200,
+      responseHeaders: [],
+      body: btoa("overriden response body")
+    });
+    await responsePromise;
+  }
+
+  {
+    testRunner.log(`Testing we're not pausing on errors when only intercepting requests...`);
+    await dp.Fetch.enable({patterns: [{requestStage: 'Request'}]});
+    dp.Fetch.onRequestPaused(event => {
+      if (isResponse(event.params))
+        testRunner.fail(`Unexpected Fetch.requestPaused event for error response`);
+    });
+    const {responsePromise} = await requestAndDump(false);
+    await responsePromise;
+  }
+  testRunner.completeTest();
+})