Revert of "Remove SSLClientAuthHandler's RDH dependency." (https://codereview.chromium.org/596873002)

Reason for revert: Causes browser crash if URL request is cancelled during client cert loading. BUG=422765, 427844, 376003 TBR=davidben@chromium.org Review URL: https://codereview.chromium.org/755933002 Cr-Commit-Position: refs/heads/master@{#305707}
author: mattm <mattm@chromium.org> 2014-11-25 14:33:28 -0800
committer: Commit bot <commit-bot@chromium.org> 2014-11-25 22:33:59 +0000
commit: e18bf1d9005b08bba3637b16d744245e5467af0e (patch)
tree: 59666decfa16683e5227ba864edb4965aab4b6e8 /content
parent: bd576720e621951616af892bcf03ffaac49f1881 (diff)
download: chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.zip
chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.tar.gz
chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.tar.bz2
9 files changed, 131 insertions, 96 deletions
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index a44d153..8031983 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -765,6 +765,16 @@ ResourceDispatcherHostImpl::MaybeInterceptAsStream(net::URLRequest* request,
   return handler.Pass();
 }
 
+void ResourceDispatcherHostImpl::ClearSSLClientAuthHandlerForRequest(
+    net::URLRequest* request) {
+  ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request);
+  if (info) {
+    ResourceLoader* loader = GetLoader(info->GetGlobalRequestID());
+    if (loader)
+      loader->ClearSSLClientAuthHandler();
+  }
+}
+
 ResourceDispatcherHostLoginDelegate*
 ResourceDispatcherHostImpl::CreateLoginDelegate(
     ResourceLoader* loader,
diff --git a/content/browser/loader/resource_dispatcher_host_impl.h b/content/browser/loader/resource_dispatcher_host_impl.h
index a2631a2..86f8d60 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.h
+++ b/content/browser/loader/resource_dispatcher_host_impl.h
@@ -245,6 +245,8 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
       ResourceResponse* response,
       std::string* payload);
 
+  void ClearSSLClientAuthHandlerForRequest(net::URLRequest* request);
+
   ResourceScheduler* scheduler() { return scheduler_.get(); }
 
   // Called by a ResourceHandler when it's ready to start reading data and
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index 8c11b80..0aa9ceee 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -97,7 +97,8 @@ ResourceLoader::ResourceLoader(scoped_ptr<net::URLRequest> request,
 ResourceLoader::~ResourceLoader() {
   if (login_delegate_.get())
     login_delegate_->OnRequestCancelled();
-  ssl_client_auth_handler_.reset();
+  if (ssl_client_auth_handler_.get())
+    ssl_client_auth_handler_->OnRequestCancelled();
 
   // Run ResourceHandler destructor before we tear-down the rest of our state
   // as the ResourceHandler may want to inspect the URLRequest and other state.
@@ -210,6 +211,10 @@ void ResourceLoader::ClearLoginDelegate() {
   login_delegate_ = NULL;
 }
 
+void ResourceLoader::ClearSSLClientAuthHandler() {
+  ssl_client_auth_handler_ = NULL;
+}
+
 void ResourceLoader::OnUploadProgressACK() {
   waiting_for_upload_progress_ack_ = false;
 }
@@ -287,14 +292,12 @@ void ResourceLoader::OnCertificateRequested(
     return;
   }
 
-  DCHECK(!ssl_client_auth_handler_)
+  DCHECK(!ssl_client_auth_handler_.get())
       << "OnCertificateRequested called with ssl_client_auth_handler pending";
-  ssl_client_auth_handler_.reset(new SSLClientAuthHandler(
+  ssl_client_auth_handler_ = new SSLClientAuthHandler(
       GetRequestInfo()->GetContext()->CreateClientCertStore(),
       request_.get(),
-      cert_info,
-      base::Bind(&ResourceLoader::ContinueWithCertificate,
-                 weak_ptr_factory_.GetWeakPtr())));
+      cert_info);
   ssl_client_auth_handler_->SelectCertificate();
 }
 
@@ -526,7 +529,10 @@ void ResourceLoader::CancelRequestInternal(int error, bool from_renderer) {
     login_delegate_->OnRequestCancelled();
     login_delegate_ = NULL;
   }
-  ssl_client_auth_handler_.reset();
+  if (ssl_client_auth_handler_.get()) {
+    ssl_client_auth_handler_->OnRequestCancelled();
+    ssl_client_auth_handler_ = NULL;
+  }
 
   request_->CancelWithError(error);
 
@@ -764,9 +770,4 @@ void ResourceLoader::RecordHistograms() {
   }
 }
 
-void ResourceLoader::ContinueWithCertificate(net::X509Certificate* cert) {
-  ssl_client_auth_handler_.reset();
-  request_->ContinueWithCertificate(cert);
-}
-
 }  // namespace content
diff --git a/content/browser/loader/resource_loader.h b/content/browser/loader/resource_loader.h
index d740ec0..b5c8b8a 100644
--- a/content/browser/loader/resource_loader.h
+++ b/content/browser/loader/resource_loader.h
@@ -15,10 +15,6 @@
 #include "content/public/common/signed_certificate_timestamp_id_and_status.h"
 #include "net/url_request/url_request.h"
 
-namespace net {
-class X509Certificate;
-}
-
 namespace content {
 class ResourceDispatcherHostLoginDelegate;
 class ResourceLoaderDelegate;
@@ -50,6 +46,7 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
   ResourceRequestInfoImpl* GetRequestInfo();
 
   void ClearLoginDelegate();
+  void ClearSSLClientAuthHandler();
 
   // IPC message handlers:
   void OnUploadProgressACK();
@@ -102,7 +99,6 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
   void ResponseCompleted();
   void CallDidFinishLoading();
   void RecordHistograms();
-  void ContinueWithCertificate(net::X509Certificate* cert);
 
   bool is_deferred() const { return deferred_stage_ != DEFERRED_NONE; }
 
@@ -133,7 +129,7 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
   ResourceLoaderDelegate* delegate_;
 
   scoped_refptr<ResourceDispatcherHostLoginDelegate> login_delegate_;
-  scoped_ptr<SSLClientAuthHandler> ssl_client_auth_handler_;
+  scoped_refptr<SSLClientAuthHandler> ssl_client_auth_handler_;
 
   uint64 last_upload_position_;
   bool waiting_for_upload_progress_ack_;
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 2af4ffc..8910dec 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -231,6 +231,7 @@ class SelectCertificateBrowserClient : public TestContentBrowserClient {
   void SelectClientCertificate(
       int render_process_id,
       int render_view_id,
+      const net::HttpNetworkSession* network_session,
       net::SSLCertRequestInfo* cert_request_info,
       const base::Callback<void(net::X509Certificate*)>& callback) override {
     ++call_count_;
diff --git a/content/browser/ssl/ssl_client_auth_handler.cc b/content/browser/ssl/ssl_client_auth_handler.cc
index 7eb680f..b063e619 100644
--- a/content/browser/ssl/ssl_client_auth_handler.cc
+++ b/content/browser/ssl/ssl_client_auth_handler.cc
@@ -5,84 +5,77 @@
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 
 #include "base/bind.h"
-#include "base/logging.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "net/cert/x509_certificate.h"
+#include "net/http/http_transaction_factory.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
 
 namespace content {
 
-namespace {
-
-typedef base::Callback<void(net::X509Certificate*)> CertificateCallback;
-
-void CertificateSelectedOnUIThread(
-    const CertificateCallback& io_thread_callback,
-    net::X509Certificate* cert) {
-  DCHECK(BrowserThread::CurrentlyOn(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 CertificateCallback& io_thread_callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  GetContentClient()->browser()->SelectClientCertificate(
-      render_process_host_id, render_frame_host_id, cert_request_info,
-      base::Bind(&CertificateSelectedOnUIThread, io_thread_callback));
-}
-
-}  // namespace
-
 SSLClientAuthHandler::SSLClientAuthHandler(
     scoped_ptr<net::ClientCertStore> client_cert_store,
     net::URLRequest* request,
-    net::SSLCertRequestInfo* cert_request_info,
-    const SSLClientAuthHandler::CertificateCallback& callback)
+    net::SSLCertRequestInfo* cert_request_info)
     : request_(request),
+      http_network_session_(
+          request_->context()->http_transaction_factory()->GetSession()),
       cert_request_info_(cert_request_info),
-      client_cert_store_(client_cert_store.Pass()),
-      callback_(callback),
-      weak_factory_(this) {
+      client_cert_store_(client_cert_store.Pass()) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }
 
 SSLClientAuthHandler::~SSLClientAuthHandler() {
+  // If we were simply dropped, then act as if we selected no certificate.
+  DoCertificateSelected(NULL);
+}
+
+void SSLClientAuthHandler::OnRequestCancelled() {
+  request_ = NULL;
 }
 
 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,
-                   weak_factory_.GetWeakPtr()));
+        base::Bind(&SSLClientAuthHandler::DidGetClientCerts, this));
   } else {
     DidGetClientCerts();
   }
 }
 
+void SSLClientAuthHandler::CertificateSelected(net::X509Certificate* cert) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DVLOG(1) << this << " CertificateSelected " << cert;
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &SSLClientAuthHandler::DoCertificateSelected, this,
+          make_scoped_refptr(cert)));
+}
+
 void SSLClientAuthHandler::DidGetClientCerts() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Request may have cancelled while we were getting client certs.
