Network Service: Rework LoginHandlerDelegate

The original implementation had some thread-safety and lifetime issues.
This CL splits LoginHandlerDelegate into a UI and IO half to fix these,
and make it cleaner overall.

Longer-term, CreateLoginDelegate is one of the many places where we
should be able to move the ContentBrowserClient-level interface to
the UI thread (south of //content, the current code is similarly a
threading mess). https://crbug.com/908926 tracks this.

Bug: 901809
Change-Id: I097523042ca62dae46b2ed7c0bf3fc27bf7bab1d
Reviewed-on: https://chromium-review.googlesource.com/c/1357645Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Jun Cai <juncai@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613236}

content/browser/network_service_client.cc

@@ -161,18 +161,15 @@ class SSLClientAuthDelegate : public SSLClientAuthHandler::Delegate {
   std::unique_ptr<SSLClientAuthHandler> ssl_client_auth_handler_;
 };
 
-// This class is created on UI thread, and deleted by
-// BrowserThread::DeleteSoon() after the |callback_| runs. The |callback_|
-// needs to run on UI thread since it is called through the
-// NetworkServiceClient interface.
+// LoginHandlerDelegateIO handles HTTP auth on the IO thread.
 //
-// The |login_delegate_| needs to be created on IO thread, and deleted
-// on the same thread by posting a BrowserThread::DeleteSoon() task to IO
+// TODO(https://crbug.com/908926): This can be folded into LoginHandlerDelegate
+// with the thread-hops simplified once CreateLoginDelegate is moved to the UI
 // thread.
-class LoginHandlerDelegate {
+class LoginHandlerDelegateIO {
  public:
-  LoginHandlerDelegate(
-      network::mojom::AuthChallengeResponderPtr auth_challenge_responder,
+  LoginHandlerDelegateIO(
+      LoginAuthRequiredCallback callback,
       ResourceRequestInfo::WebContentsGetter web_contents_getter,
       scoped_refptr<net::AuthChallengeInfo> auth_info,
       bool is_request_for_main_frame,
@@ -182,73 +179,52 @@ class LoginHandlerDelegate {
       const GURL& url,
       scoped_refptr<net::HttpResponseHeaders> response_headers,
       bool first_auth_attempt)
-      : auth_challenge_responder_(std::move(auth_challenge_responder)),
+      : callback_(std::move(callback)),
         auth_info_(auth_info),
         request_id_(process_id, request_id),
+        routing_id_(routing_id),
         is_request_for_main_frame_(is_request_for_main_frame),
         url_(url),
         response_headers_(std::move(response_headers)),
         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)));
-
-    base::PostTaskWithTraits(
-        FROM_HERE, {BrowserThread::IO},
-        base::BindOnce(&LoginHandlerDelegate::DispatchInterceptorHookAndStart,
-                       base::Unretained(this), process_id, routing_id,
-                       request_id));
+        web_contents_getter_(web_contents_getter),
+        weak_factory_(this) {
+    // This object may be created on any thread, but it must be destroyed and
+    // otherwise accessed on the IO thread.
   }
 
-  void OnRequestCancelled() {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    if (!login_delegate_)
-      return;
-
-    // LoginDelegate::OnRequestCancelled can only be called from the IO thread.
-    base::PostTaskWithTraits(
-        FROM_HERE, {BrowserThread::IO},
-        base::BindOnce(&LoginHandlerDelegate::OnRequestCancelledOnIOThread,
-                       base::Unretained(this)));
-  }
-
-  void OnRequestCancelledOnIOThread() {
+  ~LoginHandlerDelegateIO() {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
-    login_delegate_->OnRequestCancelled();
+    if (login_delegate_)
+      login_delegate_->OnRequestCancelled();
   }
 
- private:
-  void DispatchInterceptorHookAndStart(uint32_t process_id,
-                                       uint32_t routing_id,
-                                       uint32_t request_id) {
+  void Start() {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
     DevToolsURLLoaderInterceptor::HandleAuthRequest(
-        process_id, routing_id, request_id, auth_info_,
-        base::BindOnce(&LoginHandlerDelegate::ContinueAfterInterceptor,
-                       base::Unretained(this)));
+        request_id_.child_id, routing_id_, request_id_.request_id, auth_info_,
+        base::BindOnce(&LoginHandlerDelegateIO::ContinueAfterInterceptor,
+                       weak_factory_.GetWeakPtr()));
   }
 
+ private:
   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
+    if (!use_fallback) {
       RunAuthCredentials(auth_credentials);
-  }
+      return;
+    }
 
-  void CreateLoginDelegate() {
-    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    // WeakPtr is not strictly necessary here due to OnRequestCancelled.
     login_delegate_ = GetContentClient()->browser()->CreateLoginDelegate(
         auth_info_.get(), web_contents_getter_, request_id_,
         is_request_for_main_frame_, url_, response_headers_,
         first_auth_attempt_,
-        base::BindOnce(&LoginHandlerDelegate::RunAuthCredentials,
-                       base::Unretained(this)));
-
+        base::BindOnce(&LoginHandlerDelegateIO::RunAuthCredentials,
+                       weak_factory_.GetWeakPtr()));
     if (!login_delegate_) {
       RunAuthCredentials(base::nullopt);
       return;
@@ -258,13 +234,78 @@ class LoginHandlerDelegate {
   void RunAuthCredentials(
       const base::Optional<net::AuthCredentials>& auth_credentials) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    std::move(callback_).Run(auth_credentials);
+    // There is no need to call OnRequestCancelled in the destructor.
+    login_delegate_ = nullptr;
+  }
+
+  LoginAuthRequiredCallback callback_;
+  scoped_refptr<net::AuthChallengeInfo> auth_info_;
+  const content::GlobalRequestID request_id_;
+  const uint32_t routing_id_;
+  bool is_request_for_main_frame_;
+  GURL url_;
+  const scoped_refptr<net::HttpResponseHeaders> response_headers_;
+  bool first_auth_attempt_;
+  ResourceRequestInfo::WebContentsGetter web_contents_getter_;
+  scoped_refptr<LoginDelegate> login_delegate_;
+  base::WeakPtrFactory<LoginHandlerDelegateIO> weak_factory_;
+};
+
+// LoginHanderDelegate manages LoginHandlerDelegateIO from the UI thread. It is
+// self-owning and deletes itself when the credentials are resolved or the
+// AuthChallengeResponder is cancelled.
+class LoginHandlerDelegate {
+ public:
+  LoginHandlerDelegate(
+      network::mojom::AuthChallengeResponderPtr auth_challenge_responder,
+      ResourceRequestInfo::WebContentsGetter web_contents_getter,
+      scoped_refptr<net::AuthChallengeInfo> auth_info,
+      bool is_request_for_main_frame,
+      uint32_t process_id,
+      uint32_t routing_id,
+      uint32_t request_id,
+      const GURL& url,
+      scoped_refptr<net::HttpResponseHeaders> response_headers,
+      bool first_auth_attempt)
+      : auth_challenge_responder_(std::move(auth_challenge_responder)),
+        weak_factory_(this) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    auth_challenge_responder_.set_connection_error_handler(base::BindOnce(
+        &LoginHandlerDelegate::OnRequestCancelled, base::Unretained(this)));
+
+    login_handler_io_.reset(new LoginHandlerDelegateIO(
+        base::BindOnce(&LoginHandlerDelegate::OnAuthCredentialsIO,
+                       weak_factory_.GetWeakPtr()),
+        std::move(web_contents_getter), std::move(auth_info),
+        is_request_for_main_frame, process_id, routing_id, request_id, url,
+        std::move(response_headers), first_auth_attempt));
+
+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::IO},
+        base::BindOnce(&LoginHandlerDelegateIO::Start,
+                       base::Unretained(login_handler_io_.get())));
+  }
+
+ private:
+  void OnRequestCancelled() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    // This will destroy |login_handler_io_| on the IO thread and, if needed,
+    // inform the delegate.
+    delete this;
+  }
+
+  static void OnAuthCredentialsIO(
+      base::WeakPtr<LoginHandlerDelegate> handler,
+      const base::Optional<net::AuthCredentials>& auth_credentials) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     base::PostTaskWithTraits(
         FROM_HERE, {BrowserThread::UI},
-        base::BindOnce(&LoginHandlerDelegate::RunAuthCredentialsOnUI,
-                       base::Unretained(this), auth_credentials));
+        base::BindOnce(&LoginHandlerDelegate::OnAuthCredentials, handler,
+                       auth_credentials));
   }
 
-  void RunAuthCredentialsOnUI(
+  void OnAuthCredentials(
       const base::Optional<net::AuthCredentials>& auth_credentials) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     auth_challenge_responder_->OnAuthCredentials(auth_credentials);
@@ -272,14 +313,9 @@ class LoginHandlerDelegate {
   }
 
   network::mojom::AuthChallengeResponderPtr auth_challenge_responder_;
-  scoped_refptr<net::AuthChallengeInfo> auth_info_;
-  const content::GlobalRequestID request_id_;
-  bool is_request_for_main_frame_;
-  GURL url_;
-  const scoped_refptr<net::HttpResponseHeaders> response_headers_;
-  bool first_auth_attempt_;
-  ResourceRequestInfo::WebContentsGetter web_contents_getter_;
-  scoped_refptr<LoginDelegate> login_delegate_;
+  std::unique_ptr<LoginHandlerDelegateIO, BrowserThread::DeleteOnIOThread>
+      login_handler_io_;
+  base::WeakPtrFactory<LoginHandlerDelegate> weak_factory_;
 };
 
 void HandleFileUploadRequest(