Un-refcount SSLClientAuthHandler.

That class doesn't really need to be ref-counted externally. There is one
catch: the caller is responsible for ensuring ClientCertStore is alive for the
duration of its async operation.

Resolve this by detaching it into a ref-counted Core internal to
SSLClientAuthHandler. Add a test to test this case.

Also make ContentBrowserClient's default client auth hook always select no
certificate so content_shell doesn't hang.

This relands the rest of https://codereview.chromium.org/596873002

BUG=439134

Review URL: https://codereview.chromium.org/795773002

Cr-Commit-Position: refs/heads/master@{#308142}

chrome/browser/chrome_content_browser_client.cc

@@ -1793,7 +1793,8 @@ void ChromeContentBrowserClient::SelectClientCertificate(
       render_process_id, render_frame_id);
   WebContents* tab = WebContents::FromRenderFrameHost(rfh);
   if (!tab) {
-    NOTREACHED();
+    // TODO(davidben): This makes the request hang, but returning no certificate
+    // also breaks. It should abort the request. See https://crbug.com/417092
     return;
   }
 

content/browser/loader/resource_loader.cc

@@ -97,8 +97,7 @@ ResourceLoader::ResourceLoader(scoped_ptr<net::URLRequest> request,
 ResourceLoader::~ResourceLoader() {
   if (login_delegate_.get())
     login_delegate_->OnRequestCancelled();
-  if (ssl_client_auth_handler_.get())
-    ssl_client_auth_handler_->OnRequestCancelled();
+  ssl_client_auth_handler_.reset();
 
   // Run ResourceHandler destructor before we tear-down the rest of our state
   // as the ResourceHandler may want to inspect the URLRequest and other state.
@@ -307,14 +306,12 @@ void ResourceLoader::OnCertificateRequested(
     return;
   }
 
-  DCHECK(!ssl_client_auth_handler_.get())
+  DCHECK(!ssl_client_auth_handler_)
       << "OnCertificateRequested called with ssl_client_auth_handler pending";
-  ssl_client_auth_handler_ = new SSLClientAuthHandler(
-      GetRequestInfo()->GetContext()->CreateClientCertStore(),
-      request_.get(),
-      cert_info,
-      base::Bind(&ResourceLoader::ContinueWithCertificate,
-                 weak_ptr_factory_.GetWeakPtr()));
+  ssl_client_auth_handler_.reset(new SSLClientAuthHandler(
+      GetRequestInfo()->GetContext()->CreateClientCertStore(), request_.get(),
+      cert_info, base::Bind(&ResourceLoader::ContinueWithCertificate,
+                            weak_ptr_factory_.GetWeakPtr())));
   ssl_client_auth_handler_->SelectCertificate();
 }
 
@@ -587,10 +584,7 @@ void ResourceLoader::CancelRequestInternal(int error, bool from_renderer) {
     login_delegate_->OnRequestCancelled();
     login_delegate_ = NULL;
   }
-  if (ssl_client_auth_handler_.get()) {
-    ssl_client_auth_handler_->OnRequestCancelled();
-    ssl_client_auth_handler_ = NULL;
-  }
+  ssl_client_auth_handler_.reset();
 
   request_->CancelWithError(error);
 
@@ -857,7 +851,7 @@ void ResourceLoader::RecordHistograms() {
 }
 
 void ResourceLoader::ContinueWithCertificate(net::X509Certificate* cert) {
-  ssl_client_auth_handler_ = NULL;
+  ssl_client_auth_handler_.reset();
   request_->ContinueWithCertificate(cert);
 }
 

content/browser/loader/resource_loader.h

@@ -57,6 +57,7 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
  private:
   FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreLookup);
   FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreNull);
