Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org

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

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

19 files changed, 79 insertions, 28 deletions

diff --git a/chrome/browser/chromeos/login/screens/network_screen.cc b/chrome/browser/chromeos/login/screens/network_screen.cc
index 4f8c808..b10458f 100644
--- a/chrome/browser/chromeos/login/screens/network_screen.cc
+++ b/chrome/browser/chromeos/login/screens/network_screen.cc
@@ -118,6 +118,9 @@ void NetworkScreen::OnViewDestroyed(NetworkView* view) {
   if (view_ == view) {
     view_ = nullptr;
     timezone_subscription_.reset();
+    // Ownership of NetworkScreen is complicated; ensure that we remove
+    // this as a NetworkStateHandler observer when the view is destroyed.
+    UnsubscribeNetworkNotification();
   }
 }
 
diff --git a/chrome/browser/chromeos/mobile/mobile_activator_unittest.cc b/chrome/browser/chromeos/mobile/mobile_activator_unittest.cc
index 7135da7..5205b4b 100644
--- a/chrome/browser/chromeos/mobile/mobile_activator_unittest.cc
+++ b/chrome/browser/chromeos/mobile/mobile_activator_unittest.cc
@@ -122,6 +122,7 @@ class MobileActivatorTest : public testing::Test {
     NetworkHandler::Initialize();
   }
   void TearDown() override {
+    mobile_activator_.TerminateActivation();
     NetworkHandler::Shutdown();
     DBusThreadManager::Shutdown();
   }
diff --git a/chrome/browser/chromeos/proxy_config_service_impl.cc b/chrome/browser/chromeos/proxy_config_service_impl.cc
index 4e63069..6e07ba7 100644
--- a/chrome/browser/chromeos/proxy_config_service_impl.cc
+++ b/chrome/browser/chromeos/proxy_config_service_impl.cc
@@ -117,6 +117,13 @@ void ProxyConfigServiceImpl::DefaultNetworkChanged(
   DetermineEffectiveConfigFromDefaultNetwork();
 }
 
