Switch macOS to the views client certificate selector

Now that we are using views on Mac, we can use the same client
certificate selector used on CrOS, Linux, and Windows. This means less
code to maintain, and it also avoids a host of bugs with the
tab-constrained sheet logic which has become increasingly buggy as
Chrome and macOS have evolved. (See associated bugs.)

This retains tab-modality, so sites cannot prevent tab switching or tab
closing, but it switches us from the native sheet to our own UI.

Screenshots:
https://drive.google.com/drive/folders/1YlVAWm-xL8ZDP9k-amdBb4l18nh08rJZ?usp=sharing

This should leave parts of components/constrained_window and
ClientCertIdentity::sec_identity_ref unused. A follow-up CL will unwind
that logic.

Bug: 983451, 1020622, 1078158, 1098786
Change-Id: I9b2ea99a91e63e45934725dead6bade35ec3ca4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2314036Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793238}

chrome/browser/ssl/ssl_client_auth_requestor_mock.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/ssl/ssl_client_auth_requestor_mock.h"
 
+#include <utility>
+
+#include "base/callback.h"
 #include "base/macros.h"
 #include "content/public/browser/client_certificate_delegate.h"
 #include "net/cert/x509_certificate.h"
@@ -15,12 +18,15 @@ namespace {
 class FakeClientCertificateDelegate
     : public content::ClientCertificateDelegate {
  public:
-  explicit FakeClientCertificateDelegate(SSLClientAuthRequestorMock* requestor)
-      : requestor_(requestor) {}
+  FakeClientCertificateDelegate(SSLClientAuthRequestorMock* requestor,
+                                base::OnceClosure done_callback)
+      : requestor_(requestor), done_callback_(std::move(done_callback)) {}
 
   ~FakeClientCertificateDelegate() override {
-    if (requestor_)
+    if (requestor_) {
       requestor_->CancelCertificateSelection();
+      std::move(done_callback_).Run();
+    }
   }
 
   // content::ClientCertificateDelegate implementation:
@@ -28,10 +34,12 @@ class FakeClientCertificateDelegate
                                scoped_refptr<net::SSLPrivateKey> key) override {
     requestor_->CertificateSelected(cert.get(), key.get());
     requestor_ = nullptr;
+    std::move(done_callback_).Run();
   }
 
  private:
   scoped_refptr<SSLClientAuthRequestorMock> requestor_;
+  base::OnceClosure done_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeClientCertificateDelegate);
 };
@@ -47,5 +55,10 @@ SSLClientAuthRequestorMock::~SSLClientAuthRequestorMock() {}
 
 std::unique_ptr<content::ClientCertificateDelegate>
 SSLClientAuthRequestorMock::CreateDelegate() {
-  return std::make_unique<FakeClientCertificateDelegate>(this);
+  return std::make_unique<FakeClientCertificateDelegate>(
+      this, run_loop_.QuitClosure());
+}
+
+void SSLClientAuthRequestorMock::WaitForCompletion() {
+  run_loop_.Run();
 }

chrome/browser/ssl/ssl_client_auth_requestor_mock.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace content {
@@ -27,6 +28,7 @@ class SSLClientAuthRequestorMock
       const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info);
 
   std::unique_ptr<content::ClientCertificateDelegate> CreateDelegate();
+  void WaitForCompletion();
 
   MOCK_METHOD2(CertificateSelected,
                void(net::X509Certificate* cert, net::SSLPrivateKey* key));
@@ -37,6 +39,9 @@ class SSLClientAuthRequestorMock
  protected:
   friend class base::RefCountedThreadSafe<SSLClientAuthRequestorMock>;
   virtual ~SSLClientAuthRequestorMock();
+
+ private:
+  base::RunLoop run_loop_;
 };
 
 #endif  // CHROME_BROWSER_SSL_SSL_CLIENT_AUTH_REQUESTOR_MOCK_H_

