Un-refcount SSLClientAuthHandler.

content/browser/ssl/ssl_client_auth_handler.cc

Index: content/browser/ssl/ssl_client_auth_handler.cc
diff --git a/content/browser/ssl/ssl_client_auth_handler.cc b/content/browser/ssl/ssl_client_auth_handler.cc
index 80d30677fde75745b3d644e2999cf3b35ee067eb..fe2fac9012a1b66ea11a0f5ff49e2d5e0d4d6fc3 100644
--- a/content/browser/ssl/ssl_client_auth_handler.cc
+++ b/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));

[pneubeck (no reviews), 2014/12/14 16:45:12]

this seems still to be a rather weird ownership:
 Core lives as long client_cert_store_ keeps this callback that owns the Core
object.
 Core object owns client_cert_store_.

By the definition of 'Ownership = Trigger of destruction', this is a cyclic
dependency and we should fix this.

I looked into this a while ago and could take care of it. It's not a tiny change
however.

[davidben, 2015/01/23 21:05:35]

Agreed. It's still the same issue that we had before, but pushed behind the
SSLClientAuthHandler interface. So some progress, but the problem hasn't gone
away.

The fix you're thinking of, I assume, is to make it safe to kill
client_cert_store_ while GetClientCerts is in flight? And then we'd have to sort
out the craziness around cert_request_info_ and
cert_request_info_->client_certs. Then we won't need this silly Core at all.

I've added a TODO for now.

+    } 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),

[pneubeck (no reviews), 2014/12/14 16:45:12]

not required?

[davidben, 2015/01/23 21:05:35]

Done.

+      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() {

[pneubeck (no reviews), 2014/12/14 16:45:12]

this should still somehow trigger a "shutdown" of the Core object. Currently,
core_ might continue with an arbitrary number of steps.

[davidben, 2015/01/23 21:05:35]

I think this is also blocking on sorting out GetClientCerts' requirements,
otherwise there's nothing to shut down.

 }
 
 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);

[mmenke, 2014/12/14 21:04:51]

Think this should use reset and return on the callback, for it to be cleaned up
correctly.

[davidben, 2015/01/23 21:05:35]

Hrm. Does it need to be? SelectCertificate should only get used once and such.

+  // |this| may be deleted at this point.

[pneubeck (no reviews), 2014/12/14 16:45:13]

^^
shouldn't the implementor of the callback prevent this case? it seems to be in
general a very bad idea to delete your caller.
otherwise, we would have to put such comments behind nearly every callback.Run()
where the callback was provided from some external code.

[mmenke, 2014/12/14 21:04:51]

In general, I think deletion tasks are uglier.  Deleting your caller on
completion is a pretty common design pattern.

[pneubeck (no reviews), 2014/12/15 21:11:44]

There also several definitions available that you can use to make it easily
readable:
 BrowserThread::DeleteSoon
or
 scoped_ptr<Foo, BrowserThread::DeleteOnIOThread>

Is there a more concrete reason why these aren't preferable?

[davidben, 2015/01/23 21:05:35]

That actually doesn't do the same thing. If you're currently on the IO thread,
it'll delete it immediately rather than post a task. But I've done a DeleteSoon.

 }
 
 }  // namespace content