+  FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreAsyncCancel);
 
   // net::URLRequest::Delegate implementation:
   void OnReceivedRedirect(net::URLRequest* request,
@@ -133,7 +134,7 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
   ResourceLoaderDelegate* delegate_;
 
   scoped_refptr<ResourceDispatcherHostLoginDelegate> login_delegate_;
-  scoped_refptr<SSLClientAuthHandler> ssl_client_auth_handler_;
+  scoped_ptr<SSLClientAuthHandler> ssl_client_auth_handler_;
 
   uint64 last_upload_position_;
   bool waiting_for_upload_progress_ack_;

content/browser/loader/resource_loader_unittest.cc

@@ -41,11 +41,13 @@ namespace {
 class ClientCertStoreStub : public net::ClientCertStore {
  public:
   ClientCertStoreStub(const net::CertificateList& certs)
-      : response_(certs),
-        request_count_(0) {}
+      : response_(certs), async_(false), request_count_(0) {}
 
   ~ClientCertStoreStub() override {}
 
+  // Configures whether the certificates are returned asynchronously or not.
+  void set_async(bool async) { async_ = async; }
+
   // Returns |cert_authorities| field of the certificate request passed in the
   // most recent call to GetClientCerts().
   // TODO(ppi): Make the stub independent from the internal representation of
@@ -68,11 +70,16 @@ class ClientCertStoreStub : public net::ClientCertStore {
     ++request_count_;
     requested_authorities_ = cert_request_info.cert_authorities;
     *selected_certs = response_;
-    callback.Run();
+    if (async_) {
+      base::MessageLoop::current()->PostTask(FROM_HERE, callback);
+    } else {
+      callback.Run();
+    }
   }
 
  private:
   const net::CertificateList response_;
+  bool async_;
   int request_count_;
   std::vector<std::string> requested_authorities_;
 };
@@ -437,6 +444,39 @@ TEST_F(ResourceLoaderTest, ClientCertStoreNull) {
   EXPECT_EQ(net::CertificateList(), test_client.passed_certs());
 }
 
+TEST_F(ResourceLoaderTest, ClientCertStoreAsyncCancel) {
+  // Set up the test client cert store.
+  scoped_ptr<ClientCertStoreStub> test_store(
+      new ClientCertStoreStub(net::CertificateList()));
+  test_store->set_async(true);
+  EXPECT_EQ(0, test_store->request_count());
+
+  // Ownership of the |test_store| is about to be turned over to ResourceLoader.
+  // We need to keep raw pointer copies to access these objects later.
+  ClientCertStoreStub* raw_ptr_to_store = test_store.get();
+  resource_context_.SetClientCertStore(test_store.Pass());
+
+  // Prepare a dummy certificate request.
+  scoped_refptr<net::SSLCertRequestInfo> cert_request_info(
+      new net::SSLCertRequestInfo());
+  std::vector<std::string> dummy_authority(1, "dummy");
+  cert_request_info->cert_authorities = dummy_authority;
+
+  // Everything is set up. Trigger the resource loader certificate request
+  // event.
+  loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get());
+
+  // Check if the test store was queried against correct |cert_authorities|.
+  EXPECT_EQ(1, raw_ptr_to_store->request_count());
+  EXPECT_EQ(dummy_authority, raw_ptr_to_store->requested_authorities());
+
+  // Cancel the request before the store calls the callback.
+  loader_.reset();
+
+  // Pump the event loop. There shouldn't be a crash when the callback is run.
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
   raw_ptr_resource_handler_->set_defer_request_on_will_start(true);
 

content/browser/ssl/ssl_client_auth_handler.cc

@@ -5,116 +5,144 @@
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 
 #include "base/bind.h"
-#include "base/callback_helpers.h"
+#include "base/logging.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/resource_request_info.h"
+#include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/url_request/url_request.h"
 
 namespace content {
 
+namespace {
+
+void CertificateSelectedOnUIThread(
+    const SSLClientAuthHandler::CertificateCallback& io_thread_callback,
+    net::X509Certificate* cert) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(io_thread_callback, make_scoped_refptr(cert)));
+}
+
+void SelectCertificateOnUIThread(
+    int render_process_host_id,
+    int render_frame_host_id,
+    net::SSLCertRequestInfo* cert_request_info,
+    const SSLClientAuthHandler::CertificateCallback& io_thread_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  GetContentClient()->browser()->SelectClientCertificate(
+      render_process_host_id, render_frame_host_id, cert_request_info,
+      base::Bind(&CertificateSelectedOnUIThread, io_thread_callback));
+}
+
+}  // namespace
+
+// A reference-counted core to allow the ClientCertStore and SSLCertRequestInfo
+// to outlive SSLClientAuthHandler if needbe.
+class SSLClientAuthHandler::Core : public base::RefCountedThreadSafe<Core> {
+ public:
+  Core(const base::WeakPtr<SSLClientAuthHandler>& handler,
+       scoped_ptr<net::ClientCertStore> client_cert_store,
+       net::SSLCertRequestInfo* cert_request_info)
+      : handler_(handler),
+        client_cert_store_(client_cert_store.Pass()),
+        cert_request_info_(cert_request_info) {}
+
+  bool has_client_cert_store() const { return client_cert_store_; }
+
+  void GetClientCerts() {
+    if (client_cert_store_) {
+      client_cert_store_->GetClientCerts(
+          *cert_request_info_, &cert_request_info_->client_certs,
+          base::Bind(&SSLClientAuthHandler::Core::DidGetClientCerts, this));
+    } else {
+      DidGetClientCerts();
+    }
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<Core>;
+
+  ~Core() {}
+
+  // Called when |client_cert_store_| is done retrieving the cert list.
+  void DidGetClientCerts() {
+    if (handler_)
+      handler_->DidGetClientCerts();
+  }
+
+  base::WeakPtr<SSLClientAuthHandler> handler_;
+  scoped_ptr<net::ClientCertStore> client_cert_store_;
+  scoped_refptr<net::SSLCertRequestInfo> cert_request_info_;
+};
+
 SSLClientAuthHandler::SSLClientAuthHandler(
     scoped_ptr<net::ClientCertStore> client_cert_store,
     net::URLRequest* request,
     net::SSLCertRequestInfo* cert_request_info,
     const SSLClientAuthHandler::CertificateCallback& callback)
-    : request_(request),
+    : core_(nullptr),
+      request_(request),
       cert_request_info_(cert_request_info),
-      client_cert_store_(client_cert_store.Pass()),
-      callback_(callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-}
+      callback_(callback),
+      weak_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-SSLClientAuthHandler::~SSLClientAuthHandler() {
-  // If we were simply dropped, then act as if we selected no certificate.
-  DoCertificateSelected(NULL);
+  core_ = new Core(weak_factory_.GetWeakPtr(), client_cert_store.Pass(),
+                   cert_request_info_.get());
 }
 
-void SSLClientAuthHandler::OnRequestCancelled() {
-  request_ = NULL;
-  callback_.Reset();
+SSLClientAuthHandler::~SSLClientAuthHandler() {
 }
 
 void SSLClientAuthHandler::SelectCertificate() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK(request_);
-
-  if (client_cert_store_) {
-    client_cert_store_->GetClientCerts(
-        *cert_request_info_,
-        &cert_request_info_->client_certs,
-        base::Bind(&SSLClientAuthHandler::DidGetClientCerts, this));
-  } else {
-    DidGetClientCerts();
-  }
-}
-
-void SSLClientAuthHandler::CertificateSelected(net::X509Certificate* cert) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  DVLOG(1) << this << " CertificateSelected " << cert;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(
-          &SSLClientAuthHandler::DoCertificateSelected, this,
-          make_scoped_refptr(cert)));
+  // |core_| will call DidGetClientCerts when done.
+  core_->GetClientCerts();
 }
 
 void SSLClientAuthHandler::DidGetClientCerts() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // Request may have cancelled while we were getting client certs.