chrome/browser/ssl/ssl_client_certificate_selector_test.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/ssl/ssl_client_certificate_selector_test.h"
-
-#include "base/bind.h"
-#include "base/files/file_path.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "content/public/browser/web_contents.h"
-#include "content/public/test/browser_test_utils.h"
-#include "net/base/request_priority.h"
-#include "net/http/http_transaction_factory.h"
-#include "net/ssl/ssl_cert_request_info.h"
-#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-
-using ::testing::Mock;
-using ::testing::StrictMock;
-
-SSLClientCertificateSelectorTestBase::SSLClientCertificateSelectorTestBase() =
-    default;
-
-SSLClientCertificateSelectorTestBase::~SSLClientCertificateSelectorTestBase() =
-    default;
-
-void SSLClientCertificateSelectorTestBase::SetUpInProcessBrowserTestFixture() {
-  cert_request_info_ = base::MakeRefCounted<net::SSLCertRequestInfo>();
-  cert_request_info_->host_and_port = net::HostPortPair("foo", 123);
-}
-
-void SSLClientCertificateSelectorTestBase::SetUpOnMainThread() {
-  auth_requestor_ =
-      new StrictMock<SSLClientAuthRequestorMock>(cert_request_info_.get());
-
-  EXPECT_TRUE(content::WaitForLoadStop(
-      browser()->tab_strip_model()->GetActiveWebContents()));
-}
-
-// Have to release our reference to the auth handler during the test to allow
-// it to be destroyed while the Browser still exists.
-void SSLClientCertificateSelectorTestBase::TearDownOnMainThread() {
-  auth_requestor_.reset();
-}

chrome/browser/ssl/ssl_client_certificate_selector_test.h

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_SSL_SSL_CLIENT_CERTIFICATE_SELECTOR_TEST_H_
-#define CHROME_BROWSER_SSL_SSL_CLIENT_CERTIFICATE_SELECTOR_TEST_H_
-
-#include <memory>
-
-#include "chrome/browser/ssl/ssl_client_auth_requestor_mock.h"
-#include "chrome/test/base/in_process_browser_test.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-class SSLClientCertificateSelectorTestBase : public InProcessBrowserTest {
- public:
-  SSLClientCertificateSelectorTestBase();
-  ~SSLClientCertificateSelectorTestBase() override;
-
-  // InProcessBrowserTest:
-  void SetUpInProcessBrowserTestFixture() override;
-  void SetUpOnMainThread() override;
-  void TearDownOnMainThread() override;
-
- protected:
-  scoped_refptr<net::SSLCertRequestInfo> cert_request_info_;
-  scoped_refptr<testing::StrictMock<SSLClientAuthRequestorMock> >
-      auth_requestor_;
-};
-
-#endif  // CHROME_BROWSER_SSL_SSL_CLIENT_CERTIFICATE_SELECTOR_TEST_H_

chrome/browser/ui/BUILD.gn

@@ -3743,6 +3743,8 @@ static_library("ui") {
       "views/sharing/sharing_dialog_view.h",
       "views/sharing/sharing_icon_view.cc",
       "views/sharing/sharing_icon_view.h",
+      "views/ssl_client_certificate_selector.cc",
+      "views/ssl_client_certificate_selector.h",
       "views/status_bubble_views.cc",
       "views/status_bubble_views.h",
       "views/storage/storage_pressure_bubble_view.cc",
@@ -3974,8 +3976,6 @@ static_library("ui") {
         "views/frame/browser_view_commands_mac.mm",
         "views/policy/enterprise_startup_dialog_mac_util.h",
         "views/policy/enterprise_startup_dialog_mac_util.mm",
-        "views/ssl_client_certificate_selector_mac.h",
-        "views/ssl_client_certificate_selector_mac.mm",
       ]
 
       # The Views task manager is not used on Mac - a native Cocoa
@@ -3988,8 +3988,6 @@ static_library("ui") {
       sources += [
         "views/create_application_shortcut_view.cc",
         "views/create_application_shortcut_view.h",
-        "views/ssl_client_certificate_selector.cc",
-        "views/ssl_client_certificate_selector.h",
       ]
     }
 

chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc

@@ -180,9 +180,15 @@ class SSLClientCertificateSelectorMultiProfileTest
   }
 
   void SetUpOnMainThread() override {
+    SSLClientCertificateSelectorTest::SetUpOnMainThread();
+
     browser_1_ = CreateIncognitoBrowser();
 
-    SSLClientCertificateSelectorTest::SetUpOnMainThread();
+    gfx::NativeWindow window = browser_1_->window()->GetNativeWindow();
+    views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window);
+    ASSERT_NE(nullptr, widget);
+    views::test::WidgetActivationWaiter waiter(widget, true);
+    waiter.Wait();
 
     auth_requestor_1_ =
         new StrictMock<SSLClientAuthRequestorMock>(cert_request_info_1_);
@@ -197,12 +203,6 @@ class SSLClientCertificateSelectorMultiProfileTest
     selector_1_->Init();
     selector_1_->Show();
 
