Merge to 116: Simplify cert_database_mac notifier implementation

There can only be one, so don't have an overly complicated implementation that pretends otherwise. (cherry picked from commit 33d6b57) Bug: 1463342 Change-Id: I95872be22a7609de23e43394685a46e628d4f3fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4676148 Reviewed-by: David Benjamin <davidben@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Commit-Queue: Matt Mueller <mattm@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1168427} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4682165 Auto-Submit: Matt Mueller <mattm@chromium.org> Cr-Commit-Position: refs/branch-heads/5845@{#466} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
chromium · Jul 13, 2023 · 33285cb · 33285cb
1 parent c322a2e
commit 33285cb
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 88 deletions.
diff --git a/content/browser/network_service_client.cc b/content/browser/network_service_client.cc
@@ -118,8 +118,7 @@ NetworkServiceClient::NetworkServiceClient()
 {
 
 #if BUILDFLAG(IS_MAC)
-  if (base::CurrentUIThread::IsSet())  // Not set in some unit tests.
-    net::CertDatabase::GetInstance()->StartListeningForKeychainEvents();
+  net::CertDatabase::StartListeningForKeychainEvents();
 #endif
 
   if (IsOutOfProcessNetworkService()) {

diff --git a/net/cert/cert_database.h b/net/cert/cert_database.h
@@ -5,7 +5,6 @@
 #ifndef NET_CERT_CERT_DATABASE_H_
 #define NET_CERT_CERT_DATABASE_H_
 
-#include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/no_destructor.h"
 #include "build/build_config.h"
@@ -82,10 +81,9 @@ class NET_EXPORT CertDatabase {
   void RemoveObserver(Observer* observer);
 
 #if BUILDFLAG(IS_MAC)
-  // Start observing and forwarding events from Keychain services on the
-  // current thread. Current thread must have an associated CFRunLoop,
-  // which means that this must be called from a MessageLoop of TYPE_UI.
-  void StartListeningForKeychainEvents();
+  // Start observing and forwarding events from Keychain services. May be
+  // called multiple times, and may be called on any thread.
+  static void StartListeningForKeychainEvents();
 #endif
 
   // Synthetically injects notifications to all observers. In general, this
@@ -100,14 +98,6 @@ class NET_EXPORT CertDatabase {
   CertDatabase();
 
   const scoped_refptr<base::ObserverListThreadSafe<Observer>> observer_list_;
-
-#if BUILDFLAG(IS_MAC)
-  void ReleaseNotifier();
-
-  class Notifier;
-  friend class Notifier;
-  raw_ptr<Notifier, DanglingUntriaged> notifier_ = nullptr;
-#endif
 };
 
 }  // namespace net

diff --git a/net/cert/cert_database_mac.cc b/net/cert/cert_database_mac.cc
@@ -10,61 +10,24 @@
 #include "base/functional/bind.h"
 #include "base/location.h"
 #include "base/mac/mac_logging.h"
-#include "base/memory/raw_ptr.h"
 #include "base/notreached.h"
 #include "base/process/process_handle.h"
-#include "base/synchronization/lock.h"
-#include "base/task/current_thread.h"
-#include "base/task/single_thread_task_runner.h"
-#include "crypto/mac_security_services_lock.h"
-#include "net/base/net_errors.h"
-#include "net/cert/x509_certificate.h"
+#include "net/base/network_notification_thread_mac.h"
 
 namespace net {
 
+namespace {
+
 // Helper that observes events from the Keychain and forwards them to the
-// given CertDatabase.
-class CertDatabase::Notifier {
+// CertDatabase.
+class Notifier {
  public:
-  // Creates a new Notifier that will forward Keychain events to |cert_db|.
-  // |message_loop| must refer to a thread with an associated CFRunLoop - a
-  // TYPE_UI thread. Events will be dispatched from this message loop.
-  Notifier(CertDatabase* cert_db,
-           scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-      : cert_db_(cert_db), task_runner_(std::move(task_runner)) {
-    // Ensure an associated CFRunLoop.
-    DCHECK(base::CurrentUIThread::IsSet());
-    DCHECK(task_runner_->BelongsToCurrentThread());
-    task_runner_->PostTask(
+  Notifier() {
+    GetNetworkNotificationThreadMac()->PostTask(
         FROM_HERE, base::BindOnce(&Notifier::Init, base::Unretained(this)));
   }
 
-// Much of the Keychain API was marked deprecated as of the macOS 13 SDK.
-// Removal of its use is tracked in https://crbug.com/1348251 but deprecation
-// warnings are disabled in the meanwhile.
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-
-  // Should be called from the |task_runner_|'s sequence. Use Shutdown()
-  // to shutdown on arbitrary sequence.
-  ~Notifier() {
-    DCHECK(called_shutdown_);
-    // Only unregister from the same sequence where registration was performed.
-    if (registered_ && task_runner_->RunsTasksInCurrentSequence())
-      SecKeychainRemoveCallback(&Notifier::KeychainCallback);
-  }
-
-#pragma clang diagnostic pop
-
-  void Shutdown() {
-    called_shutdown_ = true;
-    if (!task_runner_->DeleteSoon(FROM_HERE, this)) {
-      // If the task runner is no longer running, it's safe to just delete
-      // the object, since no further events will or can be delivered by
-      // Keychain Services.
-      delete this;
-    }
-  }
+  ~Notifier() = delete;
 
 // Much of the Keychain API was marked deprecated as of the macOS 13 SDK.
 // Removal of its use is tracked in https://crbug.com/1348251 but deprecation
@@ -76,10 +39,7 @@ class CertDatabase::Notifier {
   void Init() {
     SecKeychainEventMask event_mask =
         kSecKeychainListChangedMask | kSecTrustSettingsChangedEventMask;
-    OSStatus status = SecKeychainAddCallback(&Notifier::KeychainCallback,
-                                             event_mask, this);
-    if (status == noErr)
-      registered_ = true;
+    SecKeychainAddCallback(&Notifier::KeychainCallback, event_mask, nullptr);
   }
 
 #pragma clang diagnostic pop
@@ -89,20 +49,12 @@ class CertDatabase::Notifier {
   static OSStatus KeychainCallback(SecKeychainEvent keychain_event,
                                    SecKeychainCallbackInfo* info,
                                    void* context);
-
-  const raw_ptr<CertDatabase> cert_db_;
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-  bool registered_ = false;
-  bool called_shutdown_ = false;
 };
 
 // static
-OSStatus CertDatabase::Notifier::KeychainCallback(
-    SecKeychainEvent keychain_event,
-    SecKeychainCallbackInfo* info,
-    void* context) {
-  Notifier* that = reinterpret_cast<Notifier*>(context);
-
+OSStatus Notifier::KeychainCallback(SecKeychainEvent keychain_event,
+                                    SecKeychainCallbackInfo* info,
+                                    void* context) {
   if (info->version > SEC_KEYCHAIN_SETTINGS_VERS1) {
     NOTREACHED();
     return errSecWrongSecVersion;
@@ -119,10 +71,10 @@ OSStatus CertDatabase::Notifier::KeychainCallback(
 
   switch (keychain_event) {
     case kSecKeychainListChangedEvent:
-      that->cert_db_->NotifyObserversClientCertStoreChanged();
+      CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
       break;
     case kSecTrustSettingsChangedEvent:
-      that->cert_db_->NotifyObserversTrustStoreChanged();
+      CertDatabase::GetInstance()->NotifyObserversTrustStoreChanged();
       break;
 
     default:
@@ -132,18 +84,10 @@ OSStatus CertDatabase::Notifier::KeychainCallback(
   return errSecSuccess;
 }
 
-void CertDatabase::StartListeningForKeychainEvents() {
-  ReleaseNotifier();
-  notifier_ =
-      new Notifier(this, base::SingleThreadTaskRunner::GetCurrentDefault());
-}
+}  // namespace
 
-void CertDatabase::ReleaseNotifier() {
-  // Shutdown will take care to delete the notifier on the right thread.
-  if (notifier_) {
-    notifier_->Shutdown();
-    notifier_ = nullptr;
-  }
+void CertDatabase::StartListeningForKeychainEvents() {
+  static base::NoDestructor<Notifier> notifier;
 }
 
 }  // namespace net