Reland "Inject NetworkMetadataStore in wifi sync classes which depend on it."

This is a reland of 3014d931
with fix for a use after free of the NetworkMetadataStore held
by wifi sync classes.

Original change's description:
> Inject NetworkMetadataStore in wifi sync classes which depend on it.
>
> The instance of NetworkMetadataStore for a user profile is created
> after the sync service is already initialized.  This change does
> the necessary plumbing to get the metadata store instance to
> the wifi sync classes.
>
> Bug: 966270
> Change-Id: Icd235cd780df97a367f09f281f3e957893baa93b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2110079
> Commit-Queue: Jon Mann <jonmann@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: James Vecore <vecore@google.com>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#752273}

TBR=stevenjb@chromium.org,treib@chromium.org

Bug: 966270
Change-Id: Ib4327a1a953ebee8d49775890eec2e6c353c4e5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2114586
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#753745}

chrome/browser/chromeos/BUILD.gn

@@ -111,6 +111,7 @@ source_set("chromeos") {
     "//chromeos/components/quick_answers/public/cpp:prefs",
     "//chromeos/components/smbfs",
     "//chromeos/components/smbfs/mojom",
+    "//chromeos/components/sync_wifi",
     "//chromeos/components/tether",
     "//chromeos/constants",
     "//chromeos/cryptohome",

chrome/browser/chromeos/net/network_pref_state_observer.cc

@@ -8,7 +8,10 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/wifi_configuration_sync_service_factory.h"
+#include "chromeos/components/sync_wifi/wifi_configuration_sync_service.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_metadata_store.h"
 #include "content/public/browser/notification_service.h"
 
 namespace chromeos {
@@ -40,6 +43,14 @@ void NetworkPrefStateObserver::Observe(
   if (ProfileHelper::IsPrimaryProfile(profile)) {
     InitializeNetworkPrefServices(profile);
     notification_registrar_.RemoveAll();
+
+    auto* wifi_sync_service =
+        WifiConfigurationSyncServiceFactory::GetForProfile(profile,
+                                                           /*create=*/false);
+    if (wifi_sync_service) {
+      wifi_sync_service->SetNetworkMetadataStore(
+          NetworkHandler::Get()->network_metadata_store()->GetWeakPtr());
+    }
   }
 }
 

chrome/browser/sync/chrome_sync_client.cc

@@ -614,7 +614,8 @@ ChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
           ->change_processor()
           ->GetControllerDelegate();
     case syncer::WIFI_CONFIGURATIONS:
-      return WifiConfigurationSyncServiceFactory::GetForProfile(profile_)
+      return WifiConfigurationSyncServiceFactory::GetForProfile(profile_,
+                                                                /*create=*/true)
           ->GetControllerDelegate();
 #endif  // defined(OS_CHROMEOS)
     case syncer::SHARING_MESSAGE:

chrome/browser/sync/wifi_configuration_sync_service_factory.cc

@@ -17,13 +17,14 @@
 
 // static
 chromeos::sync_wifi::WifiConfigurationSyncService*
-WifiConfigurationSyncServiceFactory::GetForProfile(Profile* profile) {
+WifiConfigurationSyncServiceFactory::GetForProfile(Profile* profile,
+                                                   bool create) {
   if (!ShouldRunInProfile(profile)) {
     return nullptr;
   }
 
   return static_cast<chromeos::sync_wifi::WifiConfigurationSyncService*>(
-      GetInstance()->GetServiceForBrowserContext(profile, true));
+      GetInstance()->GetServiceForBrowserContext(profile, create));
 }
 
 // static

chrome/browser/sync/wifi_configuration_sync_service_factory.h

@@ -27,7 +27,8 @@ class WifiConfigurationSyncServiceFactory
     : public BrowserContextKeyedServiceFactory {
  public:
   static chromeos::sync_wifi::WifiConfigurationSyncService* GetForProfile(
-      Profile* profile);
+      Profile* profile,
+      bool create);
   static WifiConfigurationSyncServiceFactory* GetInstance();
   static bool ShouldRunInProfile(const Profile* profile);
 

chromeos/components/sync_wifi/fake_local_network_collector.cc

@@ -40,6 +40,9 @@ void FakeLocalNetworkCollector::ClearNetworks() {
   networks_.clear();
 }
 
+void FakeLocalNetworkCollector::SetNetworkMetadataStore(
+    base::WeakPtr<NetworkMetadataStore> network_metadata_store) {}
+
 }  // namespace sync_wifi
 
-}  // namespace chromeos
+}  // namespace chromeos
\ No newline at end of file