-    gfx::NativeWindow window = browser_1_->window()->GetNativeWindow();
-    views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window);
-    ASSERT_NE(nullptr, widget);
-    views::test::WidgetActivationWaiter waiter(widget, true);
-    waiter.Wait();
-
     ASSERT_TRUE(selector_1_->GetSelectedCert());
     EXPECT_EQ(cert_identity_1_->certificate(),
               selector_1_->GetSelectedCert()->certificate());
@@ -224,17 +224,18 @@ class SSLClientCertificateSelectorMultiProfileTest
 };
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, SelectNone) {
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 
   // Let the mock get checked on destruction.
 }
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, Escape) {
   base::HistogramTester histograms;
-  EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(nullptr, nullptr));
+  EXPECT_CALL(*auth_requestor_, CertificateSelected(nullptr, nullptr));
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_ESCAPE, false, false, false, false));
+  auth_requestor_->WaitForCompletion();
 
   histograms.ExpectUniqueSample(kClientCertSelectHistogramName,
                                 ClientCertSelectionResult::kUserCancel, 1);
@@ -244,12 +245,13 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, Escape) {
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, SelectDefault) {
   base::HistogramTester histograms;
-  EXPECT_CALL(*auth_requestor_.get(),
+  EXPECT_CALL(*auth_requestor_,
               CertificateSelected(cert_identity_1_->certificate(),
                                   cert_identity_1_->ssl_private_key()));
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_RETURN, false, false, false, false));
+  auth_requestor_->WaitForCompletion();
 
   histograms.ExpectUniqueSample(kClientCertSelectHistogramName,
                                 ClientCertSelectionResult::kUserSelect, 1);
@@ -259,9 +261,10 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, SelectDefault) {
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, CloseTab) {
   base::HistogramTester histograms;
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 
   browser()->tab_strip_model()->CloseAllTabs();
+  auth_requestor_->WaitForCompletion();
 
   histograms.ExpectBucketCount(kClientCertSelectHistogramName,
                                ClientCertSelectionResult::kUserCloseTab, 1);
@@ -273,11 +276,13 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, Escape) {
   // auth_requestor_1_ should get selected automatically by the
   // SSLClientAuthObserver when selector_2_ is accepted, since both 1 & 2 have
   // the same host:port.
-  EXPECT_CALL(*auth_requestor_1_.get(), CertificateSelected(nullptr, nullptr));
-  EXPECT_CALL(*auth_requestor_2_.get(), CertificateSelected(nullptr, nullptr));
+  EXPECT_CALL(*auth_requestor_1_, CertificateSelected(nullptr, nullptr));
+  EXPECT_CALL(*auth_requestor_2_, CertificateSelected(nullptr, nullptr));
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_ESCAPE, false, false, false, false));
+  auth_requestor_1_->WaitForCompletion();
+  auth_requestor_2_->WaitForCompletion();
 
   Mock::VerifyAndClear(auth_requestor_.get());
   Mock::VerifyAndClear(auth_requestor_1_.get());
@@ -285,17 +290,17 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, Escape) {
 
   // Now let the default selection for auth_requestor_ mock get checked on
   // destruction.
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 }
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, SelectSecond) {
   // auth_requestor_1_ should get selected automatically by the
   // SSLClientAuthObserver when selector_2_ is accepted, since both 1 & 2 have
   // the same host:port.
-  EXPECT_CALL(*auth_requestor_1_.get(),
+  EXPECT_CALL(*auth_requestor_1_,
               CertificateSelected(cert_identity_2_->certificate(),
                                   cert_identity_2_->ssl_private_key()));
-  EXPECT_CALL(*auth_requestor_2_.get(),
+  EXPECT_CALL(*auth_requestor_2_,
               CertificateSelected(cert_identity_2_->certificate(),
                                   cert_identity_2_->ssl_private_key()));
 
@@ -314,6 +319,8 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, SelectSecond) {
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_RETURN, false, false, false, false));
+  auth_requestor_1_->WaitForCompletion();
+  auth_requestor_2_->WaitForCompletion();
 
   Mock::VerifyAndClear(auth_requestor_.get());
   Mock::VerifyAndClear(auth_requestor_1_.get());
@@ -321,36 +328,38 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, SelectSecond) {
 
   // Now let the default selection for auth_requestor_ mock get checked on
   // destruction.
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 }
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest, Escape) {
-  EXPECT_CALL(*auth_requestor_1_.get(), CertificateSelected(nullptr, nullptr));
+  EXPECT_CALL(*auth_requestor_1_, CertificateSelected(nullptr, nullptr));
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser_1_, ui::VKEY_ESCAPE, false, false, false, false));
+  auth_requestor_1_->WaitForCompletion();
 
   Mock::VerifyAndClear(auth_requestor_.get());
   Mock::VerifyAndClear(auth_requestor_1_.get());
 
   // Now let the default selection for auth_requestor_ mock get checked on
   // destruction.
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 }
 
 IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest,
                        SelectDefault) {
-  EXPECT_CALL(*auth_requestor_1_.get(),
+  EXPECT_CALL(*auth_requestor_1_,
               CertificateSelected(cert_identity_1_->certificate(),
                                   cert_identity_1_->ssl_private_key()));
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(
       browser_1_, ui::VKEY_RETURN, false, false, false, false));
+  auth_requestor_1_->WaitForCompletion();
 
   Mock::VerifyAndClear(auth_requestor_.get());
   Mock::VerifyAndClear(auth_requestor_1_.get());
 
   // Now let the default selection for auth_requestor_ mock get checked on
   // destruction.
-  EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection());
+  EXPECT_CALL(*auth_requestor_, CancelCertificateSelection());
 }

