Move OnCertDBChanged to SSLClientContext.

Rather than have SpdySessionPool and the socket pools listen to both
SSLClientContext and the CertDatabase separately, lump them both under
SSL-external notifications of SSL state changing.

(Note ClientSocketPoolManagerImpl's SSLClientContext::Observer
counterpart is TransportClientSocketPool. Also note this fixes
an inconsistency between HTTP/1.1 and HTTP/2. HTTP/2 had a
ERR_NETWORK_CHANGED vs ERR_CERT_DATABASE_CHANGED distinction while
HTTP/1.1 did not. I've made them both match.)

Change-Id: Ibde71856fc1d605757dbf869ce1eba482a6bbafc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703232
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683760}

net/socket/client_socket_pool_manager_impl.cc

@@ -32,13 +32,10 @@ ClientSocketPoolManagerImpl::ClientSocketPoolManagerImpl(
   // connections.
   DCHECK(!common_connect_job_params_.websocket_endpoint_lock_manager);
   DCHECK(websocket_common_connect_job_params.websocket_endpoint_lock_manager);
-
-  CertDatabase::GetInstance()->AddObserver(this);
 }
 
 ClientSocketPoolManagerImpl::~ClientSocketPoolManagerImpl() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
 void ClientSocketPoolManagerImpl::FlushSocketPoolsWithError(int error) {
@@ -111,10 +108,6 @@ ClientSocketPoolManagerImpl::SocketPoolInfoToValue() const {
   return std::move(list);
 }
 
-void ClientSocketPoolManagerImpl::OnCertDBChanged() {
-  FlushSocketPoolsWithError(ERR_NETWORK_CHANGED);
-}
-
 void ClientSocketPoolManagerImpl::DumpMemoryStats(
     base::trace_event::ProcessMemoryDump* pmd,
     const std::string& parent_dump_absolute_name) const {

net/socket/client_socket_pool_manager_impl.h

@@ -15,7 +15,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread_checker.h"
 #include "net/base/net_export.h"
-#include "net/cert/cert_database.h"
 #include "net/http/http_network_session.h"
 #include "net/socket/client_socket_pool_manager.h"
 #include "net/socket/connect_job.h"
@@ -32,8 +31,7 @@ class ProxyServer;
 class ClientSocketPool;
 
 class NET_EXPORT_PRIVATE ClientSocketPoolManagerImpl
-    : public ClientSocketPoolManager,
-      public CertDatabase::Observer {
+    : public ClientSocketPoolManager {
  public:
   // |websocket_common_connect_job_params| is only used for direct WebSocket
   // connections (No proxy in use). It's never used if |pool_type| is not
@@ -52,9 +50,6 @@ class NET_EXPORT_PRIVATE ClientSocketPoolManagerImpl
   // Creates a Value summary of the state of the socket pools.
   std::unique_ptr<base::Value> SocketPoolInfoToValue() const override;
 
-  // CertDatabase::Observer methods:
-  void OnCertDBChanged() override;
-
   void DumpMemoryStats(
       base::trace_event::ProcessMemoryDump* pmd,
       const std::string& parent_dump_absolute_name) const override;

net/socket/ssl_client_socket.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "net/socket/ssl_client_socket_impl.h"
 #include "net/socket/stream_socket.h"
+#include "net/ssl/ssl_client_session_cache.h"
 #include "net/ssl/ssl_key_logger.h"
 
 namespace net {
@@ -67,12 +68,14 @@ SSLClientContext::SSLClientContext(
     config_ = ssl_config_service_->GetSSLContextConfig();
     ssl_config_service_->AddObserver(this);
   }
+  CertDatabase::GetInstance()->AddObserver(this);
 }
 
 SSLClientContext::~SSLClientContext() {
   if (ssl_config_service_) {
     ssl_config_service_->RemoveObserver(this);
   }
+  CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
 std::unique_ptr<SSLClientSocket> SSLClientContext::CreateSSLClientSocket(
@@ -97,8 +100,19 @@ void SSLClientContext::OnSSLContextConfigChanged() {
   // never change version or cipher negotiation based on client-offered
   // sessions, other servers do.
   config_ = ssl_config_service_->GetSSLContextConfig();
+  NotifySSLConfigChanged(false /* not a cert database change */);
+}
+
+void SSLClientContext::OnCertDBChanged() {
+  if (ssl_client_session_cache_) {
+    ssl_client_session_cache_->Flush();
+  }
+  NotifySSLConfigChanged(true /* cert database change */);
+}
+
+void SSLClientContext::NotifySSLConfigChanged(bool is_cert_database_change) {
   for (Observer& observer : observers_) {
-    observer.OnSSLConfigChanged();
+    observer.OnSSLConfigChanged(is_cert_database_change);
   }
 }
 

net/socket/ssl_client_socket.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "net/base/net_export.h"
+#include "net/cert/cert_database.h"
 #include "net/socket/ssl_socket.h"
 #include "net/ssl/ssl_config_service.h"
 
@@ -80,13 +81,14 @@ class NET_EXPORT SSLClientSocket : public SSLSocket {
 };
 
 // Shared state and configuration across multiple SSLClientSockets.
-class NET_EXPORT SSLClientContext : public SSLConfigService::Observer {
+class NET_EXPORT SSLClientContext : public SSLConfigService::Observer,
+                                    public CertDatabase::Observer {
  public:
   class NET_EXPORT Observer : public base::CheckedObserver {
    public:
     // Called when SSL configuration for all hosts changed. Newly-created
     // SSLClientSockets will pick up the new configuration.
-    virtual void OnSSLConfigChanged() = 0;
+    virtual void OnSSLConfigChanged(bool is_cert_database_change) = 0;
   };
 
   // Creates a new SSLClientContext with the specified parameters. The
@@ -135,7 +137,12 @@ class NET_EXPORT SSLClientContext : public SSLConfigService::Observer {
   // SSLConfigService::Observer:
   void OnSSLContextConfigChanged() override;
 
+  // CertDatabase::Observer:
+  void OnCertDBChanged() override;
+
  private:
+  void NotifySSLConfigChanged(bool is_cert_database_change);
+
   SSLContextConfig config_;
 
   SSLConfigService* ssl_config_service_;

net/socket/transport_client_socket_pool.cc

@@ -787,10 +787,12 @@ TransportClientSocketPool::TransportClientSocketPool(
     ssl_client_context_->AddObserver(this);
 }
 
-void TransportClientSocketPool::OnSSLConfigChanged() {
+void TransportClientSocketPool::OnSSLConfigChanged(
+    bool is_cert_database_change) {
   // When the user changes the SSL config, flush all idle sockets so they won't
   // get re-used.
-  FlushWithError(ERR_NETWORK_CHANGED);
+  FlushWithError(is_cert_database_change ? ERR_CERT_DATABASE_CHANGED
+                                         : ERR_NETWORK_CHANGED);
 }
 
 bool TransportClientSocketPool::HasGroup(const GroupId& group_id) const {

net/socket/transport_client_socket_pool.h

@@ -603,7 +603,7 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool
       bool connect_backup_jobs_enabled);
 
   // SSLClientContext::Observer methods.
-  void OnSSLConfigChanged() override;
+  void OnSSLConfigChanged(bool is_cert_database_change) override;
 
   base::TimeDelta ConnectRetryInterval() const {
     // TODO(mbelshe): Make this tuned dynamically based on measured RTT.

net/spdy/spdy_session_pool.cc

@@ -107,7 +107,6 @@ SpdySessionPool::SpdySessionPool(
   NetworkChangeNotifier::AddIPAddressObserver(this);
   if (ssl_client_context_)
     ssl_client_context_->AddObserver(this);
-  CertDatabase::GetInstance()->AddObserver(this);
 }
 
 SpdySessionPool::~SpdySessionPool() {
@@ -133,7 +132,6 @@ SpdySessionPool::~SpdySessionPool() {
   if (ssl_client_context_)
     ssl_client_context_->RemoveObserver(this);
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
-  CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
 base::WeakPtr<SpdySession>
@@ -471,12 +469,9 @@ void SpdySessionPool::OnIPAddressChanged() {
   }
 }
 
-void SpdySessionPool::OnSSLConfigChanged() {
-  CloseCurrentSessions(ERR_NETWORK_CHANGED);
-}
-
-void SpdySessionPool::OnCertDBChanged() {
-  CloseCurrentSessions(ERR_CERT_DATABASE_CHANGED);
+void SpdySessionPool::OnSSLConfigChanged(bool is_cert_database_change) {
+  CloseCurrentSessions(is_cert_database_change ? ERR_CERT_DATABASE_CHANGED
+                                               : ERR_NETWORK_CHANGED);
 }
 
 void SpdySessionPool::RemoveRequestForSpdySession(SpdySessionRequest* request) {

net/spdy/spdy_session_pool.h

@@ -25,7 +25,6 @@
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/proxy_server.h"
-#include "net/cert/cert_database.h"
 #include "net/log/net_log_source.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/socket/connect_job.h"
@@ -57,8 +56,7 @@ class TransportSecurityState;
 // This is a very simple pool for open SpdySessions.
 class NET_EXPORT SpdySessionPool
     : public NetworkChangeNotifier::IPAddressObserver,
-      public SSLClientContext::Observer,
-      public CertDatabase::Observer {
+      public SSLClientContext::Observer {
  public:
   typedef base::TimeTicks (*TimeFunc)(void);
 
@@ -293,13 +291,7 @@ class NET_EXPORT SpdySessionPool
   // SSLClientContext::Observer methods:
 
   // We perform the same flushing as described above when SSL settings change.
-  void OnSSLConfigChanged() override;
-
-  // CertDatabase::Observer methods:
-
-  // We perform the same flushing as described above when certificate database
-  // is changed.
-  void OnCertDBChanged() override;
+  void OnSSLConfigChanged(bool is_cert_database_change) override;
 
   void DumpMemoryStats(base::trace_event::ProcessMemoryDump* pmd,
                        const std::string& parent_dump_absolute_name) const;

net/ssl/ssl_client_session_cache.cc

@@ -31,15 +31,9 @@ SSLClientSessionCache::SSLClientSessionCache(const Config& config)
       lookups_since_flush_(0) {
   memory_pressure_listener_.reset(new base::MemoryPressureListener(base::Bind(
       &SSLClientSessionCache::OnMemoryPressure, base::Unretained(this))));
-  CertDatabase::GetInstance()->AddObserver(this);
 }
 
 SSLClientSessionCache::~SSLClientSessionCache() {
-  CertDatabase::GetInstance()->RemoveObserver(this);
-  Flush();
-}
-
-void SSLClientSessionCache::OnCertDBChanged() {
   Flush();
 }
 

net/ssl/ssl_client_session_cache.h

@@ -18,7 +18,6 @@
 #include "base/time/time.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "net/base/net_export.h"
-#include "net/cert/cert_database.h"
 #include "third_party/boringssl/src/include/openssl/base.h"
 
 namespace base {
@@ -30,7 +29,7 @@ class ProcessMemoryDump;
 
 namespace net {
 
-class NET_EXPORT SSLClientSessionCache : public CertDatabase::Observer {
+class NET_EXPORT SSLClientSessionCache {
  public:
   struct Config {
     // The maximum number of entries in the cache.
@@ -40,9 +39,7 @@ class NET_EXPORT SSLClientSessionCache : public CertDatabase::Observer {
   };
 
   explicit SSLClientSessionCache(const Config& config);
-  ~SSLClientSessionCache() override;
-
-  void OnCertDBChanged() override;
+  ~SSLClientSessionCache();
 
   // Returns true if |entry| is expired as of |now|.
   static bool IsExpired(SSL_SESSION* session, time_t now);