Cancel client auth requests when not promptable.

Currently they hang (holding the cache lock) or crash. This plumbs
through a dedicated delegate interface. If the delegate is destroyed with
no notification, the request is aborted. This is distinct from
affirmatively continuing with no certificat (what you get from pressing
cancel). This is extremely bizarre UI, but this CL does not attempt to
address the existing UI being odd.

This fixes the following:
- Closing a tab with a client auth prompt acts as if you affirmatively selected
  to continue without a cert.
- A SharedWorker requesting client auth hangs the request.
- Hitting client auth in an extension background page crashes.

BUG=417092,410967

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

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

3 files changed, 75 insertions, 47 deletions

diff --git a/android_webview/native/aw_contents_client_bridge.cc b/android_webview/native/aw_contents_client_bridge.cc
index 9d68603..c54b8a1 100644
--- a/android_webview/native/aw_contents_client_bridge.cc
+++ b/android_webview/native/aw_contents_client_bridge.cc
@@ -10,7 +10,9 @@
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/callback_helpers.h"
+#include "base/macros.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
@@ -58,11 +60,18 @@ AwContentsClientBridge::~AwContentsClientBridge() {
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
-  if (obj.is_null())
-    return;
-  // Clear the weak reference from the java peer to the native object since
-  // it is possible that java object lifetime can exceed the AwContens.
-  Java_AwContentsClientBridge_setNativeContentsClientBridge(env, obj.obj(), 0);
+  if (!obj.is_null()) {
+    // Clear the weak reference from the java peer to the native object since
+    // it is possible that java object lifetime can exceed the AwContens.
+    Java_AwContentsClientBridge_setNativeContentsClientBridge(env, obj.obj(),
+                                                              0);
+  }
+
+  for (IDMap<content::ClientCertificateDelegate>::iterator iter(
+           &pending_client_cert_request_delegates_);
+       !iter.IsAtEnd(); iter.Advance()) {
+    delete iter.GetCurrentValue();
+  }
 }
 
 void AwContentsClientBridge::AllowCertificateError(
@@ -114,13 +123,13 @@ void AwContentsClientBridge::ProceedSslError(JNIEnv* env, jobject obj,
 // This method is inspired by SelectClientCertificate() in
 // chrome/browser/ui/android/ssl_client_certificate_request.cc
 void AwContentsClientBridge::SelectClientCertificate(
-      net::SSLCertRequestInfo* cert_request_info,
-      const SelectCertificateCallback& callback) {
+    net::SSLCertRequestInfo* cert_request_info,
+    scoped_ptr<content::ClientCertificateDelegate> delegate) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Add the callback to id map.
-  int request_id = pending_client_cert_request_callbacks_.Add(
-      new SelectCertificateCallback(callback));
+  int request_id =
+      pending_client_cert_request_delegates_.Add(delegate.release());
   // Make sure callback is run on error.
   base::ScopedClosureRunner guard(base::Bind(
       &AwContentsClientBridge::HandleErrorInClientCertificateResponse,
@@ -196,19 +205,24 @@ void AwContentsClientBridge::ProvideClientCertificateResponse(
     jobject private_key_ref) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  SelectCertificateCallback* callback =
-      pending_client_cert_request_callbacks_.Lookup(request_id);
-  DCHECK(callback);
+  content::ClientCertificateDelegate* delegate =
+      pending_client_cert_request_delegates_.Lookup(request_id);
+  DCHECK(delegate);
+
+  if (encoded_chain_ref == NULL || private_key_ref == NULL) {
+    LOG(ERROR) << "No client certificate selected";
+    pending_client_cert_request_delegates_.Remove(request_id);
+    delegate->ContinueWithCertificate(nullptr);
+    delete delegate;
+    return;
+  }
 
   // Make sure callback is run on error.
   base::ScopedClosureRunner guard(base::Bind(
       &AwContentsClientBridge::HandleErrorInClientCertificateResponse,
       base::Unretained(this),
       request_id));
-  if (encoded_chain_ref == NULL || private_key_ref == NULL) {
-    LOG(ERROR) << "Client certificate request cancelled";
-    return;
-  }
+
   // Convert the encoded chain to a vector of strings.
   std::vector<std::string> encoded_chain_strings;
   if (encoded_chain_ref) {
@@ -236,20 +250,20 @@ void AwContentsClientBridge::ProvideClientCertificateResponse(
     return;
   }
 
+  // Release the guard and |pending_client_cert_request_delegates_| references
+  // to |delegate|.
+  pending_client_cert_request_delegates_.Remove(request_id);
+  ignore_result(guard.Release());
+
   // RecordClientCertificateKey() must be called on the I/O thread,
-  // before the callback is called with the selected certificate on
+  // before the delegate 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));
-  pending_client_cert_request_callbacks_.Remove(request_id);
-
-  // Release the guard.
-  ignore_result(guard.Release());
+      base::Bind(&content::ClientCertificateDelegate::ContinueWithCertificate,
+                 base::Owned(delegate), client_cert));
 }
 
 void AwContentsClientBridge::RunJavaScriptDialog(
@@ -377,10 +391,11 @@ void AwContentsClientBridge::CancelJsResult(JNIEnv*, jobject, int id) {
 // Use to cleanup if there is an error in client certificate response.
 void AwContentsClientBridge::HandleErrorInClientCertificateResponse(
     int request_id) {
-  SelectCertificateCallback* callback =
-      pending_client_cert_request_callbacks_.Lookup(request_id);
-  callback->Run(nullptr);
-  pending_client_cert_request_callbacks_.Remove(request_id);
+  content::ClientCertificateDelegate* delegate =
+      pending_client_cert_request_delegates_.Lookup(request_id);
+  pending_client_cert_request_delegates_.Remove(request_id);
+
+  delete delegate;
 }
 
 bool RegisterAwContentsClientBridge(JNIEnv* env) {
diff --git a/android_webview/native/aw_contents_client_bridge.h b/android_webview/native/aw_contents_client_bridge.h
index 2a5caba..a2bde4b 100644
--- a/android_webview/native/aw_contents_client_bridge.h
+++ b/android_webview/native/aw_contents_client_bridge.h
@@ -40,7 +40,7 @@ class AwContentsClientBridge : public AwContentsClientBridgeBase {
                              bool* cancel_request) override;
   void SelectClientCertificate(
       net::SSLCertRequestInfo* cert_request_info,
-      const SelectCertificateCallback& callback) override;
+      scoped_ptr<content::ClientCertificateDelegate> delegate) override;
 
   void RunJavaScriptDialog(
       content::JavaScriptMessageType message_type,
@@ -73,8 +73,10 @@ class AwContentsClientBridge : public AwContentsClientBridgeBase {
   IDMap<CertErrorCallback, IDMapOwnPointer> pending_cert_error_callbacks_;
   IDMap<content::JavaScriptDialogManager::DialogClosedCallback, IDMapOwnPointer>
       pending_js_dialog_callbacks_;
-  IDMap<SelectCertificateCallback, IDMapOwnPointer>
-      pending_client_cert_request_callbacks_;
+  // |pending_client_cert_request_delegates_| owns its pointers, but IDMap
+  // doesn't provide Release, so ownership is managed manually.
+  IDMap<content::ClientCertificateDelegate>
+      pending_client_cert_request_delegates_;
 };
 
 bool RegisterAwContentsClientBridge(JNIEnv* env);
diff --git a/android_webview/native/aw_contents_client_bridge_unittest.cc b/android_webview/native/aw_contents_client_bridge_unittest.cc
index b301f91..4b59144 100644
--- a/android_webview/native/aw_contents_client_bridge_unittest.cc
+++ b/android_webview/native/aw_contents_client_bridge_unittest.cc
@@ -8,8 +8,10 @@
 #include "base/android/jni_array.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/bind.h"
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
+#include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "jni/MockAwContentsClientBridge_jni.h"
 #include "net/android/net_jni_registrar.h"
@@ -33,9 +35,6 @@ namespace {
 // Tests the android_webview contents client bridge.
 class AwContentsClientBridgeTest : public Test {
  public:
-  typedef AwContentsClientBridge::SelectCertificateCallback
-      SelectCertificateCallback;
-
   AwContentsClientBridgeTest() { }
 
   // Callback method called when a cert is selected.
@@ -53,6 +52,24 @@ class AwContentsClientBridgeTest : public Test {
   JNIEnv* env_;
 };
 
+class TestClientCertificateDelegate
+    : public content::ClientCertificateDelegate {
+ public:
+  explicit TestClientCertificateDelegate(AwContentsClientBridgeTest* test)
+      : test_(test) {}
+
+  // content::ClientCertificateDelegate.
+  void ContinueWithCertificate(net::X509Certificate* cert) override {
+    test_->CertSelected(cert);
+    test_ = nullptr;
+  }
+
+ private:
+  AwContentsClientBridgeTest* test_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate);
+};
+
 }   // namespace
 
 void AwContentsClientBridgeTest::SetUp() {
@@ -90,9 +107,7 @@ void AwContentsClientBridgeTest::TestCertType(SSLClientCertType type,
   cert_request_info_->cert_key_types.push_back(type);
   bridge_->SelectClientCertificate(
       cert_request_info_.get(),
-      base::Bind(
-          &AwContentsClientBridgeTest::CertSelected,
-          base::Unretained(static_cast<AwContentsClientBridgeTest*>(this))));
+      make_scoped_ptr(new TestClientCertificateDelegate(this)));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0, cert_selected_callbacks_);
   ScopedJavaLocalRef<jobjectArray> key_types =
@@ -112,10 +127,8 @@ TEST_F(AwContentsClientBridgeTest,
   // Call SelectClientCertificate to create a callback id that mock java object
   // can call on.
   bridge_->SelectClientCertificate(
-    cert_request_info_.get(),
-    base::Bind(
-        &AwContentsClientBridgeTest::CertSelected,
-        base::Unretained(static_cast<AwContentsClientBridgeTest*>(this))));
+      cert_request_info_.get(),
+      make_scoped_ptr(new TestClientCertificateDelegate(this)));
   bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(),
       Java_MockAwContentsClientBridge_getRequestId(env_, jbridge_.obj()),
       Java_MockAwContentsClientBridge_createTestCertChain(
@@ -133,10 +146,8 @@ TEST_F(AwContentsClientBridgeTest,
   // Call SelectClientCertificate to create a callback id that mock java object
   // can call on.
   bridge_->SelectClientCertificate(
-    cert_request_info_.get(),
-    base::Bind(
-        &AwContentsClientBridgeTest::CertSelected,
-        base::Unretained(static_cast<AwContentsClientBridgeTest*>(this))));
+      cert_request_info_.get(),
+      make_scoped_ptr(new TestClientCertificateDelegate(this)));
   int requestId =
     Java_MockAwContentsClientBridge_getRequestId(env_, jbridge_.obj());
   bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(),