Wrap ARC_PACKAGE in pseudo-USS codepath

The sync datatype is the last leftover that hasn't been wrapped in the
USS architecture, and could end up in the critical path to future
dismantling efforts.

The new controller has been simplified by instead introducing the
notion of model-readiness in SyncableService, which the USS codepath
honors. This makes the new controller a lot simpler, and is aligned
with our plans to avoid the notion of model readiness in controllers,
and narrow down the scope of DataTypeController::ReadyForStart() to
non-transient business logic (i.e. is a datatype allowed to start).

Sync integration tests are now parameterized to exercise both codepaths
in case the feature toggle must be used in the future as kill switch.

Bug: 870624,939329
Change-Id: I9d415eba7bd53f0fd9cf7bbfa00db98b7e916e1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1507657Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638630}

chrome/browser/sync/chrome_sync_client.cc

@@ -125,6 +125,7 @@
 #include "chrome/browser/chromeos/printing/synced_printers_manager.h"
 #include "chrome/browser/chromeos/printing/synced_printers_manager_factory.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h"
+#include "chrome/browser/ui/app_list/arc/arc_package_sync_model_type_controller.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_syncable_service.h"
 #include "components/arc/arc_util.h"
 #endif  // defined(OS_CHROMEOS)
@@ -448,8 +449,16 @@ ChromeSyncClient::CreateDataTypeControllers(syncer::SyncService* sync_service) {
 #if defined(OS_CHROMEOS)
   if (arc::IsArcAllowedForProfile(profile_) &&
       !arc::IsArcAppSyncFlowDisabled()) {
-    controllers.push_back(std::make_unique<ArcPackageSyncDataTypeController>(
-        syncer::ARC_PACKAGE, dump_stack, sync_service, this, profile_));
+    if (base::FeatureList::IsEnabled(switches::kSyncPseudoUSSArcPackage)) {
+      controllers.push_back(std::make_unique<ArcPackageSyncModelTypeController>(
+          GetModelTypeStoreService()->GetStoreFactory(),
+          base::BindOnce(&ChromeSyncClient::GetSyncableServiceForType,
+                         base::Unretained(this), syncer::ARC_PACKAGE),
+          dump_stack, sync_service, profile_));
+    } else {
+      controllers.push_back(std::make_unique<ArcPackageSyncDataTypeController>(
+          syncer::ARC_PACKAGE, dump_stack, sync_service, this, profile_));
+    }
   }
 #endif  // defined(OS_CHROMEOS)
 

chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc

@@ -3,10 +3,12 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/sync_arc_package_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_syncable_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
 
 namespace arc {
 
@@ -19,9 +21,11 @@ bool AllProfilesHaveSameArcPackageDetails() {
 
 }  // namespace
 
-class SingleClientArcPackageSyncTest : public SyncTest {
+class SingleClientArcPackageSyncTest : public FeatureToggler, public SyncTest {
  public:
-  SingleClientArcPackageSyncTest() : SyncTest(SINGLE_CLIENT) {}
+  SingleClientArcPackageSyncTest()
+      : FeatureToggler(switches::kSyncPseudoUSSArcPackage),
+        SyncTest(SINGLE_CLIENT) {}
 
   ~SingleClientArcPackageSyncTest() override {}
 
@@ -29,13 +33,13 @@ class SingleClientArcPackageSyncTest : public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(SingleClientArcPackageSyncTest);
 };
 
-IN_PROC_BROWSER_TEST_F(SingleClientArcPackageSyncTest, ArcPackageEmpty) {
+IN_PROC_BROWSER_TEST_P(SingleClientArcPackageSyncTest, ArcPackageEmpty) {
   ASSERT_TRUE(SetupSync());
 
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 }
 
-IN_PROC_BROWSER_TEST_F(SingleClientArcPackageSyncTest,
+IN_PROC_BROWSER_TEST_P(SingleClientArcPackageSyncTest,
                        ArcPackageInstallSomePackages) {
   ASSERT_TRUE(SetupSync());
 
@@ -49,4 +53,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientArcPackageSyncTest,
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 }
 
+INSTANTIATE_TEST_SUITE_P(USS,
+                         SingleClientArcPackageSyncTest,
+                         ::testing::Values(false, true));
+
 }  // namespace arc

chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc

@@ -3,9 +3,11 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/sync_arc_package_helper.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "components/sync/driver/sync_driver_switches.h"
 
 namespace arc {
 
@@ -18,9 +20,13 @@ bool AllProfilesHaveSameArcPackageDetails() {
 
 }  // namespace
 
-class TwoClientArcPackageSyncTest : public SyncTest {
+class TwoClientArcPackageSyncTest : public FeatureToggler, public SyncTest {
  public:
-  TwoClientArcPackageSyncTest() : SyncTest(TWO_CLIENT) { DisableVerifier(); }
+  TwoClientArcPackageSyncTest()
+      : FeatureToggler(switches::kSyncPseudoUSSArcPackage),
+        SyncTest(TWO_CLIENT) {
+    DisableVerifier();
+  }
 
   ~TwoClientArcPackageSyncTest() override {}
 
@@ -28,13 +34,13 @@ class TwoClientArcPackageSyncTest : public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(TwoClientArcPackageSyncTest);
 };
 
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, StartWithNoPackages) {
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest, StartWithNoPackages) {
   ASSERT_TRUE(SetupSync());
 
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, StartWithSamePackages) {
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest, StartWithSamePackages) {
   ASSERT_TRUE(SetupClients());
 
   constexpr size_t kNumPackages = 5;
@@ -50,7 +56,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, StartWithSamePackages) {
 
 // In this test, packages are installed before sync started. Client1 will have
 // package 0 to 4 installed while client2 has no package installed.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest,
                        OneClientHasPackagesAnotherHasNone) {
   ASSERT_TRUE(SetupClients());
 
@@ -68,7 +74,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
 
 // In this test, packages are installed before sync started. Client1 will have
 // package 0 to 9 installed and client2 will have package 0 to 4 installed.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest,
                        OneClientHasPackagesAnotherHasSubSet) {
   ASSERT_TRUE(SetupClients());
 
@@ -91,7 +97,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
 
 // In this test, packages are installed before sync started. Client1 will have
 // package 0 to 4 installed and client2 will have package 1 to 5 installed.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest,
                        StartWithDifferentPackages) {
   ASSERT_TRUE(SetupClients());
 
@@ -111,7 +117,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest,
 }
 
 // Tests package installaton after sync started.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, Install) {
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest, Install) {
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 
@@ -122,7 +128,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, Install) {
 
 // In this test, packages are installed after sync started. Client1 installs
 // package 0 to 4 and client2 installs package 3 to 7.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, InstallDifferent) {
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest, InstallDifferent) {
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 
@@ -140,7 +146,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, InstallDifferent) {
 
 // Installs package from one client and uninstalls from another after sync
 // started.
-IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, Uninstall) {
+IN_PROC_BROWSER_TEST_P(TwoClientArcPackageSyncTest, Uninstall) {
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(AllProfilesHaveSameArcPackageDetails());
 
@@ -154,4 +160,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientArcPackageSyncTest, Uninstall) {
   EXPECT_TRUE(AllProfilesHaveSameArcPackageDetails());
 }
 
+INSTANTIATE_TEST_SUITE_P(USS,
+                         TwoClientArcPackageSyncTest,
+                         ::testing::Values(false, true));
+
 }  // namespace arc

chrome/browser/ui/BUILD.gn

@@ -3265,6 +3265,8 @@ jumbo_split_static_library("ui") {
         "app_list/arc/arc_fast_app_reinstall_starter.h",
         "app_list/arc/arc_package_sync_data_type_controller.cc",
         "app_list/arc/arc_package_sync_data_type_controller.h",
+        "app_list/arc/arc_package_sync_model_type_controller.cc",
+        "app_list/arc/arc_package_sync_model_type_controller.h",
         "app_list/arc/arc_package_syncable_service.cc",
         "app_list/arc/arc_package_syncable_service.h",
         "app_list/arc/arc_package_syncable_service_factory.cc",

chrome/browser/ui/app_list/arc/arc_package_sync_model_type_controller.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/app_list/arc/arc_package_sync_model_type_controller.h"
+
+#include <utility>
+
+#include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/sync/driver/sync_service.h"
+
+ArcPackageSyncModelTypeController::ArcPackageSyncModelTypeController(
+    syncer::OnceModelTypeStoreFactory store_factory,
+    SyncableServiceProvider syncable_service_provider,
+    const base::RepeatingClosure& dump_stack,
+    syncer::SyncService* sync_service,
+    Profile* profile)
+    : syncer::SyncableServiceBasedModelTypeController(
+          syncer::ARC_PACKAGE,
+          std::move(store_factory),
+          std::move(syncable_service_provider),
+          dump_stack),
+      sync_service_(sync_service),
+      profile_(profile) {
+  arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get();
+  if (arc_session_manager) {
+    arc_session_manager->AddObserver(this);
+  }
+}
+
+ArcPackageSyncModelTypeController::~ArcPackageSyncModelTypeController() {
+  arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get();
+  if (arc_session_manager) {
+    arc_session_manager->RemoveObserver(this);
+  }
+}
+
+bool ArcPackageSyncModelTypeController::ReadyForStart() const {
+  DCHECK(CalledOnValidThread());
+  return arc::IsArcPlayStoreEnabledForProfile(profile_);
+}
+
+void ArcPackageSyncModelTypeController::OnArcPlayStoreEnabledChanged(
+    bool enabled) {
+  DCHECK(CalledOnValidThread());
+  sync_service_->ReadyForStartChanged(type());
+}
+
+void ArcPackageSyncModelTypeController::OnArcInitialStart() {
+  DCHECK(CalledOnValidThread());
+  sync_service_->ReadyForStartChanged(type());
+}

chrome/browser/ui/app_list/arc/arc_package_sync_model_type_controller.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_APP_LIST_ARC_ARC_PACKAGE_SYNC_MODEL_TYPE_CONTROLLER_H_
+#define CHROME_BROWSER_UI_APP_LIST_ARC_ARC_PACKAGE_SYNC_MODEL_TYPE_CONTROLLER_H_
+
+#include "base/macros.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager.h"
+#include "components/sync/driver/syncable_service_based_model_type_controller.h"
+
+class Profile;
+
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
+// A DataTypeController for arc package sync datatypes, which enables or
+// disables these types based on whether ArcAppInstance is ready.
+class ArcPackageSyncModelTypeController
+    : public syncer::SyncableServiceBasedModelTypeController,
+      public arc::ArcSessionManager::Observer {
+ public:
+  // |dump_stack| is called when an unrecoverable error occurs.
+  ArcPackageSyncModelTypeController(
+      syncer::OnceModelTypeStoreFactory store_factory,
+      SyncableServiceProvider syncable_service_provider,
+      const base::RepeatingClosure& dump_stack,
+      syncer::SyncService* sync_service,
+      Profile* profile);
+  ~ArcPackageSyncModelTypeController() override;
+
+  // DataTypeController overrides.
+  bool ReadyForStart() const override;
+
+  // ArcSessionManager::Observer:
+  void OnArcPlayStoreEnabledChanged(bool enabled) override;
+  void OnArcInitialStart() override;
+
+ private:
+  syncer::SyncService* const sync_service_;
+  Profile* const profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcPackageSyncModelTypeController);
+};
+#endif  // CHROME_BROWSER_UI_APP_LIST_ARC_ARC_PACKAGE_SYNC_MODEL_TYPE_CONTROLLER_H_

chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc

@@ -120,6 +120,17 @@ bool ArcPackageSyncableService::IsPackageSyncing(
       pending_install_items_.end();
 }
 
+void ArcPackageSyncableService::WaitUntilReadyToSync(base::OnceClosure done) {
+  if (prefs_->package_list_initial_refreshed()) {
+    std::move(done).Run();
+    return;
+  }
+
+  // Wait until the initial list is loaded, handled in
+  // OnPackageListInitialRefreshed().
+  wait_until_ready_to_sync_cb_ = std::move(done);
+}
+
 syncer::SyncMergeResult ArcPackageSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
@@ -322,6 +333,11 @@ void ArcPackageSyncableService::OnPackageModified(
   SendSyncChange(package_info, SyncChange::ACTION_UPDATE);
 }
 
+void ArcPackageSyncableService::OnPackageListInitialRefreshed() {
+  if (wait_until_ready_to_sync_cb_)
+    std::move(wait_until_ready_to_sync_cb_).Run();
+}
+
 void ArcPackageSyncableService::SendSyncChange(
     const mojom::ArcPackageInfo& package_info,
     const syncer::SyncChange::SyncChangeType& sync_change_type) {

chrome/browser/ui/app_list/arc/arc_package_syncable_service.h

@@ -55,6 +55,7 @@ class ArcPackageSyncableService : public syncer::SyncableService,
   bool IsPackageSyncing(const std::string& package_name) const;
 
   // syncer::SyncableService:
+  void WaitUntilReadyToSync(base::OnceClosure done) override;
   syncer::SyncMergeResult MergeDataAndStartSyncing(
       syncer::ModelType type,
       const syncer::SyncDataList& initial_sync_data,
@@ -79,6 +80,7 @@ class ArcPackageSyncableService : public syncer::SyncableService,
   void OnPackageModified(const mojom::ArcPackageInfo& package_info) override;
   void OnPackageRemoved(const std::string& package_name,
                         bool uninstalled) override;
+  void OnPackageListInitialRefreshed() override;
 
   // Sends adds/updates sync change to sync server.
   void SendSyncChange(
@@ -105,6 +107,7 @@ class ArcPackageSyncableService : public syncer::SyncableService,
   bool ShouldSyncPackage(const std::string& package_name) const;
 
   Profile* const profile_;
+  base::OnceClosure wait_until_ready_to_sync_cb_;
   std::unique_ptr<syncer::SyncChangeProcessor> sync_processor_;
   std::unique_ptr<syncer::SyncErrorFactory> sync_error_handler_;
 

components/sync/driver/data_type_manager_impl.cc

@@ -100,17 +100,19 @@ void DataTypeManagerImpl::ReenableType(ModelType type) {
 }
 
 void DataTypeManagerImpl::ReadyForStartChanged(ModelType type) {
-  const auto& dtc_iter = controllers_->find(type);
-  if (dtc_iter == controllers_->end())
+  if (!UpdateUnreadyTypeError(type)) {
+    // Nothing changed.
     return;
+  }
 
-  if (dtc_iter->second->ReadyForStart()) {
-    ForceReconfiguration();
-  } else {
+  if (data_type_status_table_.GetUnreadyErrorTypes().Has(type)) {
     model_association_manager_.StopDatatype(
         type, DISABLE_SYNC,
         SyncError(FROM_HERE, syncer::SyncError::UNREADY_ERROR,
                   "Data type is unready.", type));
+  } else if (last_requested_types_.Has(type)) {
+    // Only reconfigure if the type is both ready and desired.
+    ForceReconfiguration();
   }
 }
 
@@ -347,28 +349,36 @@ TypeSetPriorityList DataTypeManagerImpl::PrioritizeTypes(
 void DataTypeManagerImpl::UpdateUnreadyTypeErrors(
     const ModelTypeSet& desired_types) {
   for (ModelType type : desired_types) {
-    const auto& iter = controllers_->find(type);
-    if (iter == controllers_->end())
-      continue;
-    const DataTypeController* dtc = iter->second.get();
-    bool unready_status =
-        data_type_status_table_.GetUnreadyErrorTypes().Has(type);
-    if (dtc->ReadyForStart() != (unready_status == false)) {
-      // Adjust data_type_status_table_ if unready state in it doesn't match
-      // DataTypeController::ReadyForStart().
-      if (dtc->ReadyForStart()) {
-        data_type_status_table_.ResetUnreadyErrorFor(type);
-      } else {
-        SyncError error(FROM_HERE, SyncError::UNREADY_ERROR,
-                        "Datatype not ready at config time.", type);
-        std::map<ModelType, SyncError> errors;
-        errors[type] = error;
-        data_type_status_table_.UpdateFailedDataTypes(errors);
-      }
-    }
+    UpdateUnreadyTypeError(type);
   }
 }
 
+bool DataTypeManagerImpl::UpdateUnreadyTypeError(ModelType type) {
+  const auto& iter = controllers_->find(type);
+  if (iter == controllers_->end())
+    return false;
+
+  const DataTypeController* dtc = iter->second.get();
+  bool unready_status =
+      data_type_status_table_.GetUnreadyErrorTypes().Has(type);
+  if (dtc->ReadyForStart() == (unready_status == false))
+    return false;
+
+  // Adjust data_type_status_table_ if unready state in it doesn't match
+  // DataTypeController::ReadyForStart().
+  if (dtc->ReadyForStart()) {
+    data_type_status_table_.ResetUnreadyErrorFor(type);
+  } else {
+    SyncError error(FROM_HERE, SyncError::UNREADY_ERROR,
+                    "Datatype not ready at config time.", type);
+    std::map<ModelType, SyncError> errors;
+    errors[type] = error;
+    data_type_status_table_.UpdateFailedDataTypes(errors);
+  }
+
+  return true;
+}
+
 void DataTypeManagerImpl::ProcessReconfigure() {
   // This may have been called asynchronously; no-op if it is no longer needed.
   if (!needs_reconfigure_) {

components/sync/driver/data_type_manager_impl.h

@@ -141,6 +141,11 @@ class DataTypeManagerImpl : public DataTypeManager,
   // DataTypeController::ReadyForStart().
   void UpdateUnreadyTypeErrors(const ModelTypeSet& desired_types);
 
+  // Update unready state for |type|, such that data_type_status_table_ matches
+  // DataTypeController::ReadyForStart(). Returns true if there was an actual
+  // change.
+  bool UpdateUnreadyTypeError(ModelType type);
+
   // Post a task to reconfigure when no downloading or association are running.
   void ProcessReconfigure();
 

components/sync/driver/sync_driver_switches.cc

@@ -48,6 +48,8 @@ const base::Feature kSyncPseudoUSSAppList{"SyncPseudoUSSAppList",
                                           base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSApps{"SyncPseudoUSSApps",
                                        base::FEATURE_ENABLED_BY_DEFAULT};
+const base::Feature kSyncPseudoUSSArcPackage{"SyncPseudoUSSArcPackage",
+                                             base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSDictionary{"SyncPseudoUSSDictionary",
                                              base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSExtensionSettings{

components/sync/driver/sync_driver_switches.h

@@ -24,6 +24,7 @@ extern const base::Feature
     kSyncAllowWalletDataInTransportModeWithCustomPassphrase;
 extern const base::Feature kSyncPseudoUSSAppList;
 extern const base::Feature kSyncPseudoUSSApps;
+extern const base::Feature kSyncPseudoUSSArcPackage;
 extern const base::Feature kSyncPseudoUSSDictionary;
 extern const base::Feature kSyncPseudoUSSExtensionSettings;
 extern const base::Feature kSyncPseudoUSSExtensions;

components/sync/model/syncable_service.cc

@@ -4,10 +4,16 @@
 
 #include "components/sync/model/syncable_service.h"
 
+#include <utility>
+
 namespace syncer {
 
 SyncableService::SyncableService() {}
 
 SyncableService::~SyncableService() {}
 
+void SyncableService::WaitUntilReadyToSync(base::OnceClosure done) {
+  std::move(done).Run();
+}
+
 }  // namespace syncer

components/sync/model/syncable_service.h

@@ -43,6 +43,12 @@ class SyncableService : public base::SupportsWeakPtr<SyncableService> {
   // make pptimizations or tradeoffs by type, etc.
   using StartSyncFlare = base::Callback<void(ModelType)>;
 
+  // Allows the SyncableService to delay sync events (all below) until the model
+  // becomes ready to sync.
+  // TODO(crbug.com/939329): Make this pure to enforce discussion on all
+  // subclasses.
+  virtual void WaitUntilReadyToSync(base::OnceClosure done);
+
   // Informs the service to begin syncing the specified synced datatype |type|.
   // The service should then merge |initial_sync_data| into it's local data,
   // calling |sync_processor|'s ProcessSyncChanges as necessary to reconcile the

components/sync/model_impl/syncable_service_based_bridge.cc

@@ -319,10 +319,9 @@ void SyncableServiceBasedBridge::OnSyncStarting(
     return;
   }
 
-  std::move(store_factory_)
-      .Run(type_, base::BindOnce(&SyncableServiceBasedBridge::OnStoreCreated,
-                                 weak_ptr_factory_.GetWeakPtr()));
-  DCHECK(!store_factory_);
+  syncable_service_->WaitUntilReadyToSync(
+      base::BindOnce(&SyncableServiceBasedBridge::OnSyncableServiceReady,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 base::Optional<ModelError> SyncableServiceBasedBridge::MergeSyncData(
@@ -456,6 +455,15 @@ SyncableServiceBasedBridge::CreateLocalChangeProcessorForTesting(
       other);
 }
 
+void SyncableServiceBasedBridge::OnSyncableServiceReady() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  std::move(store_factory_)
+      .Run(type_, base::BindOnce(&SyncableServiceBasedBridge::OnStoreCreated,
+                                 weak_ptr_factory_.GetWeakPtr()));
+  DCHECK(!store_factory_);
+}
+
 void SyncableServiceBasedBridge::OnStoreCreated(
     const base::Optional<ModelError>& error,
     std::unique_ptr<ModelTypeStore> store) {

components/sync/model_impl/syncable_service_based_bridge.h

@@ -76,6 +76,7 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
                                        ModelTypeChangeProcessor* other);
 
  private:
+  void OnSyncableServiceReady();
   void OnStoreCreated(const base::Optional<ModelError>& error,
                       std::unique_ptr<ModelTypeStore> store);
   void OnReadAllDataForInit(std::unique_ptr<InMemoryStore> in_memory_store,

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -30,14 +30,16 @@
 namespace syncer {
 namespace {
 
+using testing::_;
 using testing::DoAll;
 using testing::ElementsAre;
+using testing::Invoke;
 using testing::IsEmpty;
+using testing::IsNull;
 using testing::NotNull;
 using testing::Pair;
 using testing::Return;
 using testing::SaveArg;
-using testing::_;
 
 const ModelType kModelType = PREFERENCES;
 
@@ -65,6 +67,7 @@ MATCHER_P(HasName, name, "") {
 
 class MockSyncableService : public SyncableService {
  public:
+  MOCK_METHOD1(WaitUntilReadyToSync, void(base::OnceClosure done));
   MOCK_METHOD4(
       MergeDataAndStartSyncing,
       SyncMergeResult(ModelType type,
@@ -82,6 +85,9 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
  protected:
   SyncableServiceBasedBridgeTest()
       : store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()) {
+    ON_CALL(syncable_service_, WaitUntilReadyToSync(_))
+        .WillByDefault(
+            Invoke([](base::OnceClosure done) { std::move(done).Run(); }));
     ON_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _))
         .WillByDefault(
             [&](ModelType type, const SyncDataList& initial_sync_data,
@@ -113,15 +119,18 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
     real_processor_.reset();
   }
 
-  void StartSyncing() {
+  syncer::DataTypeActivationRequest GetTestActivationRequest() {
     syncer::DataTypeActivationRequest request;
     request.error_handler = mock_error_handler_.Get();
     request.cache_guid = "TestCacheGuid";
     request.authenticated_account_id = "SomeAccountId";
+    return request;
+  }
 
+  void StartSyncing() {
     base::RunLoop loop;
     real_processor_->OnSyncStarting(
-        request,
+        GetTestActivationRequest(),
         base::BindLambdaForTesting(
             [&](std::unique_ptr<syncer::DataTypeActivationResponse> response) {
               worker_ = std::make_unique<MockModelTypeWorker>(
@@ -204,6 +213,38 @@ TEST_F(SyncableServiceBasedBridgeTest,
   EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, _)));
 }
 
+TEST_F(SyncableServiceBasedBridgeTest, ShouldWaitUntilModelReadyToSync) {
+  base::OnceClosure syncable_service_ready_cb;
+  ON_CALL(syncable_service_, WaitUntilReadyToSync(_))
+      .WillByDefault(Invoke([&](base::OnceClosure done) {
+        syncable_service_ready_cb = std::move(done);
+      }));
+
+  EXPECT_CALL(mock_processor_, ModelReadyToSync(_)).Times(0);
+  EXPECT_CALL(syncable_service_, WaitUntilReadyToSync(_)).Times(0);
+  EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _)).Times(0);
+
+  // Bridge initialization alone, without sync itself starting, should not
+  // issue calls to the syncable service.
+  InitializeBridge();
+  EXPECT_FALSE(syncable_service_ready_cb);
+
+  // Sync itself starting should wait until the syncable service becomes ready,
+  // before issuing any other call (e.g. MergeDataAndStartSyncing()).
+  EXPECT_CALL(syncable_service_, WaitUntilReadyToSync(_));
+  real_processor_->OnSyncStarting(GetTestActivationRequest(),
+                                  base::DoNothing());
+  ASSERT_TRUE(syncable_service_ready_cb);
+
+  // When the SyncableService gets ready, the bridge should propagate this
+  // information to the processor.
+  EXPECT_CALL(mock_processor_, ModelReadyToSync(_));
+  std::move(syncable_service_ready_cb).Run();
+
+  // Required to initialize the store.
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(SyncableServiceBasedBridgeTest,
        ShouldStopSyncableServiceIfPreviouslyStarted) {
   InitializeBridge();