+void ProxyConfigServiceImpl::OnShuttingDown() {
+  // Ownership of this class is complicated. Stop observing NetworkStateHandler
+  // when the class shuts down.
+  NetworkHandler::Get()->network_state_handler()->RemoveObserver(this,
+                                                                 FROM_HERE);
+}
+
 // static
 bool ProxyConfigServiceImpl::IgnoreProxy(const PrefService* profile_prefs,
                                          const std::string network_profile_path,
@@ -167,6 +174,9 @@ bool ProxyConfigServiceImpl::IgnoreProxy(const PrefService* profile_prefs,
 }
 
 void ProxyConfigServiceImpl::DetermineEffectiveConfigFromDefaultNetwork() {
+  if (!NetworkHandler::IsInitialized())
+    return;
+
   NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
   const NetworkState* network = handler->DefaultNetwork();
 
diff --git a/chrome/browser/chromeos/proxy_config_service_impl.h b/chrome/browser/chromeos/proxy_config_service_impl.h
index 3276fc6..8a1fc6a 100644
--- a/chrome/browser/chromeos/proxy_config_service_impl.h
+++ b/chrome/browser/chromeos/proxy_config_service_impl.h
@@ -52,6 +52,7 @@ class ProxyConfigServiceImpl : public PrefProxyConfigTrackerImpl,
 
   // NetworkStateHandlerObserver implementation.
   void DefaultNetworkChanged(const NetworkState* network) override;
+  void OnShuttingDown() override;
 
  protected:
   friend class UIProxyConfigService;
diff --git a/chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc b/chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc
index 01e9c51..b45809c 100644
--- a/chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc
+++ b/chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc
@@ -118,6 +118,17 @@ class VpnProviderApiTest : public ExtensionApiTest,
   VpnProviderApiTest() {}
   ~VpnProviderApiTest() override {}
 
+  void SetUpOnMainThread() override {
+    ExtensionApiTest::SetUpOnMainThread();
+    NetworkHandler::Get()->network_configuration_handler()->AddObserver(this);
+  }
+
+  void TearDownOnMainThread() override {
+    ExtensionApiTest::TearDownOnMainThread();
+    NetworkHandler::Get()->network_configuration_handler()->RemoveObserver(
+        this);
+  }
+
   void SetUpInProcessBrowserTestFixture() override {
     ExtensionApiTest::SetUpInProcessBrowserTestFixture();
     test_client_ = new TestShillThirdPartyVpnDriverClient();
@@ -135,7 +146,6 @@ class VpnProviderApiTest : public ExtensionApiTest,
   }
 
   void LoadVpnExtension() {
-    NetworkHandler::Get()->network_configuration_handler()->AddObserver(this);
     extension_ = LoadExtension(test_data_dir_.AppendASCII("vpn_provider"));
     extension_id_ = extension_->id();
     service_ = VpnServiceFactory::GetForBrowserContext(profile());
diff --git a/chromeos/network/auto_connect_handler.cc b/chromeos/network/auto_connect_handler.cc
index b444789..4411b18 100644
--- a/chromeos/network/auto_connect_handler.cc
+++ b/chromeos/network/auto_connect_handler.cc
@@ -37,10 +37,12 @@ AutoConnectHandler::AutoConnectHandler()
 }
 
 AutoConnectHandler::~AutoConnectHandler() {
-  if (client_cert_resolver_)
-    client_cert_resolver_->RemoveObserver(this);
   if (LoginState::IsInitialized())
     LoginState::Get()->RemoveObserver(this);
+  if (client_cert_resolver_)
+    client_cert_resolver_->RemoveObserver(this);
+  if (network_connection_handler_)
+    network_connection_handler_->RemoveObserver(this);
   if (network_state_handler_)
     network_state_handler_->RemoveObserver(this, FROM_HERE);
   if (managed_configuration_handler_)
diff --git a/chromeos/network/client_cert_resolver.h b/chromeos/network/client_cert_resolver.h
index 91d7fb5..bc2af20 100644
--- a/chromeos/network/client_cert_resolver.h
+++ b/chromeos/network/client_cert_resolver.h
@@ -122,7 +122,7 @@ class CHROMEOS_EXPORT ClientCertResolver : public NetworkStateHandlerObserver,
   // instead.
   base::Time Now() const;
 
-  base::ObserverList<Observer> observers_;
+  base::ObserverList<Observer, true> observers_;
 
   // The set of networks that were checked/resolved in previous passes. These
   // networks are skipped in the NetworkListChanged notification.
diff --git a/chromeos/network/host_resolver_impl_chromeos.cc b/chromeos/network/host_resolver_impl_chromeos.cc
index ce0cd18..61a9da9 100644
--- a/chromeos/network/host_resolver_impl_chromeos.cc
+++ b/chromeos/network/host_resolver_impl_chromeos.cc
@@ -26,7 +26,7 @@ namespace chromeos {
 //
 // An instance of this class is created on the NetworkHandler (UI) thread and
 // manages its own lifetime, destroying itself when NetworkStateHandlerObserver
-// ::IsShuttingDown() gets called.
+// ::OnShuttingDown() gets called.
 
 class HostResolverImplChromeOS::NetworkObserver
     : public chromeos::NetworkStateHandlerObserver {
@@ -96,7 +96,7 @@ class HostResolverImplChromeOS::NetworkObserver
     CallResolverSetIpAddress(ipv4_address, ipv6_address);
   }
 
-  void IsShuttingDown() override { delete this; }
+  void OnShuttingDown() override { delete this; }
 
   void CallResolverSetIpAddress(const std::string& ipv4_address,
                                 const std::string& ipv6_address) {
diff --git a/chromeos/network/managed_network_configuration_handler_impl.h b/chromeos/network/managed_network_configuration_handler_impl.h
index 76d6c2e..a6337f3 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.h
+++ b/chromeos/network/managed_network_configuration_handler_impl.h
@@ -215,7 +215,7 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl
   // associated set of GUIDs is empty.
   UserToModifiedPoliciesMap queued_modified_policies_;
 
-  base::ObserverList<NetworkPolicyObserver> observers_;
+  base::ObserverList<NetworkPolicyObserver, true> observers_;
 
   // For Shill client callbacks
   base::WeakPtrFactory<ManagedNetworkConfigurationHandlerImpl>
diff --git a/chromeos/network/network_configuration_handler.h b/chromeos/network/network_configuration_handler.h
index 6b0bbbd..d76be19 100644
--- a/chromeos/network/network_configuration_handler.h
+++ b/chromeos/network/network_configuration_handler.h
@@ -205,7 +205,7 @@ class CHROMEOS_EXPORT NetworkConfigurationHandler
   // Map of in-progress deleter instances. Owned by this class.
   std::map<std::string, ProfileEntryDeleter*> profile_entry_deleters_;
 
-  base::ObserverList<NetworkConfigurationObserver> observers_;
+  base::ObserverList<NetworkConfigurationObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkConfigurationHandler);
 };
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index f2ff1a0..fc38ce3 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -233,7 +233,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   void HandleShillDisconnectSuccess(const std::string& service_path,
                                     const base::Closure& success_callback);
 
-  base::ObserverList<NetworkConnectionObserver> observers_;
+  base::ObserverList<NetworkConnectionObserver, true> observers_;
 
   // Local references to the associated handler instances.
   CertLoader* cert_loader_;
diff --git a/chromeos/network/network_handler.cc b/chromeos/network/network_handler.cc
index b10bad7..05f7f0c 100644
--- a/chromeos/network/network_handler.cc
+++ b/chromeos/network/network_handler.cc
@@ -50,6 +50,7 @@ NetworkHandler::NetworkHandler()
 }
 
 NetworkHandler::~NetworkHandler() {
+  network_state_handler_->Shutdown();
 }
 
 void NetworkHandler::Init() {
diff --git a/chromeos/network/network_profile_handler.h b/chromeos/network/network_profile_handler.h
index 5015dd2..f7c70c7 100644
--- a/chromeos/network/network_profile_handler.h
+++ b/chromeos/network/network_profile_handler.h
@@ -81,7 +81,7 @@ class CHROMEOS_EXPORT NetworkProfileHandler
   // properties are retrieved and the path is still in this set, a new profile
   // object is created.
   std::set<std::string> pending_profile_creations_;
-  base::ObserverList<NetworkProfileObserver> observers_;
+  base::ObserverList<NetworkProfileObserver, true> observers_;
 
   // For Shill client callbacks
   base::WeakPtrFactory<NetworkProfileHandler> weak_ptr_factory_;
diff --git a/chromeos/network/network_sms_handler.h b/chromeos/network/network_sms_handler.h
index eb8ed88..1e987fc 100644
--- a/chromeos/network/network_sms_handler.h
+++ b/chromeos/network/network_sms_handler.h
@@ -90,7 +90,7 @@ class CHROMEOS_EXPORT NetworkSmsHandler : public ShillPropertyChangedObserver {
                                 DBusMethodCallStatus call_status,
                                 const base::DictionaryValue& properties);
 
-  base::ObserverList<Observer> observers_;
+  base::ObserverList<Observer, true> observers_;
   ScopedVector<NetworkSmsDeviceHandler> device_handlers_;
   ScopedVector<base::DictionaryValue> received_messages_;
   base::WeakPtrFactory<NetworkSmsHandler> weak_ptr_factory_;
@@ -98,6 +98,6 @@ class CHROMEOS_EXPORT NetworkSmsHandler : public ShillPropertyChangedObserver {
   DISALLOW_COPY_AND_ASSIGN(NetworkSmsHandler);
 };
 
-}  // namespace
+}  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_NETWORK_SMS_HANDLER_H_
diff --git a/chromeos/network/network_sms_handler_unittest.cc b/chromeos/network/network_sms_handler_unittest.cc
index 13b3b58..9ee2b0a 100644
--- a/chromeos/network/network_sms_handler_unittest.cc
+++ b/chromeos/network/network_sms_handler_unittest.cc
@@ -81,6 +81,7 @@ class NetworkSmsHandlerTest : public testing::Test {
   }
 
   void TearDown() override {
+    network_sms_handler_->RemoveObserver(test_observer_.get());
     network_sms_handler_.reset();
     DBusThreadManager::Shutdown();
   }
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index 5f4738a..1d521d9 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -67,15 +67,24 @@ std::string ValueAsString(const base::Value& value) {
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
     "ethernet,wifi,cellular";
 
-NetworkStateHandler::NetworkStateHandler() : network_list_sorted_(false) {
-}
+NetworkStateHandler::NetworkStateHandler() {}
 
 NetworkStateHandler::~NetworkStateHandler() {
-  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, IsShuttingDown());
+  // Normally Shutdown() will get called in ~NetworkHandler, however unit
+  // tests do not use that class so this needs to call Shutdown when we
+  // destry the class.
+  if (!did_shutdown_)
+    Shutdown();
   STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
   STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
+void NetworkStateHandler::Shutdown() {
+  DCHECK(!did_shutdown_);
+  did_shutdown_ = true;
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, OnShuttingDown());
+}
+
 void NetworkStateHandler::InitShillPropertyHandler() {
   shill_property_handler_.reset(new internal::ShillPropertyHandler(this));
   shill_property_handler_->Init();
@@ -92,20 +101,27 @@ void NetworkStateHandler::AddObserver(
     NetworkStateHandlerObserver* observer,
     const tracked_objects::Location& from_here) {
   observers_.AddObserver(observer);
-  device_event_log::AddEntry(from_here.file_name(), from_here.line_number(),
-                             device_event_log::LOG_TYPE_NETWORK,
-                             device_event_log::LOG_LEVEL_DEBUG,
-                             "NetworkStateHandler::AddObserver");
+  device_event_log::AddEntry(
+      from_here.file_name(), from_here.line_number(),
+      device_event_log::LOG_TYPE_NETWORK, device_event_log::LOG_LEVEL_DEBUG,
+      base::StringPrintf("NetworkStateHandler::AddObserver: 0x%p", observer));
+
+  LOG(ERROR) << "ADD Observer: " << from_here.file_name() << ":"
+             << from_here.line_number() << ": " << observer;
 }
 
 void NetworkStateHandler::RemoveObserver(
     NetworkStateHandlerObserver* observer,
     const tracked_objects::Location& from_here) {
   observers_.RemoveObserver(observer);
-  device_event_log::AddEntry(from_here.file_name(), from_here.line_number(),
-                             device_event_log::LOG_TYPE_NETWORK,
-                             device_event_log::LOG_LEVEL_DEBUG,
-                             "NetworkStateHandler::RemoveObserver");
+  device_event_log::AddEntry(
+      from_here.file_name(), from_here.line_number(),
+      device_event_log::LOG_TYPE_NETWORK, device_event_log::LOG_LEVEL_DEBUG,
+      base::StringPrintf("NetworkStateHandler::RemoveObserver: 0x%p",
+                         observer));
+
+  LOG(ERROR) << "REM Observer: " << from_here.file_name() << ":"
+             << from_here.line_number() << ": " << observer;
 }
 
 NetworkStateHandler::TechnologyState NetworkStateHandler::GetTechnologyState(
diff --git a/chromeos/network/network_state_handler.h b/chromeos/network/network_state_handler.h
index 2477c1a..90b5105 100644
--- a/chromeos/network/network_state_handler.h
+++ b/chromeos/network/network_state_handler.h
@@ -81,6 +81,10 @@ class CHROMEOS_EXPORT NetworkStateHandler
 
   ~NetworkStateHandler() override;
 
+  // Called just before destruction to give observers a chance to remove
+  // themselves and disable any networking.
+  void Shutdown();
+
   // Add/remove observers.
   void AddObserver(NetworkStateHandlerObserver* observer,
                    const tracked_objects::Location& from_here);
@@ -374,14 +378,14 @@ class CHROMEOS_EXPORT NetworkStateHandler
   scoped_ptr<internal::ShillPropertyHandler> shill_property_handler_;
 
   // Observer list
-  base::ObserverList<NetworkStateHandlerObserver> observers_;
+  base::ObserverList<NetworkStateHandlerObserver, true> observers_;
 
   // List of managed network states
   ManagedStateList network_list_;
 
   // Set to true when the network list is sorted, cleared when network updates
   // arrive. Used to trigger sorting when needed.
-  bool network_list_sorted_;
+  bool network_list_sorted_ = false;
 
   // List of managed device states
   ManagedStateList device_list_;
@@ -396,6 +400,9 @@ class CHROMEOS_EXPORT NetworkStateHandler
   // NetworkState that is not saved in a profile.
   SpecifierGuidMap specifier_guid_map_;
 
+  // Ensure that Shutdown() gets called exactly once.
+  bool did_shutdown_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);
 };
 
diff --git a/chromeos/network/network_state_handler_observer.cc b/chromeos/network/network_state_handler_observer.cc
index 56ab375..971439d 100644
--- a/chromeos/network/network_state_handler_observer.cc
+++ b/chromeos/network/network_state_handler_observer.cc
@@ -37,7 +37,6 @@ void NetworkStateHandlerObserver::DevicePropertiesUpdated(
 void NetworkStateHandlerObserver::ScanCompleted(const DeviceState* device) {
 }
 
-void NetworkStateHandlerObserver::IsShuttingDown() {
-}
+void NetworkStateHandlerObserver::OnShuttingDown() {}
 
 }  // namespace chromeos
diff --git a/chromeos/network/network_state_handler_observer.h b/chromeos/network/network_state_handler_observer.h
index a25dc59..b1a7ae6 100644
--- a/chromeos/network/network_state_handler_observer.h
+++ b/chromeos/network/network_state_handler_observer.h
@@ -53,7 +53,7 @@ class CHROMEOS_EXPORT NetworkStateHandlerObserver {
 
   // Called just before NetworkStateHandler is destroyed so that observers
   // can safely stop observing.
-  virtual void IsShuttingDown();
+  virtual void OnShuttingDown();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandlerObserver);