Network Service: Fix some LoginPromptBrowserTest related to cancelling auth request

This CL fixes some LoginPromptBrowserTest related to cancelling auth
request, in this case, LoginHandler::OnRequestCancelled() needs to be
called.

Bug: 783990
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id87ce38aec5191e40b3b282b4cde0304ea769d92
Reviewed-on: https://chromium-review.googlesource.com/1008846Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553369}

content/browser/network_service_client.cc

@@ -157,7 +157,7 @@ class SSLClientAuthDelegate : public SSLClientAuthHandler::Delegate {
 class LoginHandlerDelegate {
  public:
   LoginHandlerDelegate(
-      network::mojom::NetworkServiceClient::OnAuthRequiredCallback callback,
+      network::mojom::AuthChallengeResponderPtr auth_challenge_responder,
       ResourceRequestInfo::WebContentsGetter web_contents_getter,
       scoped_refptr<net::AuthChallengeInfo> auth_info,
       bool is_main_frame,
@@ -166,12 +166,16 @@ class LoginHandlerDelegate {
       uint32_t request_id,
       const GURL& url,
       bool first_auth_attempt)
-      : callback_(std::move(callback)),
+      : auth_challenge_responder_(std::move(auth_challenge_responder)),
         auth_info_(auth_info),
         is_main_frame_(is_main_frame),
         url_(url),
         first_auth_attempt_(first_auth_attempt),
         web_contents_getter_(web_contents_getter) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    auth_challenge_responder_.set_connection_error_handler(base::BindOnce(
+        &LoginHandlerDelegate::OnRequestCancelled, base::Unretained(this)));
+
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::BindOnce(&LoginHandlerDelegate::DispatchInterceptorHookAndStart,
@@ -179,10 +183,28 @@ class LoginHandlerDelegate {
                        request_id));
   }
 
+  void OnRequestCancelled() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    if (!login_delegate_)
+      return;
+
+    // LoginDelegate::OnRequestCancelled can only be called from the IO thread.
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&LoginHandlerDelegate::OnRequestCancelledOnIOThread,
+                       base::Unretained(this)));
+  }
+
+  void OnRequestCancelledOnIOThread() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    login_delegate_->OnRequestCancelled();
+  }
+
  private:
   void DispatchInterceptorHookAndStart(uint32_t process_id,
                                        uint32_t routing_id,
                                        uint32_t request_id) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     DevToolsURLLoaderInterceptor::HandleAuthRequest(
         process_id, routing_id, request_id, auth_info_,
         base::BindOnce(&LoginHandlerDelegate::ContinueAfterInterceptor,
@@ -192,35 +214,45 @@ class LoginHandlerDelegate {
   void ContinueAfterInterceptor(
       bool use_fallback,
       const base::Optional<net::AuthCredentials>& auth_credentials) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     DCHECK(!(use_fallback && auth_credentials.has_value()));
     if (use_fallback)
       CreateLoginDelegate();
     else
-      RunAuthRequiredCallback(auth_credentials);
+      RunAuthCredentials(auth_credentials);
   }
 
   void CreateLoginDelegate() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     login_delegate_ = GetContentClient()->browser()->CreateLoginDelegate(
         auth_info_.get(), web_contents_getter_, is_main_frame_, url_,
         first_auth_attempt_,
-        base::Bind(&LoginHandlerDelegate::RunAuthRequiredCallback,
+        base::Bind(&LoginHandlerDelegate::RunAuthCredentials,
                    base::Unretained(this)));
 
     if (!login_delegate_) {
-      RunAuthRequiredCallback(base::nullopt);
+      RunAuthCredentials(base::nullopt);
       return;
     }
   }
 
-  void RunAuthRequiredCallback(
+  void RunAuthCredentials(
       const base::Optional<net::AuthCredentials>& auth_credentials) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(std::move(callback_), auth_credentials));
+        base::BindOnce(&LoginHandlerDelegate::RunAuthCredentialsOnUI,
+                       base::Unretained(this), auth_credentials));
+  }
+
+  void RunAuthCredentialsOnUI(
+      const base::Optional<net::AuthCredentials>& auth_credentials) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    auth_challenge_responder_->OnAuthCredentials(auth_credentials);
     delete this;
   }
 
-  network::mojom::NetworkServiceClient::OnAuthRequiredCallback callback_;
+  network::mojom::AuthChallengeResponderPtr auth_challenge_responder_;
   scoped_refptr<net::AuthChallengeInfo> auth_info_;
   bool is_main_frame_;
   GURL url_;