chromeos/components/sync_wifi/fake_local_network_collector.h

@@ -29,6 +29,8 @@ class FakeLocalNetworkCollector : public LocalNetworkCollector {
 
   void AddNetwork(sync_pb::WifiConfigurationSpecifics proto);
   void ClearNetworks();
+  void SetNetworkMetadataStore(
+      base::WeakPtr<NetworkMetadataStore> network_metadata_store) override;
 
  private:
   std::vector<sync_pb::WifiConfigurationSpecifics> networks_;

chromeos/components/sync_wifi/local_network_collector.h

@@ -19,6 +19,8 @@ class WifiConfigurationSpecifics;
 
 namespace chromeos {
 
+class NetworkMetadataStore;
+
 namespace sync_wifi {
 
 class NetworkIdentifier;
@@ -48,6 +50,10 @@ class LocalNetworkCollector {
   // exist or isn't syncable it will provide base::nullopt to the callback.
   virtual void GetSyncableNetwork(const NetworkIdentifier& id,
                                   ProtoCallback callback) = 0;
+
+  // Provides the metadata store which gets constructed later.
+  virtual void SetNetworkMetadataStore(
+      base::WeakPtr<NetworkMetadataStore> network_metadata_store) = 0;
 };
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/local_network_collector_impl.cc

@@ -36,10 +36,8 @@ dbus::ObjectPath GetServicePathForGuid(const std::string& guid) {
 }  // namespace
 
 LocalNetworkCollectorImpl::LocalNetworkCollectorImpl(
-    network_config::mojom::CrosNetworkConfig* cros_network_config,
-    NetworkMetadataStore* network_metadata_store)
-    : cros_network_config_(cros_network_config),
-      network_metadata_store_(network_metadata_store) {
+    network_config::mojom::CrosNetworkConfig* cros_network_config)
+    : cros_network_config_(cros_network_config) {
   cros_network_config_->AddObserver(
       cros_network_config_observer_receiver_.BindNewPipeAndPassRemote());
 
@@ -97,6 +95,11 @@ void LocalNetworkCollectorImpl::GetSyncableNetwork(const NetworkIdentifier& id,
   StartGetNetworkDetails(network, request_guid);
 }
 
+void LocalNetworkCollectorImpl::SetNetworkMetadataStore(
+    base::WeakPtr<NetworkMetadataStore> network_metadata_store) {
+  network_metadata_store_ = network_metadata_store;
+}
+
 std::string LocalNetworkCollectorImpl::InitializeRequest() {
   std::string request_guid = base::GenerateGUID();
   request_guid_to_complete_protos_[request_guid] =

chromeos/components/sync_wifi/local_network_collector_impl.h

@@ -38,9 +38,8 @@ class LocalNetworkCollectorImpl
   // LocalNetworkCollector:
 
   // |cros_network_config| and |network_metadata_store| must outlive this class.
-  LocalNetworkCollectorImpl(
-      network_config::mojom::CrosNetworkConfig* cros_network_config,
-      NetworkMetadataStore* network_metadata_store);
+  explicit LocalNetworkCollectorImpl(
+      network_config::mojom::CrosNetworkConfig* cros_network_config);
   ~LocalNetworkCollectorImpl() override;
 
   // Can only execute one request at a time.
@@ -50,6 +49,9 @@ class LocalNetworkCollectorImpl
   void GetSyncableNetwork(const NetworkIdentifier& id,
                           ProtoCallback callback) override;
 
+  void SetNetworkMetadataStore(
+      base::WeakPtr<NetworkMetadataStore> network_metadata_store) override;
+
   // CrosNetworkConfigObserver:
   void OnNetworkStateListChanged() override;
   void OnActiveNetworksChanged(
@@ -100,7 +102,7 @@ class LocalNetworkCollectorImpl
   mojo::Receiver<chromeos::network_config::mojom::CrosNetworkConfigObserver>
       cros_network_config_observer_receiver_{this};
   std::vector<network_config::mojom::NetworkStatePropertiesPtr> mojo_networks_;
-  NetworkMetadataStore* network_metadata_store_;
+  base::WeakPtr<NetworkMetadataStore> network_metadata_store_;
 
   base::flat_map<std::string, std::vector<sync_pb::WifiConfigurationSpecifics>>
       request_guid_to_complete_protos_;

chromeos/components/sync_wifi/local_network_collector_impl_unittest.cc

@@ -19,6 +19,7 @@
 #include "chromeos/components/sync_wifi/test_data_generator.h"
 #include "chromeos/dbus/shill/fake_shill_simulated_result.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_metadata_store.h"
 #include "chromeos/services/network_config/cros_network_config.h"
 #include "chromeos/services/network_config/in_process_instance.h"
 #include "chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h"
@@ -58,8 +59,9 @@ class LocalNetworkCollectorImplTest : public testing::Test {
     testing::Test::SetUp();
     helper()->SetUp();
     local_network_collector_ = std::make_unique<LocalNetworkCollectorImpl>(
-        remote_cros_network_config_.get(),
-        NetworkHandler::Get()->network_metadata_store());
+        remote_cros_network_config_.get());
+    local_network_collector_->SetNetworkMetadataStore(
+        NetworkHandler::Get()->network_metadata_store()->GetWeakPtr());
   }
 
   void TearDown() override {

chromeos/components/sync_wifi/wifi_configuration_bridge.cc

@@ -15,6 +15,7 @@
 #include "base/time/time.h"
 #include "chromeos/components/sync_wifi/network_identifier.h"
 #include "chromeos/components/sync_wifi/synced_network_updater.h"
+#include "chromeos/network/network_metadata_store.h"
 #include "components/device_event_log/device_event_log.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/model/metadata_batch.h"
@@ -45,14 +46,19 @@ WifiConfigurationBridge::WifiConfigurationBridge(
     syncer::OnceModelTypeStoreFactory create_store_callback)
     : ModelTypeSyncBridge(std::move(change_processor)),
       synced_network_updater_(synced_network_updater),
-      local_network_collector_(local_network_collector) {
+      local_network_collector_(local_network_collector),
+      network_metadata_store_(nullptr) {
   std::move(create_store_callback)
       .Run(syncer::WIFI_CONFIGURATIONS,
            base::BindOnce(&WifiConfigurationBridge::OnStoreCreated,
                           weak_ptr_factory_.GetWeakPtr()));
 }
 
-WifiConfigurationBridge::~WifiConfigurationBridge() {}
+WifiConfigurationBridge::~WifiConfigurationBridge() {
+  if (network_metadata_store_) {
+    network_metadata_store_->RemoveObserver(this);
+  }
+}
 
 std::unique_ptr<syncer::MetadataChangeList>
 WifiConfigurationBridge::CreateMetadataChangeList() {
@@ -294,6 +300,20 @@ std::vector<NetworkIdentifier> WifiConfigurationBridge::GetAllIdsForTesting() {
   return ids;
 }
 
+void WifiConfigurationBridge::OnFirstConnectionToNetwork(
+    const std::string& guid) {
+  // TODO(jonmann): Add network to sync.
+}
+
+void WifiConfigurationBridge::SetNetworkMetadataStore(
+    base::WeakPtr<NetworkMetadataStore> network_metadata_store) {
+  if (network_metadata_store_) {
+    network_metadata_store->RemoveObserver(this);
+  }
+  network_metadata_store_ = network_metadata_store;
+  network_metadata_store->AddObserver(this);
+}
+
 }  // namespace sync_wifi
 
 }  // namespace chromeos

chromeos/components/sync_wifi/wifi_configuration_bridge.h

@@ -16,6 +16,7 @@
 #include "base/time/time.h"
 #include "chromeos/components/sync_wifi/local_network_collector.h"
 #include "chromeos/components/sync_wifi/synced_network_updater.h"
+#include "chromeos/network/network_metadata_observer.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/model/model_type_store.h"
 #include "components/sync/model/model_type_sync_bridge.h"
@@ -26,11 +27,14 @@ class ModelTypeChangeProcessor;
 
 namespace chromeos {
 
+class NetworkMetadataStore;
+
 namespace sync_wifi {
 
 // Receives updates to network configurations from the Chrome sync back end and
 // from the system network stack and keeps both lists in sync.
-class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge {
+class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge,
+                                public NetworkMetadataObserver {
  public:
   WifiConfigurationBridge(
       SyncedNetworkUpdater* synced_network_updater,
@@ -53,9 +57,15 @@ class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge {
   std::string GetClientTag(const syncer::EntityData& entity_data) override;
   std::string GetStorageKey(const syncer::EntityData& entity_data) override;
 
+  // NetworkMetadataObserver:
+  void OnFirstConnectionToNetwork(const std::string& guid) override;
+
   // Comes from |entries_| the in-memory map.
   std::vector<NetworkIdentifier> GetAllIdsForTesting();
 
+  void SetNetworkMetadataStore(
+      base::WeakPtr<NetworkMetadataStore> network_metadata_store);
+
  private:
   void Commit(std::unique_ptr<syncer::ModelTypeStore::WriteBatch> batch);
 
@@ -86,8 +96,8 @@ class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge {
   std::unique_ptr<syncer::ModelTypeStore> store_;
 
   SyncedNetworkUpdater* synced_network_updater_;
-
   LocalNetworkCollector* local_network_collector_;
+  base::WeakPtr<NetworkMetadataStore> network_metadata_store_;
 
   base::WeakPtrFactory<WifiConfigurationBridge> weak_ptr_factory_{this};
 

chromeos/components/sync_wifi/wifi_configuration_sync_service.cc

@@ -15,6 +15,7 @@
 #include "chromeos/components/sync_wifi/timer_factory.h"
 #include "chromeos/components/sync_wifi/wifi_configuration_bridge.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_metadata_store.h"
 #include "chromeos/services/network_config/public/mojom/cros_network_config.mojom.h"
 #include "components/sync/base/report_unrecoverable_error.h"
 #include "components/sync/model/model_type_store.h"
@@ -34,8 +35,7 @@ WifiConfigurationSyncService::WifiConfigurationSyncService(
       std::make_unique<PendingNetworkConfigurationTrackerImpl>(pref_service),
       remote_cros_network_config_.get(), std::make_unique<TimerFactory>());
   collector_ = std::make_unique<LocalNetworkCollectorImpl>(
-      remote_cros_network_config_.get(),
-      NetworkHandler::Get()->network_metadata_store());
+      remote_cros_network_config_.get());
   bridge_ = std::make_unique<sync_wifi::WifiConfigurationBridge>(
       updater_.get(), collector_.get(),
       std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
@@ -51,6 +51,12 @@ WifiConfigurationSyncService::GetControllerDelegate() {
   return bridge_->change_processor()->GetControllerDelegate();
 }
 
+void WifiConfigurationSyncService::SetNetworkMetadataStore(
+    base::WeakPtr<NetworkMetadataStore> network_metadata_store) {
+  bridge_->SetNetworkMetadataStore(network_metadata_store);
+  collector_->SetNetworkMetadataStore(network_metadata_store);
+}
+
 }  // namespace sync_wifi
 
 }  // namespace chromeos

chromeos/components/sync_wifi/wifi_configuration_sync_service.h

@@ -23,6 +23,8 @@ class ModelTypeControllerDelegate;
 
 namespace chromeos {
 
+class NetworkMetadataStore;
+
 namespace sync_wifi {
 
 class LocalNetworkCollectorImpl;
@@ -40,6 +42,8 @@ class WifiConfigurationSyncService : public KeyedService {
   ~WifiConfigurationSyncService() override;
 
   base::WeakPtr<syncer::ModelTypeControllerDelegate> GetControllerDelegate();
+  void SetNetworkMetadataStore(
+      base::WeakPtr<NetworkMetadataStore> network_metadata_store);
 
  private:
   std::unique_ptr<WifiConfigurationBridge> bridge_;

chromeos/network/network_metadata_store.h

@@ -72,6 +72,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkMetadataStore
   void AddObserver(NetworkMetadataObserver* observer);
   void RemoveObserver(NetworkMetadataObserver* observer);
 
+  base::WeakPtr<NetworkMetadataStore> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
  private:
   void RemoveNetworkFromPref(const std::string& network_guid,
                              PrefService* pref_service);
@@ -88,6 +92,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkMetadataStore
   NetworkStateHandler* network_state_handler_;
   PrefService* profile_pref_service_;
   PrefService* device_pref_service_;
+  base::WeakPtrFactory<NetworkMetadataStore> weak_ptr_factory_{this};
 };
 
 }  // namespace chromeos