+  if (!request_)
+    return;
 
   // 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. Those platforms handle the cert matching before
-  // showing the dialog.
+  // 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()) {
     // No need to query the user if there are no certs to choose from.
-    CertificateSelected(NULL);
+    DoCertificateSelected(NULL);
     return;
   }
 
@@ -90,27 +83,43 @@ void SSLClientAuthHandler::DidGetClientCerts() {
   int render_frame_host_id;
   if (!ResourceRequestInfo::ForRequest(request_)->GetAssociatedRenderFrame(
           &render_process_host_id,
-          &render_frame_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(&SelectCertificateOnUIThread,
-                 render_process_host_id, render_frame_host_id,
-                 cert_request_info_,
-                 base::Bind(&SSLClientAuthHandler::CertificateSelected,
-                            weak_factory_.GetWeakPtr())));
+      base::Bind(
+          &SSLClientAuthHandler::DoSelectCertificate, this,
+          render_process_host_id, render_frame_host_id));
 }
 
-void SSLClientAuthHandler::CertificateSelected(net::X509Certificate* cert) {
+void SSLClientAuthHandler::DoCertificateSelected(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_->ContinueWithCertificate(cert);
+
+    ResourceDispatcherHostImpl::Get()->
+        ClearSSLClientAuthHandlerForRequest(request_);
+    request_ = NULL;
+  }
+}
 
-  callback_.Run(cert);
-  // |this| may be deleted at this point.
+void SSLClientAuthHandler::DoSelectCertificate(
+    int render_process_host_id, int render_frame_host_id) {
+  GetContentClient()->browser()->SelectClientCertificate(
+      render_process_host_id,
+      render_frame_host_id,
+      http_network_session_,
+      cert_request_info_.get(),
+      base::Bind(&SSLClientAuthHandler::CertificateSelected, this));
 }
 
 }  // namespace content
diff --git a/content/browser/ssl/ssl_client_auth_handler.h b/content/browser/ssl/ssl_client_auth_handler.h
index f95e65d..b848d54 100644
--- a/content/browser/ssl/ssl_client_auth_handler.h
+++ b/content/browser/ssl/ssl_client_auth_handler.h
@@ -6,57 +6,79 @@
 #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/memory/weak_ptr.h"
+#include "base/sequenced_task_runner_helpers.h"
+#include "content/common/content_export.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. 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 {
+// 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> {
  public:
-  typedef base::Callback<void(net::X509Certificate*)> CertificateCallback;
-
   SSLClientAuthHandler(scoped_ptr<net::ClientCertStore> client_cert_store,
                        net::URLRequest* request,
-                       net::SSLCertRequestInfo* cert_request_info,
-                       const CertificateCallback& callback);
-  ~SSLClientAuthHandler();
+                       net::SSLCertRequestInfo* cert_request_info);
 
   // 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>;
+
   // Called when ClientCertStore is done retrieving the cert list.
   void DidGetClientCerts();
 
-  // Called when the user has selected a cert.
-  void CertificateSelected(net::X509Certificate* cert);
+  // Notifies that the user has selected a cert.
+  // Called on the IO thread.
+  void DoCertificateSelected(net::X509Certificate* cert);
+
+  // Selects a client certificate on the UI thread.
+  void DoSelectCertificate(int render_process_host_id,
+                           int render_frame_host_id);
 
   // The net::URLRequest that triggered this client auth.
   net::URLRequest* request_;
 
+  // The HttpNetworkSession |request_| is associated with.
+  const net::HttpNetworkSession* http_network_session_;
+
   // 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);
 };
 
diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
index e323449..ae99730 100644
--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -176,14 +176,6 @@ 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;
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index c44788b..c489eee 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -58,6 +58,7 @@ class ImageSkia;
 namespace net {
 class CookieOptions;
 class CookieStore;
+class HttpNetworkSession;
 class NetLog;
 class SSLCertRequestInfo;
 class SSLInfo;
@@ -401,8 +402,9 @@ class CONTENT_EXPORT ContentBrowserClient {
   virtual void SelectClientCertificate(
       int render_process_id,
       int render_frame_id,
+      const net::HttpNetworkSession* network_session,
       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.
author	mattm <mattm@chromium.org>	2014-11-25 14:33:28 -0800
committer	Commit bot <commit-bot@chromium.org>	2014-11-25 22:33:59 +0000
commit	e18bf1d9005b08bba3637b16d744245e5467af0e (patch)
tree	59666decfa16683e5227ba864edb4965aab4b6e8 /content
parent	bd576720e621951616af892bcf03ffaac49f1881 (diff)
download	chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.zip chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.tar.gz chromium_src-e18bf1d9005b08bba3637b16d744245e5467af0e.tar.bz2