Fix DRP not retrying requests because of net errors

This CL plumbs OnComplete to DRP url loader throttle to act on the net errors.

Bug: 934293
Change-Id: I31bf231c80ab96e712f5a3e0a50314ec22cbb1d6
Reviewed-on: https://chromium-review.googlesource.com/c/1483735
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634897}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -385,6 +385,9 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertestWithNetworkService,
 class DataReductionProxyFallbackBrowsertest
     : public DataReductionProxyBrowsertest {
  public:
+  using ResponseHook =
+      base::RepeatingCallback<void(net::test_server::BasicHttpResponse*)>;
+
   void SetUpOnMainThread() override {
     // Set up a primary server which will return the Chrome-Proxy header set by
     // SetHeader() and status set by SetStatusCode(). Secondary server will just
@@ -411,6 +414,10 @@ class DataReductionProxyFallbackBrowsertest
     DataReductionProxyBrowsertest::SetUpOnMainThread();
   }
 
+  void SetResponseHook(ResponseHook response_hook) {
+    response_hook_ = response_hook;
+  }
+
   void SetHeader(const std::string& header) { header_ = header; }
 
   void SetStatusCode(net::HttpStatusCode status_code) {
@@ -423,6 +430,8 @@ class DataReductionProxyFallbackBrowsertest
     auto response = std::make_unique<net::test_server::BasicHttpResponse>();
     if (!header_.empty())
       response->AddCustomHeader(chrome_proxy_header(), header_);
+    if (response_hook_)
+      response_hook_.Run(response.get());
     response->set_code(status_code_);
     response->set_content(kPrimaryResponse);
     response->set_content_type("text/plain");
@@ -431,10 +440,33 @@ class DataReductionProxyFallbackBrowsertest
 
   net::HttpStatusCode status_code_ = net::HTTP_OK;
   std::string header_;
+  ResponseHook response_hook_;
   net::EmbeddedTestServer primary_server_;
   net::EmbeddedTestServer secondary_server_;
 };
 
+IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
+                       FallbackProxyUsedOnNetError) {
+  SetResponseHook(
+      base::BindRepeating([](net::test_server::BasicHttpResponse* response) {
+        response->AddCustomHeader("Content-Disposition", "inline");
+        response->AddCustomHeader("Content-Disposition", "form-data");
+      }));
+  base::HistogramTester histogram_tester;
+  ui_test_utils::NavigateToURL(
+      browser(), GURL("http://does.not.resolve/echoheader?Chrome-Proxy"));
+  EXPECT_THAT(GetBody(), kSecondaryResponse);
+  histogram_tester.ExpectUniqueSample(
+      "DataReductionProxy.InvalidResponseHeadersReceived.NetError",
+      -net::ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION, 1);
+
+  // Bad proxy should still be bypassed.
+  SetResponseHook(ResponseHook());
+  ui_test_utils::NavigateToURL(
+      browser(), GURL("http://does.not.resolve/echoheader?Chrome-Proxy"));
+  EXPECT_THAT(GetBody(), kSecondaryResponse);
+}
+
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        FallbackProxyUsedOn500Status) {
   base::HistogramTester histogram_tester;

components/data_reduction_proxy/content/common/data_reduction_proxy_url_loader_throttle.cc

@@ -76,6 +76,14 @@ void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
   if (params::IsWarmupURL(response_url))
     return;
 
+  MaybeRetry(proxy_server, response_head.headers.get(), net::OK, defer);
+}
+
+void DataReductionProxyURLLoaderThrottle::MaybeRetry(
+    const net::ProxyServer& proxy_server,
+    const net::HttpResponseHeaders* headers,
+    net::Error net_error,
+    bool* defer) {
   // The set of data reduction proxy servers to mark as bad prior to
   // restarting the request.
   std::vector<net::ProxyServer> bad_proxies;
@@ -83,10 +91,6 @@ void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
   // TODO(https://crbug.com/721403): Implement retry due to authentication
   // failure.
 
-  // TODO(https://crbug.com/721403): Should be calling this for cases where the
-  // request failed with an error too.
-  net::Error net_error = net::OK;
-
   // TODO(https://crbug.com/721403): Need the actual bad proxies map. Since
   // this is only being used for some metrics logging not a big deal.
   net::ProxyRetryInfoMap proxy_retry_info;
@@ -97,8 +101,8 @@ void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
 
   DataReductionProxyBypassProtocol protocol;
   pending_restart_ = protocol.MaybeBypassProxyAndPrepareToRetry(
-      request_method_, url_chain_, response_head.headers.get(),
-      response_head.proxy_server, net_error, proxy_retry_info,
+      request_method_, url_chain_, headers, proxy_server, net_error,
+      proxy_retry_info,
       manager_->FindConfiguredDataReductionProxy(proxy_server), &bypass_type,
       &data_reduction_proxy_info, &bad_proxies, &pending_restart_load_flags_);
 
@@ -130,6 +134,13 @@ void DataReductionProxyURLLoaderThrottle::WillProcessResponse(
                          is_main_frame_);
 }
 
+void DataReductionProxyURLLoaderThrottle::WillOnCompleteWithError(
+    const network::URLLoaderCompletionStatus& status,
+    bool* defer) {
+  MaybeRetry(status.proxy_server, nullptr,
+             static_cast<net::Error>(status.error_code), defer);
+}
+
 void DataReductionProxyURLLoaderThrottle::MarkProxiesAsBad(
     const std::vector<net::ProxyServer>& bad_proxies,
     base::TimeDelta bypass_duration) {

components/data_reduction_proxy/content/common/data_reduction_proxy_url_loader_throttle.h

@@ -45,8 +45,16 @@ class DataReductionProxyURLLoaderThrottle : public content::URLLoaderThrottle {
   void WillProcessResponse(const GURL& response_url,
                            network::ResourceResponseHead* response_head,
                            bool* defer) override;
+  void WillOnCompleteWithError(const network::URLLoaderCompletionStatus& status,
+                               bool* defer) override;
 
  private:
+  // Retry the request bypassing proxies or falling back to next proxy based on
+  // |net_error| and the response headers.
+  void MaybeRetry(const net::ProxyServer& proxy_server,
+                  const net::HttpResponseHeaders* headers,
+                  net::Error net_error,
+                  bool* defer);
   // Marks |bad_proxies| to be bypassed for |bypass_duration|. Once that action
   // has completed will call OnMarkProxiesAsBadComplete().
   void MarkProxiesAsBad(const std::vector<net::ProxyServer>& bad_proxies,

content/common/throttling_url_loader.cc

@@ -614,6 +614,31 @@ void ThrottlingURLLoader::OnComplete(
   DCHECK_EQ(DEFERRED_NONE, deferred_stage_);
   DCHECK(!loader_completed_);
 
+  // Only dispatch WillOnCompleteWithError() if status is not OK.
+  if (!throttles_.empty() && status.error_code != net::OK) {
+    pending_restart_flags_ = 0;
+    has_pending_restart_ = false;
+    bool deferred = false;
+    for (auto& entry : throttles_) {
+      auto* throttle = entry.throttle.get();
+      bool throttle_deferred = false;
+      throttle->WillOnCompleteWithError(status, &throttle_deferred);
+      if (!HandleThrottleResult(throttle, throttle_deferred, &deferred))
+        return;
+    }
+
+    if (deferred) {
+      deferred_stage_ = DEFERRED_COMPLETE;
+      client_binding_.PauseIncomingMethodCallProcessing();
+      return;
+    }
+
+    if (has_pending_restart_) {
+      RestartWithFlagsNow();
+      return;
+    }
+  }
+
   // This is the last expected message. Pipe closure before this is an error
   // (see OnClientConnectionError). After this it is expected and should be
   // ignored. The owner of |this| is expected to destroy |this| when
@@ -680,6 +705,17 @@ void ThrottlingURLLoader::Resume() {
       // Note: |this| may be deleted here.
       break;
     }
+    case DEFERRED_COMPLETE: {
+      // TODO(eroman): For simplicity we require throttles that defer during
+      // WillOnCompleteWithError() to do a restart. We could support deferring
+      // and choosing not to restart if needed, however the current consumers
+      // don't need that.
+      CHECK(has_pending_restart_);
+
+      RestartWithFlagsNow();
+      // Note: |this| may be deleted here.
+      break;
+    }
     default:
       NOTREACHED();
       break;

content/common/throttling_url_loader.h

@@ -151,7 +151,8 @@ class CONTENT_EXPORT ThrottlingURLLoader
     DEFERRED_START,
     DEFERRED_REDIRECT,
     DEFERRED_BEFORE_RESPONSE,
-    DEFERRED_RESPONSE
+    DEFERRED_RESPONSE,
+    DEFERRED_COMPLETE
   };
   DeferredStage deferred_stage_ = DEFERRED_NONE;
   bool loader_completed_ = false;

content/public/common/url_loader_throttle.cc

@@ -54,6 +54,10 @@ void URLLoaderThrottle::BeforeWillProcessResponse(
     const network::ResourceResponseHead& response_head,
     bool* defer) {}
 
+void URLLoaderThrottle::WillOnCompleteWithError(
+    const network::URLLoaderCompletionStatus& status,
+    bool* defer) {}
+
 URLLoaderThrottle::URLLoaderThrottle() {}
 
 }  // namespace content

content/public/common/url_loader_throttle.h

@@ -156,6 +156,11 @@ class CONTENT_EXPORT URLLoaderThrottle {
       const network::ResourceResponseHead& response_head,
       bool* defer);
 
+  // Called if there is a non-OK net::Error in the completion status.
+  virtual void WillOnCompleteWithError(
+      const network::URLLoaderCompletionStatus& status,
+      bool* defer);
+
   void set_delegate(Delegate* delegate) { delegate_ = delegate; }
 
  protected:

services/network/public/cpp/network_ipc_param_traits.h

@@ -136,6 +136,7 @@ IPC_STRUCT_TRAITS_BEGIN(network::URLLoaderCompletionStatus)
   IPC_STRUCT_TRAITS_MEMBER(cors_error_status)
   IPC_STRUCT_TRAITS_MEMBER(ssl_info)
   IPC_STRUCT_TRAITS_MEMBER(should_report_corb_blocking)
+  IPC_STRUCT_TRAITS_MEMBER(proxy_server)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(network::ResourceResponseInfo)

services/network/public/cpp/url_loader_completion_status.cc

@@ -34,7 +34,8 @@ bool URLLoaderCompletionStatus::operator==(
          encoded_body_length == rhs.encoded_body_length &&
          decoded_body_length == rhs.decoded_body_length &&
          cors_error_status == rhs.cors_error_status &&
-         should_report_corb_blocking == rhs.should_report_corb_blocking;
+         should_report_corb_blocking == rhs.should_report_corb_blocking &&
+         proxy_server == rhs.proxy_server;
 }
 
 }  // namespace network

services/network/public/cpp/url_loader_completion_status.h

@@ -70,6 +70,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) URLLoaderCompletionStatus {
   // Set when response blocked by CORB needs to be reported to the DevTools
   // console.
   bool should_report_corb_blocking = false;
+
+  // The proxy server used for this request, if any.
+  net::ProxyServer proxy_server;
 };
 
 }  // namespace network

services/network/url_loader.cc

@@ -1109,6 +1109,7 @@ void URLLoader::NotifyCompleted(int error_code) {
     status.encoded_data_length = url_request_->GetTotalReceivedBytes();
     status.encoded_body_length = url_request_->GetRawBodyBytes();
     status.decoded_body_length = total_written_bytes_;
+    status.proxy_server = url_request_->proxy_server();
 
     if ((options_ & mojom::kURLLoadOptionSendSSLInfoForCertificateError) &&
         net::IsCertStatusError(url_request_->ssl_info().cert_status) &&