[SPDY] Use WeakPtr<HttpServerProperties> instead of raw pointers

This will let us better track down what is causing SpdySessions to be
accessing them after they're destroyed, since we'll have crash reports
instead of just the SyzyASAN reports.

Also use scoped_ptr<HttpServerProperties> when appropriate.

BUG=236451
TBR=ajwong@chromium.org, rtenneti@chromium.org, simonjam@chromium.org, wez@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@212466 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/io_thread.cc

@@ -514,7 +514,7 @@ void IOThread::InitAsync() {
   }
   globals_->http_auth_handler_factory.reset(CreateDefaultAuthHandlerFactory(
       globals_->host_resolver.get()));
-  globals_->http_server_properties.reset(new net::HttpServerPropertiesImpl);
+  globals_->http_server_properties.reset(new net::HttpServerPropertiesImpl());
   // For the ProxyScriptFetcher, we use a direct ProxyService.
   globals_->proxy_script_fetcher_proxy_service.reset(
       net::ProxyService::CreateDirectWithNetLog(net_log_));
@@ -862,7 +862,8 @@ void IOThread::InitializeNetworkSessionParams(
   params->transport_security_state = globals_->transport_security_state.get();
   params->ssl_config_service = globals_->ssl_config_service.get();
   params->http_auth_handler_factory = globals_->http_auth_handler_factory.get();
-  params->http_server_properties = globals_->http_server_properties.get();
+  params->http_server_properties =
+      globals_->http_server_properties->GetWeakPtr();
   params->network_delegate = globals_->system_network_delegate.get();
   params->host_mapping_rules = globals_->host_mapping_rules.get();
   params->ignore_certificate_errors = globals_->ignore_certificate_errors;

chrome/browser/net/connection_tester.cc

@@ -113,7 +113,9 @@ class ExperimentURLRequestContext : public net::URLRequestContext {
     storage_.set_ssl_config_service(new net::SSLConfigServiceDefaults);
     storage_.set_http_auth_handler_factory(
         net::HttpAuthHandlerFactory::CreateDefault(host_resolver()));
-    storage_.set_http_server_properties(new net::HttpServerPropertiesImpl);
+    storage_.set_http_server_properties(
+        scoped_ptr<net::HttpServerProperties>(
+            new net::HttpServerPropertiesImpl()));
 
     net::HttpNetworkSession::Params session_params;
     session_params.host_resolver = host_resolver();

chrome/browser/net/connection_tester_unittest.cc

@@ -138,7 +138,8 @@ class ConnectionTesterTest : public PlatformTest {
     session_params.http_auth_handler_factory = &http_auth_handler_factory_;
     session_params.ssl_config_service = ssl_config_service_.get();
     session_params.proxy_service = proxy_service_.get();
-    session_params.http_server_properties = &http_server_properties_impl_;
+    session_params.http_server_properties =
+        http_server_properties_impl_.GetWeakPtr();
     scoped_refptr<net::HttpNetworkSession> network_session(
         new net::HttpNetworkSession(session_params));
     http_transaction_factory_.reset(

chrome/browser/net/http_server_properties_manager.cc

@@ -67,10 +67,14 @@ HttpServerPropertiesManager::HttpServerPropertiesManager(
 }
 
 HttpServerPropertiesManager::~HttpServerPropertiesManager() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  io_weak_ptr_factory_.reset();
 }
 
 void HttpServerPropertiesManager::InitializeOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  io_weak_ptr_factory_.reset(
+      new base::WeakPtrFactory<HttpServerPropertiesManager>(this));
   http_server_properties_impl_.reset(new net::HttpServerPropertiesImpl());
 
   io_prefs_update_timer_.reset(
@@ -111,6 +115,12 @@ void HttpServerPropertiesManager::SetVersion(
 }
 
 // This is required for conformance with the HttpServerProperties interface.
+base::WeakPtr<net::HttpServerProperties>
+    HttpServerPropertiesManager::GetWeakPtr() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  return io_weak_ptr_factory_->GetWeakPtr();
+}
+
 void HttpServerPropertiesManager::Clear() {
   Clear(base::Closure());
 }

chrome/browser/net/http_server_properties_manager.h

@@ -81,6 +81,9 @@ class HttpServerPropertiesManager
   // net::HttpServerProperties methods:
   // ----------------------------------
 
+  // Gets a weak pointer for this object.
+  virtual base::WeakPtr<net::HttpServerProperties> GetWeakPtr() OVERRIDE;
+
   // Deletes all data. Works asynchronously.
   virtual void Clear() OVERRIDE;
 
@@ -229,6 +232,10 @@ class HttpServerPropertiesManager
   // IO thread
   // ---------
 
+  // Used to get |weak_ptr_| to self on the IO thread.
+  scoped_ptr<base::WeakPtrFactory<HttpServerPropertiesManager> >
+      io_weak_ptr_factory_;
+
   // Used to post |prefs::kHttpServerProperties| pref update tasks.
   scoped_ptr<base::OneShotTimer<HttpServerPropertiesManager> >
       io_prefs_update_timer_;

chrome/browser/profiles/off_the_record_profile_io_data.cc

@@ -190,7 +190,9 @@ void OffTheRecordProfileIOData::InitializeInternal(
       io_thread_globals->throttler_manager.get());
 
   // For incognito, we use the default non-persistent HttpServerPropertiesImpl.
-  set_http_server_properties(new net::HttpServerPropertiesImpl);
+  set_http_server_properties(
+      scoped_ptr<net::HttpServerProperties>(
+          new net::HttpServerPropertiesImpl()));
   main_context->set_http_server_properties(http_server_properties());
 
   // For incognito, we use a non-persistent server bound cert store.

chrome/browser/profiles/profile_impl_io_data.cc

@@ -291,7 +291,8 @@ void ProfileImplIOData::Handle::LazyInitialize() const {
   io_data_->http_server_properties_manager_ =
       new chrome_browser_net::HttpServerPropertiesManager(pref_service);
   io_data_->set_http_server_properties(
-      io_data_->http_server_properties_manager_);
+      scoped_ptr<net::HttpServerProperties>(
+          io_data_->http_server_properties_manager_));
   io_data_->session_startup_pref()->Init(
       prefs::kRestoreOnStartup, pref_service);
   io_data_->session_startup_pref()->MoveToThread(

chrome/browser/profiles/profile_io_data.cc

@@ -596,13 +596,14 @@ bool ProfileIOData::GetMetricsEnabledStateOnIOThread() const {
 #endif  // defined(OS_CHROMEOS)
 }
 
-net::HttpServerProperties* ProfileIOData::http_server_properties() const {
-  return http_server_properties_.get();
+base::WeakPtr<net::HttpServerProperties>
+ProfileIOData::http_server_properties() const {
+  return http_server_properties_->GetWeakPtr();
 }
 
 void ProfileIOData::set_http_server_properties(
-    net::HttpServerProperties* http_server_properties) const {
-  http_server_properties_.reset(http_server_properties);
+    scoped_ptr<net::HttpServerProperties> http_server_properties) const {
+  http_server_properties_ = http_server_properties.Pass();
 }
 
 ProfileIOData::ResourceContext::ResourceContext(ProfileIOData* io_data)

chrome/browser/profiles/profile_io_data.h

@@ -319,10 +319,10 @@ class ProfileIOData {
     return proxy_service_.get();
   }
 
-  net::HttpServerProperties* http_server_properties() const;
+  base::WeakPtr<net::HttpServerProperties> http_server_properties() const;
 
   void set_http_server_properties(
-      net::HttpServerProperties* http_server_properties) const;
+      scoped_ptr<net::HttpServerProperties> http_server_properties) const;
 
   ChromeURLRequestContext* main_request_context() const {
     return main_request_context_.get();

chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc

@@ -88,7 +88,7 @@ void AddDummyHttpPipelineFeedbackOnIOThread(
   net::URLRequestContext* context = context_getter->GetURLRequestContext();
   net::HttpNetworkSession* http_network_session =
       context->http_transaction_factory()->GetSession();
-  net::HttpServerProperties* http_server_properties =
+  base::WeakPtr<net::HttpServerProperties> http_server_properties =
       http_network_session->http_server_properties();
   net::HostPortPair origin(hostname, port);
   http_server_properties->SetPipelineCapability(origin, capability);

chrome/service/net/service_url_request_context.cc

@@ -116,7 +116,9 @@ ServiceURLRequestContext::ServiceURLRequestContext(
   storage_.set_ssl_config_service(new net::SSLConfigServiceDefaults);
   storage_.set_http_auth_handler_factory(
       net::HttpAuthHandlerFactory::CreateDefault(host_resolver()));
-  storage_.set_http_server_properties(new net::HttpServerPropertiesImpl);
+  storage_.set_http_server_properties(
+      scoped_ptr<net::HttpServerProperties>(
+          new net::HttpServerPropertiesImpl()));
   storage_.set_transport_security_state(new net::TransportSecurityState);
   storage_.set_throttler_manager(new net::URLRequestThrottlerManager);
 

content/browser/loader/resource_scheduler_unittest.cc

@@ -126,7 +126,7 @@ class ResourceSchedulerTest : public testing::Test {
         ui_thread_(BrowserThread::UI, &message_loop_),
         io_thread_(BrowserThread::IO, &message_loop_) {
     scheduler_.OnClientCreated(kChildId, kRouteId);
-    context_.set_http_server_properties(&http_server_properties_);
+    context_.set_http_server_properties(http_server_properties_.GetWeakPtr());
   }
 
   virtual ~ResourceSchedulerTest() {

content/shell/shell_url_request_context_getter.cc

@@ -126,7 +126,9 @@ net::URLRequestContext* ShellURLRequestContextGetter::GetURLRequestContext() {
     storage_->set_ssl_config_service(new net::SSLConfigServiceDefaults);
     storage_->set_http_auth_handler_factory(
         net::HttpAuthHandlerFactory::CreateDefault(host_resolver.get()));
-    storage_->set_http_server_properties(new net::HttpServerPropertiesImpl);
+    storage_->set_http_server_properties(
+        scoped_ptr<net::HttpServerProperties>(
+            new net::HttpServerPropertiesImpl()));
 
     base::FilePath cache_path = base_path_.Append(FILE_PATH_LITERAL("Cache"));
     net::HttpCache::DefaultBackend* main_backend =

net/http/http_network_layer_unittest.cc

@@ -42,7 +42,8 @@ class HttpNetworkLayerTest : public PlatformTest {
     session_params.transport_security_state = transport_security_state_.get();
     session_params.proxy_service = proxy_service_.get();
     session_params.ssl_config_service = ssl_config_service_.get();
-    session_params.http_server_properties = &http_server_properties_;
+    session_params.http_server_properties =
+        http_server_properties_.GetWeakPtr();
     network_session_ = new HttpNetworkSession(session_params);
     factory_.reset(new HttpNetworkLayer(network_session_.get()));
   }

net/http/http_network_session.cc

@@ -62,7 +62,6 @@ HttpNetworkSession::Params::Params()
       ssl_config_service(NULL),
       http_auth_handler_factory(NULL),
       network_delegate(NULL),
-      http_server_properties(NULL),
       net_log(NULL),
       host_mapping_rules(NULL),
       force_http_pipelining(false),
@@ -88,6 +87,8 @@ HttpNetworkSession::Params::Params()
       quic_crypto_client_stream_factory(NULL) {
 }
 
+HttpNetworkSession::Params::~Params() {}
+
 // TODO(mbelshe): Move the socket factories into HttpStreamFactory.
 HttpNetworkSession::HttpNetworkSession(const Params& params)
     : net_log_(params.net_log),

net/http/http_network_session.h

@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_export.h"
@@ -54,6 +55,7 @@ class NET_EXPORT HttpNetworkSession
  public:
   struct NET_EXPORT Params {
     Params();
+    ~Params();
 
     ClientSocketFactory* client_socket_factory;
     HostResolver* host_resolver;
@@ -65,7 +67,7 @@ class NET_EXPORT HttpNetworkSession
     SSLConfigService* ssl_config_service;
     HttpAuthHandlerFactory* http_auth_handler_factory;
     NetworkDelegate* network_delegate;
-    HttpServerProperties* http_server_properties;
+    base::WeakPtr<HttpServerProperties> http_server_properties;
     NetLog* net_log;
     HostMappingRules* host_mapping_rules;
     bool force_http_pipelining;
@@ -133,7 +135,7 @@ class NET_EXPORT HttpNetworkSession
   NetworkDelegate* network_delegate() {
     return network_delegate_;
   }
-  HttpServerProperties* http_server_properties() {
+  base::WeakPtr<HttpServerProperties> http_server_properties() {
     return http_server_properties_;
   }
   HttpStreamFactory* http_stream_factory() {
@@ -180,7 +182,7 @@ class NET_EXPORT HttpNetworkSession
 
   NetLog* const net_log_;
   NetworkDelegate* const network_delegate_;
-  HttpServerProperties* const http_server_properties_;
+  const base::WeakPtr<HttpServerProperties> http_server_properties_;
   CertVerifier* const cert_verifier_;
   HttpAuthHandlerFactory* const http_auth_handler_factory_;
   bool force_http_pipelining_;

net/http/http_network_transaction.cc

@@ -66,10 +66,11 @@ namespace net {
 
 namespace {
 
-void ProcessAlternateProtocol(HttpStreamFactory* factory,
-                              HttpServerProperties* http_server_properties,
-                              const HttpResponseHeaders& headers,
-                              const HostPortPair& http_host_port_pair) {
+void ProcessAlternateProtocol(
+    HttpStreamFactory* factory,
+    const base::WeakPtr<HttpServerProperties>& http_server_properties,
+    const HttpResponseHeaders& headers,
+    const HostPortPair& http_host_port_pair) {
   std::string alternate_protocol_str;
 
   if (!headers.EnumerateHeader(NULL, kAlternateProtocolHeader,

net/http/http_network_transaction_ssl_unittest.cc

@@ -74,7 +74,8 @@ class HttpNetworkTransactionSSLTest : public testing::Test {
 
     session_params_.client_socket_factory = &mock_socket_factory_;
     session_params_.host_resolver = &mock_resolver_;
-    session_params_.http_server_properties = &http_server_properties_;
+    session_params_.http_server_properties =
+        http_server_properties_.GetWeakPtr();
     session_params_.transport_security_state = &transport_security_state_;
   }
 

net/http/http_network_transaction_unittest.cc

@@ -7011,7 +7011,7 @@ scoped_refptr<HttpNetworkSession> SetupSessionForGroupNameTests(
     SpdySessionDependencies* session_deps_) {
   scoped_refptr<HttpNetworkSession> session(CreateSession(session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   http_server_properties->SetAlternateProtocol(
       HostPortPair("host.with.alternate", 80), 443,
@@ -8005,7 +8005,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   // Port must be < 1024, or the header will be ignored (since initial port was
   // port 80 (another restricted port).
@@ -8068,7 +8068,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kUnrestrictedAlternatePort = 1024;
   http_server_properties->SetAlternateProtocol(
@@ -8119,7 +8119,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kUnrestrictedAlternatePort = 1024;
   http_server_properties->SetAlternateProtocol(
@@ -8167,7 +8167,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kRestrictedAlternatePort = 80;
   http_server_properties->SetAlternateProtocol(
@@ -8216,7 +8216,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kRestrictedAlternatePort = 80;
   http_server_properties->SetAlternateProtocol(
@@ -8264,7 +8264,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kUnrestrictedAlternatePort = 1024;
   http_server_properties->SetAlternateProtocol(
@@ -8308,7 +8308,7 @@ TEST_P(HttpNetworkTransactionTest,
 
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  HttpServerProperties* http_server_properties =
+  base::WeakPtr<HttpServerProperties> http_server_properties =
       session->http_server_properties();
   const int kUnsafePort = 7;
   http_server_properties->SetAlternateProtocol(

net/http/http_pipelined_host_pool.cc

@@ -33,7 +33,7 @@ class HttpPipelinedHostImplFactory : public HttpPipelinedHost::Factory {
 HttpPipelinedHostPool::HttpPipelinedHostPool(
     Delegate* delegate,
     HttpPipelinedHost::Factory* factory,
-    HttpServerProperties* http_server_properties,
+    const base::WeakPtr<HttpServerProperties>& http_server_properties,
     bool force_pipelining)
     : delegate_(delegate),
       factory_(factory),

net/http/http_pipelined_host_pool.h

@@ -10,6 +10,7 @@
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "net/http/http_pipelined_host.h"
 #include "net/http/http_pipelined_host_capability.h"
 
@@ -37,10 +38,11 @@ class NET_EXPORT_PRIVATE HttpPipelinedHostPool
         HttpPipelinedHost* host) = 0;
   };
 
-  HttpPipelinedHostPool(Delegate* delegate,
-                        HttpPipelinedHost::Factory* factory,
-                        HttpServerProperties* http_server_properties_,
-                        bool force_pipelining);
+  HttpPipelinedHostPool(
+      Delegate* delegate,
+      HttpPipelinedHost::Factory* factory,
+      const base::WeakPtr<HttpServerProperties>& http_server_properties,
+      bool force_pipelining);
   virtual ~HttpPipelinedHostPool();
 
   // Returns true if pipelining might work for |key|. Generally, this returns
@@ -89,7 +91,7 @@ class NET_EXPORT_PRIVATE HttpPipelinedHostPool
   Delegate* delegate_;
   scoped_ptr<HttpPipelinedHost::Factory> factory_;
   HostMap host_map_;
-  HttpServerProperties* http_server_properties_;
+  const base::WeakPtr<HttpServerProperties> http_server_properties_;
   bool force_pipelining_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHostPool);