Cancel client auth requests when not promptable.

chrome/browser/ui/android/ssl_client_certificate_request.cc

Index: chrome/browser/ui/android/ssl_client_certificate_request.cc
diff --git a/chrome/browser/ui/android/ssl_client_certificate_request.cc b/chrome/browser/ui/android/ssl_client_certificate_request.cc
index 65c9adafafd3265103280ed4e0f8c11c9b330d03..ce9c9798cff9a7ab6556ff7ff5a367130c515f72 100644
--- a/chrome/browser/ui/android/ssl_client_certificate_request.cc
+++ b/chrome/browser/ui/android/ssl_client_certificate_request.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/ssl/ssl_client_certificate_selector.h"
 #include "chrome/browser/ui/android/window_android_helper.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/client_certificate_delegate.h"
 #include "crypto/scoped_openssl_types.h"
 #include "jni/SSLClientCertificateRequest_jni.h"
 #include "net/android/keystore_openssl.h"
@@ -44,18 +45,13 @@ void RecordClientCertificateKey(
 void StartClientCertificateRequest(
     const net::SSLCertRequestInfo* cert_request_info,
     ui::WindowAndroid* window,
-    const chrome::SelectCertificateCallback& callback) {
+    scoped_ptr<content::ClientCertificateDelegate> delegate) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Ensure that callback(NULL) is posted as a task on the UI thread
-  // in case of an error.
-  base::Closure post_task_closure = base::Bind(
-      base::IgnoreResult(&content::BrowserThread::PostTask),
-      content::BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(callback, scoped_refptr<net::X509Certificate>()));
-
-  base::ScopedClosureRunner guard(post_task_closure);
+  // Ensure that ContinueWithCertificate(nullptr) is called in case of an error.
+  base::ScopedClosureRunner guard(base::Bind(
+      &content::ClientCertificateDelegate::ContinueWithCertificate,

[mattm, 2015/02/14 00:32:58]

Is there a reason these aren't CancelCertificateSelection?

[davidben, 2015/02/18 22:31:44]

Mostly I figured I'd leave the ones I wasn't sure about alone so I don't
accidentally change import behavior. I agree CancelCertificateSelection is
correct here and flipped it to that.

+      base::Unretained(delegate.get()), scoped_refptr<net::X509Certificate>()));
 
   // Build the |key_types| JNI parameter, as a String[]
   std::vector<std::string> key_types;
@@ -99,12 +95,8 @@ void StartClientCertificateRequest(
       base::android::ConvertUTF8ToJavaString(
           env, cert_request_info->host_and_port.host());
 
-  // Create a copy of the callback on the heap so that its address
-  // and ownership can be passed through and returned from Java via JNI.
-  scoped_ptr<chrome::SelectCertificateCallback> request(
-      new chrome::SelectCertificateCallback(callback));
-
-  jlong request_id = reinterpret_cast<intptr_t>(request.get());
+  // Pass the address of the delegate through to Java.
+  jlong request_id = reinterpret_cast<intptr_t>(delegate.get());
 
   if (!chrome::android::
       Java_SSLClientCertificateRequest_selectClientCertificate(
@@ -121,7 +113,7 @@ void StartClientCertificateRequest(
   ignore_result(guard.Release());
 
   // Ownership was transferred to Java.
-  ignore_result(request.release());
+  ignore_result(delegate.release());
 }
 
 }  // namespace
@@ -147,15 +139,14 @@ static void OnSystemRequestCompletion(
     jobject private_key_ref) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Take back ownership of the request object.
-  scoped_ptr<chrome::SelectCertificateCallback> callback(
-      reinterpret_cast<chrome::SelectCertificateCallback*>(request_id));
-
-  // Ensure that callback(NULL) is called in case of an error.
-  base::Closure null_closure =
-      base::Bind(*callback, scoped_refptr<net::X509Certificate>());
+  // Take back ownership of the delegate object.
+  scoped_ptr<content::ClientCertificateDelegate> delegate(
+      reinterpret_cast<content::ClientCertificateDelegate*>(request_id));
 
-  base::ScopedClosureRunner guard(null_closure);
+  // Ensure that ContinueWithCertificate(nullptr) is called in case of an error.
+  base::ScopedClosureRunner guard(base::Bind(
+      &content::ClientCertificateDelegate::ContinueWithCertificate,
+      base::Unretained(delegate.get()), scoped_refptr<net::X509Certificate>()));

[mmenke, 2015/02/17 19:50:14]

Hrm...This is more than a little ugly.  We're depending on this being destroyed
before the delegate and calling Run() synchronously when we call return.  We
depend on destruction order a lot, which is fine, but this dependency seems less
obvious than most.  Not a change of how this code works, though, so I guess it's
fine.

[davidben, 2015/02/18 22:31:44]

Acknowledged.

 
   if (encoded_chain_ref == NULL || private_key_ref == NULL) {
     LOG(ERROR) << "Client certificate request cancelled";
@@ -195,12 +186,11 @@ static void OnSystemRequestCompletion(
   // before the callback is called with the selected certificate on
   // the UI thread.
   content::BrowserThread::PostTaskAndReply(
-      content::BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&RecordClientCertificateKey,
-                 client_cert,
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&RecordClientCertificateKey, client_cert,
                  base::Passed(&private_key)),
-      base::Bind(*callback, client_cert));
+      base::Bind(&content::ClientCertificateDelegate::ContinueWithCertificate,
+                 base::Owned(delegate.release()), client_cert));
 }
 
 static void NotifyClientCertificatesChanged() {
@@ -227,12 +217,12 @@ bool RegisterSSLClientCertificateRequestAndroid(JNIEnv* env) {
 void ShowSSLClientCertificateSelector(
     content::WebContents* contents,
     net::SSLCertRequestInfo* cert_request_info,
-    const chrome::SelectCertificateCallback& callback) {
+    scoped_ptr<content::ClientCertificateDelegate> delegate) {
   ui::WindowAndroid* window =
       WindowAndroidHelper::FromWebContents(contents)->GetWindowAndroid();
   DCHECK(window);
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  StartClientCertificateRequest(cert_request_info, window, callback);
+  StartClientCertificateRequest(cert_request_info, window, delegate.Pass());
 }
 
 }  // namespace chrome