chrome/browser/ui/views/ssl_client_certificate_selector_mac.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_UI_VIEWS_SSL_CLIENT_CERTIFICATE_SELECTOR_MAC_H_
-#define CHROME_BROWSER_UI_VIEWS_SSL_CLIENT_CERTIFICATE_SELECTOR_MAC_H_
-
-#include <memory>
-
-#include "base/callback.h"
-#include "net/ssl/client_cert_identity.h"
-
-// This header file exists only for testing.  Chrome should access the
-// certificate selector only through the cross-platform interface
-// chrome/browser/ssl_client_certificate_selector.h.
-
-namespace content {
-class ClientCertificateDelegate;
-class WebContents;
-}  // namespace content
-
-namespace net {
-class SSLCertRequestInfo;
-}
-
-namespace chrome {
-
-class OkAndCancelableForTesting {
- public:
-  virtual void ClickOkButton() = 0;
-  virtual void ClickCancelButton() = 0;
-};
-
-OkAndCancelableForTesting* ShowSSLClientCertificateSelectorMacForTesting(
-    content::WebContents* contents,
-    net::SSLCertRequestInfo* cert_request_info,
-    net::ClientCertIdentityList client_certs,
-    std::unique_ptr<content::ClientCertificateDelegate> delegate,
-    base::OnceClosure dealloc_closure);
-
-}  // namespace chrome
-
-#endif  // CHROME_BROWSER_UI_VIEWS_SSL_CLIENT_CERTIFICATE_SELECTOR_MAC_H_

chrome/test/BUILD.gn

@@ -1202,8 +1202,6 @@ if (!is_android) {
       "../browser/ssl/ocsp_browsertest.cc",
       "../browser/ssl/security_state_tab_helper_browsertest.cc",
       "../browser/ssl/ssl_browsertest.cc",
-      "../browser/ssl/ssl_client_certificate_selector_test.cc",
-      "../browser/ssl/ssl_client_certificate_selector_test.h",
       "../browser/ssl/stateful_ssl_host_state_delegate_test.cc",
       "../browser/storage/durable_storage_browsertest.cc",
       "../browser/storage_access_api/api_browsertest.cc",
@@ -2685,7 +2683,6 @@ if (!is_android) {
         "../browser/ui/cocoa/task_manager_mac_browsertest.mm",
         "../browser/ui/cocoa/touchbar/browser_window_touch_bar_controller_browsertest.mm",
         "../browser/ui/views/frame/browser_non_client_frame_view_mac_browsertest.cc",
-        "../browser/ui/views/ssl_client_certificate_selector_mac_browsertest.mm",
         "../common/profiler/stack_sampling_browsertest.cc",
 
         # TODO(crbug/845389): Re-Enable the following, which were temporarily
@@ -6118,6 +6115,7 @@ if (!is_android) {
         "../browser/ui/views/passwords/password_bubble_interactive_uitest.cc",
         "../browser/ui/views/permission_bubble/permission_bubble_interactive_uitest.cc",
         "../browser/ui/views/sad_tab_view_interactive_uitest.cc",
+        "../browser/ui/views/ssl_client_certificate_selector_browsertest.cc",
         "../browser/ui/views/status_icons/status_tray_state_changer_interactive_uitest_win.cc",
         "../browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc",
         "../browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h",
@@ -6152,10 +6150,6 @@ if (!is_android) {
       }
       if (is_mac) {
         deps += [ "//content/test:browsertest_support" ]
-      } else {
-        sources += [
-          "../browser/ui/views/ssl_client_certificate_selector_browsertest.cc",
-        ]
       }
       if (is_win && use_aura) {
         sources += [