-  if (!request_)
-    return;
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Note that if |client_cert_store_| is NULL, we intentionally fall through to
   // DoCertificateSelected. This is for platforms where the client cert matching
-  // is not performed by Chrome, the platform can handle the cert matching
-  // before showing the dialog.
-  if (client_cert_store_ && cert_request_info_->client_certs.empty()) {
+  // is not performed by Chrome. Those platforms handle the cert matching before
+  // showing the dialog.
+  if (core_->has_client_cert_store() &&
+      cert_request_info_->client_certs.empty()) {
     // No need to query the user if there are no certs to choose from.
-    DoCertificateSelected(NULL);
+    CertificateSelected(NULL);
     return;
   }
 
   int render_process_host_id;
   int render_frame_host_id;
   if (!ResourceRequestInfo::ForRequest(request_)->GetAssociatedRenderFrame(
-          &render_process_host_id,
-          &render_frame_host_id))
+          &render_process_host_id, &render_frame_host_id)) {
     NOTREACHED();
+    CertificateSelected(NULL);
+    return;
+  }
 
-  // If the RVH does not exist by the time this task gets run, then the task
-  // will be dropped and the scoped_refptr to SSLClientAuthHandler will go
-  // away, so we do not leak anything. The destructor takes care of ensuring
-  // the net::URLRequest always gets a response.
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(
-          &SSLClientAuthHandler::DoSelectCertificate, this,
-          render_process_host_id, render_frame_host_id));
+      base::Bind(&SelectCertificateOnUIThread, render_process_host_id,
+                 render_frame_host_id, cert_request_info_,
+                 base::Bind(&SSLClientAuthHandler::CertificateSelected,
+                            weak_factory_.GetWeakPtr())));
 }
 