@@ -244,14 +276,15 @@ void NetworkServiceClient::OnAuthRequired(
     const GURL& url,
     bool first_auth_attempt,
     const scoped_refptr<net::AuthChallengeInfo>& auth_info,
-    network::mojom::NetworkServiceClient::OnAuthRequiredCallback callback) {
+    network::mojom::AuthChallengeResponderPtr auth_challenge_responder) {
   base::Callback<WebContents*(void)> web_contents_getter =
       process_id ? base::Bind(WebContentsImpl::FromRenderFrameHostID,
                               process_id, routing_id)
                  : base::Bind(WebContents::FromFrameTreeNodeId, routing_id);
 
   if (!web_contents_getter.Run()) {
-    std::move(callback).Run(net::AuthCredentials());
+    std::move(auth_challenge_responder)
+        ->OnAuthCredentials(net::AuthCredentials());
     return;
   }
 
@@ -260,10 +293,10 @@ void NetworkServiceClient::OnAuthRequired(
           ? RenderFrameHost::FromID(process_id, routing_id)
           : FrameTreeNode::GloballyFindByID(routing_id)->current_frame_host();
   bool is_main_frame = !rfh->GetParent();
-  new LoginHandlerDelegate(std::move(callback), std::move(web_contents_getter),
-                           auth_info, is_main_frame, process_id, routing_id,
-                           request_id, url,
-                           first_auth_attempt);  // deletes self
+  new LoginHandlerDelegate(
+      std::move(auth_challenge_responder), std::move(web_contents_getter),
+      auth_info, is_main_frame, process_id, routing_id, request_id, url,
+      first_auth_attempt);  // deletes self
 }
 
 void NetworkServiceClient::OnCertificateRequested(

content/browser/network_service_client.h

@@ -19,15 +19,14 @@ class NetworkServiceClient : public network::mojom::NetworkServiceClient {
   ~NetworkServiceClient() override;
 
   // network::mojom::NetworkServiceClient implementation:
-  void OnAuthRequired(
-      uint32_t process_id,
-      uint32_t routing_id,
-      uint32_t request_id,
-      const GURL& url,
-      bool first_auth_attempt,
-      const scoped_refptr<net::AuthChallengeInfo>& auth_info,
-      network::mojom::NetworkServiceClient::OnAuthRequiredCallback callback)
-      override;
+  void OnAuthRequired(uint32_t process_id,
+                      uint32_t routing_id,
+                      uint32_t request_id,
+                      const GURL& url,
+                      bool first_auth_attempt,
+                      const scoped_refptr<net::AuthChallengeInfo>& auth_info,
+                      network::mojom::AuthChallengeResponderPtr
+                          auth_challenge_responder) override;
   void OnCertificateRequested(
       uint32_t process_id,
       uint32_t routing_id,

services/network/public/mojom/network_service.mojom

@@ -288,18 +288,25 @@ interface SSLPrivateKey {
        array<uint8> input) => (int32 net_error, array<uint8> signature);
 };
 
+// The |credentials| output parameter is given to URLRequest::SetAuth()
+// through this mojo interface. It is not set when URLRequest::CancelAuth()
+// needs to be called.
+interface AuthChallengeResponder {
+   OnAuthCredentials(AuthCredentials? credentials);
+};
+
 // Network service interface to the browser.
 interface NetworkServiceClient {
   // Called when we receive an authentication failure.
-  // The |credentials| output parameter is given to URLRequest::SetAuth()
-  // through this mojo interface. It is not set when URLRequest::CancelAuth()
-  // needs to be called.
+  // The |auth_challenge_responder| will respond to auth challenge with
+  // credentials.
   OnAuthRequired(uint32 process_id,
                  uint32 routing_id,
                  uint32 request_id,
                  url.mojom.Url url,
                  bool first_auth_attempt,
-                 AuthChallengeInfo auth_info) => (AuthCredentials? credentials);
+                 AuthChallengeInfo auth_info,
+                 AuthChallengeResponder auth_challenge_responder);
   // Called when an SSL certificate requested message is received for client
   // authentication.
   // The |algorithm_preferences| parameter corresponds to the return value

services/network/url_loader.cc

@@ -284,6 +284,7 @@ URLLoader::URLLoader(
       request_id_(request_id),
       keepalive_(request.keepalive),
       binding_(this, std::move(url_loader_request)),
+      auth_challenge_responder_binding_(this),
       url_loader_client_(std::move(url_loader_client)),
       writable_handle_watcher_(FROM_HERE,
                                mojo::SimpleWatcher::ArmingPolicy::MANUAL,
@@ -468,15 +469,19 @@ void URLLoader::OnReceivedRedirect(net::URLRequest* url_request,
 void URLLoader::OnAuthRequired(net::URLRequest* unused,
                                net::AuthChallengeInfo* auth_info) {
   if (!network_service_client_) {
-    OnAuthRequiredResponse(base::nullopt);
+    OnAuthCredentials(base::nullopt);
     return;
   }
 
+  network::mojom::AuthChallengeResponderPtr auth_challenge_responder;
+  auto request = mojo::MakeRequest(&auth_challenge_responder);
+  DCHECK(!auth_challenge_responder_binding_.is_bound());
+  auth_challenge_responder_binding_.Bind(std::move(request));
+  auth_challenge_responder_binding_.set_connection_error_handler(
+      base::BindOnce(&URLLoader::DeleteSelf, base::Unretained(this)));
   network_service_client_->OnAuthRequired(
       process_id_, render_frame_id_, request_id_, url_request_->url(),
-      first_auth_attempt_, auth_info,
-      base::BindOnce(&URLLoader::OnAuthRequiredResponse,
-                     weak_ptr_factory_.GetWeakPtr()));
+      first_auth_attempt_, auth_info, std::move(auth_challenge_responder));
 
   first_auth_attempt_ = false;
 }
@@ -691,6 +696,20 @@ net::LoadState URLLoader::GetLoadStateForTesting() const {
   return url_request_->GetLoadState().state;
 }
 
+void URLLoader::OnAuthCredentials(
+    const base::Optional<net::AuthCredentials>& credentials) {
+  auth_challenge_responder_binding_.Close();
+
+  if (!url_request_)
+    return;
+
+  if (!credentials.has_value()) {
+    url_request_->CancelAuth();
+  } else {
+    url_request_->SetAuth(credentials.value());
+  }
+}
+
 void URLLoader::NotifyCompleted(int error_code) {
   // Ensure sending the final upload progress message here, since
   // OnResponseCompleted can be called without OnResponseStarted on cancellation
@@ -824,18 +843,6 @@ void URLLoader::OnCertificateRequestedResponse(
   }
 }
 
-void URLLoader::OnAuthRequiredResponse(
-    const base::Optional<net::AuthCredentials>& credentials) {
-  if (!url_request_)
-    return;
-
-  if (!credentials.has_value()) {
-    url_request_->CancelAuth();
-  } else {
-    url_request_->SetAuth(credentials.value());
-  }
-}
-
 bool URLLoader::HasDataPipe() const {
   return pending_write_ || response_body_stream_.is_valid();
 }

services/network/url_loader.h

@@ -41,7 +41,8 @@ struct ResourceResponse;
 
 class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
     : public mojom::URLLoader,
-      public net::URLRequest::Delegate {
+      public net::URLRequest::Delegate,
+      public mojom::AuthChallengeResponder {
  public:
   using DeleteCallback = base::OnceCallback<void(URLLoader* url_loader)>;
 
@@ -85,6 +86,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   void OnResponseStarted(net::URLRequest* url_request, int net_error) override;
   void OnReadCompleted(net::URLRequest* url_request, int bytes_read) override;
 
+  // mojom::AuthChallengeResponder:
+  void OnAuthCredentials(
+      const base::Optional<net::AuthCredentials>& credentials) override;
+
   net::LoadState GetLoadStateForTesting() const;
 
  private:
@@ -107,8 +112,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
       const std::vector<uint16_t>& algorithm_preferences,
       mojom::SSLPrivateKeyPtr ssl_private_key,
       bool cancel_certificate_selection);
-  void OnAuthRequiredResponse(
-      const base::Optional<net::AuthCredentials>& credentials);
   bool HasDataPipe() const;
   void RecordBodyReadFromNetBeforePausedIfNeeded();
   void ResumeStart();
@@ -126,6 +129,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   const bool keepalive_;
   std::unique_ptr<net::URLRequest> url_request_;
   mojo::Binding<mojom::URLLoader> binding_;
+  mojo::Binding<mojom::AuthChallengeResponder>
+      auth_challenge_responder_binding_;
   mojom::URLLoaderClientPtr url_loader_client_;
   int64_t total_written_bytes_ = 0;
 

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -141,10 +141,8 @@
 # BlockCrossdomainPromptForSubresources is flaky, see
 # https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8952857968271661120%2F%2B%2Fsteps%2Fnetwork_service_browser_tests__with_patch_%2F0%2Flogs%2FLoginPromptBrowserTest.BlockCrossdomainPromptForSubresources%2F0
 -LoginPromptBrowserTest.BlockCrossdomainPromptForSubresources
--LoginPromptBrowserTest.CancelLoginInterstitialOnRedirect
 -LoginPromptBrowserTest.NoLoginPromptForFavicon
 -LoginPromptBrowserTest.NoLoginPromptForXHRWithBadCredentials
--LoginPromptBrowserTest.TestCancelAuth_OnForward
 
 # These rely on proxy configuration and PAC execution being configured on the
 # legacy in-process URLRequestContext. They should be removed or updated to