-void SSLClientAuthHandler::DoCertificateSelected(net::X509Certificate* cert) {
+void SSLClientAuthHandler::CertificateSelected(net::X509Certificate* cert) {
   DVLOG(1) << this << " DoCertificateSelected " << cert;
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // request_ could have been NULLed if the request was cancelled while the
-  // user was choosing a cert, or because we have already responded to the
-  // certificate.
-  if (request_) {
-    request_ = NULL;
-    DCHECK(!callback_.is_null());
-    base::ResetAndReturn(&callback_).Run(cert);
-  }
-}
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-void SSLClientAuthHandler::DoSelectCertificate(
-    int render_process_host_id, int render_frame_host_id) {
-  GetContentClient()->browser()->SelectClientCertificate(
-      render_process_host_id,
-      render_frame_host_id,
-      cert_request_info_.get(),
-      base::Bind(&SSLClientAuthHandler::CertificateSelected, this));
+  callback_.Run(cert);
+  // |this| may be deleted at this point.
 }
 
 }  // namespace content

content/browser/ssl/ssl_client_auth_handler.h

@@ -6,30 +6,25 @@
 #define CONTENT_BROWSER_SSL_SSL_CLIENT_AUTH_HANDLER_H_
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
-#include "base/sequenced_task_runner_helpers.h"
-#include "content/common/content_export.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/ssl/ssl_cert_request_info.h"
 
 namespace net {
 class ClientCertStore;
-class HttpNetworkSession;
 class URLRequest;
 class X509Certificate;
 }  // namespace net
 
 namespace content {
 
-class ResourceContext;
-
 // This class handles the approval and selection of a certificate for SSL client
-// authentication by the user.
-// It is self-owned and deletes itself when the UI reports the user selection or
-// when the net::URLRequest is cancelled.
-class CONTENT_EXPORT SSLClientAuthHandler
-    : public base::RefCountedThreadSafe<
-          SSLClientAuthHandler, BrowserThread::DeleteOnIOThread> {
+// authentication by the user. Should only be used on the IO thread. If the
+// SSLClientAuthHandler is destroyed before the certificate is selected, the
+// selection is canceled and the callback never called.
+class SSLClientAuthHandler {
  public:
   using CertificateCallback = base::Callback<void(net::X509Certificate*)>;
 
@@ -37,39 +32,24 @@ class CONTENT_EXPORT SSLClientAuthHandler
                        net::URLRequest* request,
                        net::SSLCertRequestInfo* cert_request_info,
                        const CertificateCallback& callback);
+  ~SSLClientAuthHandler();
 
   // Selects a certificate and resumes the URL request with that certificate.
-  // Should only be called on the IO thread.
   void SelectCertificate();
 
-  // Invoked when the request associated with this handler is cancelled.
-  // Should only be called on the IO thread.
-  void OnRequestCancelled();
-
-  // Calls DoCertificateSelected on the I/O thread.
-  // Called on the UI thread after the user has made a selection (which may
-  // be long after DoSelectCertificate returns, if the UI is modeless/async.)
-  void CertificateSelected(net::X509Certificate* cert);
-
- protected:
-  virtual ~SSLClientAuthHandler();
-
  private:
-  friend class base::RefCountedThreadSafe<
-      SSLClientAuthHandler, BrowserThread::DeleteOnIOThread>;
-  friend class BrowserThread;
-  friend class base::DeleteHelper<SSLClientAuthHandler>;
+  class Core;
 
-  // Called when ClientCertStore is done retrieving the cert list.
+  // Called when |core_| is done retrieving the cert list.
   void DidGetClientCerts();
 
-  // Notifies that the user has selected a cert.
-  // Called on the IO thread.
-  void DoCertificateSelected(net::X509Certificate* cert);
+  // Called when the user has selected a cert.
+  void CertificateSelected(net::X509Certificate* cert);
 
-  // Selects a client certificate on the UI thread.
-  void DoSelectCertificate(int render_process_host_id,
-                           int render_frame_host_id);
+  // A reference-counted core so the ClientCertStore may outlive
+  // SSLClientAuthHandler if the handler is destroyed while an operation on the
+  // ClientCertStore is in progress.
+  scoped_refptr<Core> core_;
 
   // The net::URLRequest that triggered this client auth.
   net::URLRequest* request_;
@@ -77,11 +57,11 @@ class CONTENT_EXPORT SSLClientAuthHandler
   // The certs to choose from.
   scoped_refptr<net::SSLCertRequestInfo> cert_request_info_;
 
-  scoped_ptr<net::ClientCertStore> client_cert_store_;
-
   // The callback to call when the certificate is selected.
   CertificateCallback callback_;
 
+  base::WeakPtrFactory<SSLClientAuthHandler> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SSLClientAuthHandler);
 };
 

content/public/browser/content_browser_client.cc

@@ -176,6 +176,14 @@ QuotaPermissionContext* ContentBrowserClient::CreateQuotaPermissionContext() {
   return NULL;
 }
 
+void ContentBrowserClient::SelectClientCertificate(
+    int render_process_id,
+    int render_frame_id,
+    net::SSLCertRequestInfo* cert_request_info,
+    const base::Callback<void(net::X509Certificate*)>& callback) {
+  callback.Run(NULL);
+}
+
 net::URLRequestContext* ContentBrowserClient::OverrideRequestContextForURL(
     const GURL& url, ResourceContext* context) {
   return NULL;

content/public/browser/content_browser_client.h

@@ -400,7 +400,7 @@ class CONTENT_EXPORT ContentBrowserClient {
       int render_process_id,
       int render_frame_id,
       net::SSLCertRequestInfo* cert_request_info,
-      const base::Callback<void(net::X509Certificate*)>& callback) {}
+      const base::Callback<void(net::X509Certificate*)>& callback);
 
   // Adds a new installable certificate or private key.
   // Typically used to install an X.509 user certificate.