chromeos: Fix default app loading when SplitSettingsSync enabled

SplitSettingsSync leaves sync disabled until the user completes the
sync consent OOBE dialog. ExternalPrefLoader was incorrectly deciding
that sync would never enable and starting default app loading before
priority prefs are synced. This results in default apps loading too
early, potentially before the user's language is synced.

Add a pref for whether the sync consent screen is completed. If the
user hasn't completed it, observe for that pref changing, then wait
for OS priority pref sync to complete.

Bug: 1078479
Test: added to unit_tests and browser_tests
Change-Id: If633133acccfd43fe80b727b45080599ad569ceb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207967Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771277}

chrome/browser/chromeos/login/screens/sync_consent_browsertest.cc

@@ -33,6 +33,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_pref_names.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/public/identity_manager/consent_level.h"
@@ -436,6 +437,9 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_DefaultFlow) {
   PrefService* prefs = profile->GetPrefs();
   EXPECT_FALSE(prefs->GetBoolean(syncer::prefs::kOsSyncFeatureEnabled));
 
+  // Dialog not completed yet.
+  EXPECT_FALSE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
+
   // Wait for content to load.
   SyncConsentScreen* screen = GetSyncConsentScreen();
   ConsentRecordedWaiter consent_recorded_waiter;
@@ -493,6 +497,9 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_DefaultFlow) {
   histogram_tester_.ExpectTotalCount(
       "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 1);
   histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 1);
+
+  // Dialog is completed.
+  EXPECT_TRUE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
 }
 
 // Flaky failures on sanitizer builds. https://crbug.com/1054377
@@ -580,6 +587,10 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest,
   // Browser sync is on.
   EXPECT_TRUE(settings->IsSyncRequested());
   EXPECT_TRUE(settings->IsFirstSetupComplete());
+
+  // Dialog is completed.
+  PrefService* prefs = ProfileManager::GetPrimaryUserProfile()->GetPrefs();
+  EXPECT_TRUE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
 }
 
 IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest,
@@ -598,6 +609,10 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest,
   // Browser sync is off.
   EXPECT_FALSE(settings->IsSyncRequested());
   EXPECT_FALSE(settings->IsFirstSetupComplete());
+
+  // Dialog is completed.
+  PrefService* prefs = ProfileManager::GetPrimaryUserProfile()->GetPrefs();
+  EXPECT_TRUE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
 }
 
 // Tests that the SyncConsent screen performs a timezone request so that
@@ -617,7 +632,7 @@ IN_PROC_BROWSER_TEST_F(SyncConsentTimezoneOverride, MakesTimezoneRequest) {
   LoginToSyncConsentScreen();
   EXPECT_EQ("TimezeonPropagationTest",
             g_browser_process->local_state()->GetString(
-                prefs::kSigninScreenTimezone));
+                ::prefs::kSigninScreenTimezone));
 }
 
 }  // namespace

chrome/browser/chromeos/login/screens/sync_consent_screen.cc

@@ -19,6 +19,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_pref_names.h"
 #include "components/consent_auditor/consent_auditor.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/public/identity_manager/consent_level.h"
@@ -71,7 +72,8 @@ std::string SyncConsentScreen::GetResultString(Result result) {
 
 // static
 void SyncConsentScreen::MaybeLaunchSyncConsentSettings(Profile* profile) {
-  if (profile->GetPrefs()->GetBoolean(prefs::kShowSyncSettingsOnSessionStart)) {
+  if (profile->GetPrefs()->GetBoolean(
+          ::prefs::kShowSyncSettingsOnSessionStart)) {
     // TODO (alemate): In a very special case when chrome is exiting at the very
     // moment we show Settings, it might crash here because profile could be
     // already destroyed. This needs to be fixed.
@@ -80,7 +82,7 @@ void SyncConsentScreen::MaybeLaunchSyncConsentSettings(Profile* profile) {
         base::BindOnce(
             [](Profile* profile) {
               profile->GetPrefs()->ClearPref(
-                  prefs::kShowSyncSettingsOnSessionStart);
+                  ::prefs::kShowSyncSettingsOnSessionStart);
               chrome::ShowSettingsSubPageForProfile(profile,
                                                     chrome::kSyncSetupSubPage);
             },
@@ -111,13 +113,20 @@ void SyncConsentScreen::Init() {
   UpdateScreen();
 }
 
+void SyncConsentScreen::Finish(Result result) {
+  DCHECK(profile_);
+  // Always set completed, even if the dialog was skipped (e.g. by policy).
+  profile_->GetPrefs()->SetBoolean(chromeos::prefs::kSyncOobeCompleted, true);
+  exit_callback_.Run(result);
+}
+
 bool SyncConsentScreen::MaybeSkip() {
   Init();
 
   if (behavior_ == SyncScreenBehavior::kSkip ||
       behavior_ == SyncScreenBehavior::kSkipAndEnableSync) {
     MaybeEnableSyncForSkip();
-    exit_callback_.Run(Result::NOT_APPLICABLE);
+    Finish(Result::NOT_APPLICABLE);
     return true;
   }
 
@@ -156,9 +165,9 @@ void SyncConsentScreen::OnContinueAndReview(
     return;
   RecordUmaReviewFollowingSetup(true);
   RecordConsent(CONSENT_GIVEN, consent_description, consent_confirmation);
-  profile_->GetPrefs()->SetBoolean(prefs::kShowSyncSettingsOnSessionStart,
+  profile_->GetPrefs()->SetBoolean(::prefs::kShowSyncSettingsOnSessionStart,
                                    true);
-  exit_callback_.Run(Result::NEXT);
+  Finish(Result::NEXT);
 }
 
 void SyncConsentScreen::OnContinueWithDefaults(
@@ -168,7 +177,7 @@ void SyncConsentScreen::OnContinueWithDefaults(
     return;
   RecordUmaReviewFollowingSetup(false);
   RecordConsent(CONSENT_GIVEN, consent_description, consent_confirmation);
-  exit_callback_.Run(Result::NEXT);
+  Finish(Result::NEXT);
 }
 
 void SyncConsentScreen::OnAcceptAndContinue(
@@ -183,7 +192,7 @@ void SyncConsentScreen::OnAcceptAndContinue(
   // they chose to enable.
   RecordConsent(CONSENT_GIVEN, consent_description, consent_confirmation);
   UpdateSyncSettings(enable_os_sync, enable_browser_sync);
-  exit_callback_.Run(Result::NEXT);
+  Finish(Result::NEXT);
 }
 
 void SyncConsentScreen::UpdateSyncSettings(bool enable_os_sync,
@@ -279,7 +288,7 @@ SyncConsentScreen::SyncScreenBehavior SyncConsentScreen::GetSyncScreenBehavior()
 
   // Skip if the sync consent screen is disabled by policy, for example, in
   // education scenarios. https://crbug.com/841156
-  if (!profile_->GetPrefs()->GetBoolean(prefs::kEnableSyncConsent))
+  if (!profile_->GetPrefs()->GetBoolean(::prefs::kEnableSyncConsent))
     return SyncScreenBehavior::kSkipAndEnableSync;
 
   // Skip if sync-the-feature is disabled by policy.
@@ -311,7 +320,7 @@ void SyncConsentScreen::UpdateScreen() {
     case SyncScreenBehavior::kSkip:
     case SyncScreenBehavior::kSkipAndEnableSync:
       MaybeEnableSyncForSkip();
-      exit_callback_.Run(Result::NEXT);
+      Finish(Result::NEXT);
       break;
     case SyncScreenBehavior::kUnknown:
       NOTREACHED();

chrome/browser/chromeos/login/screens/sync_consent_screen.h

@@ -121,6 +121,9 @@ class SyncConsentScreen : public BaseScreen,
   }
 
  private:
+  // Marks the dialog complete and runs |exit_callback_|.
+  void Finish(Result result);
+
   // BaseScreen:
   bool MaybeSkip() override;
   void ShowImpl() override;

chrome/browser/chromeos/preferences.cc

@@ -444,6 +444,8 @@ void Preferences::RegisterProfilePrefs(
   // By default showing Sync Consent is set to true. It can changed by policy.
   registry->RegisterBooleanPref(::prefs::kEnableSyncConsent, true);
 
+  registry->RegisterBooleanPref(chromeos::prefs::kSyncOobeCompleted, false);
+
   registry->RegisterBooleanPref(::prefs::kTPMFirmwareUpdateCleanupDismissed,
                                 false);
 

chrome/browser/extensions/external_pref_loader.cc

@@ -31,7 +31,10 @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_pref_names.h"
 #include "chromeos/constants/chromeos_switches.h"
+#include "components/prefs/pref_change_registrar.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_service_observer.h"
 #include "components/sync/driver/sync_user_settings.h"
@@ -131,17 +134,53 @@ class ExternalPrefLoader::PrioritySyncReadyWaiter
       // Note: |this| is deleted here.
       return;
     }
-    // Start observing sync changes.
+    DCHECK(!done_closure_);
+    done_closure_ = std::move(done_closure);
+    if (chromeos::features::IsSplitSettingsSyncEnabled()) {
+      // SplitSettingsSync lets users opt-out of sync during OOBE.
+      PrefService* prefs = profile_->GetPrefs();
+      if (!prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted)) {
+        // Need to wait for OOBE completion before checking if sync is enabled.
+        pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
+        pref_change_registrar_->Init(prefs);
+        // base::Unretained is safe because we own |pref_changed_registrar_|.
+        pref_change_registrar_->Add(
+            chromeos::prefs::kSyncOobeCompleted,
+            base::BindRepeating(&PrioritySyncReadyWaiter::OnSyncOobeCompleted,
+                                base::Unretained(this)));
+        return;
+      }
+    }
+    MaybeObserveSyncStart();
+  }
+
+ private:
+  void OnSyncOobeCompleted() {
+    DCHECK(chromeos::features::IsSplitSettingsSyncEnabled());
+    DCHECK(
+        profile_->GetPrefs()->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
+    pref_change_registrar_.reset();
+    syncer::SyncService* service =
+        ProfileSyncServiceFactory::GetForProfile(profile_);
+    if (!service->GetUserSettings()->IsOsSyncFeatureEnabled()) {
+      // User opted-out of OS sync, OS sync will never start, we're done here.
+      Finish();
+      // Note: |this| is deleted.
+      return;
+    }
+    MaybeObserveSyncStart();
+  }
+
+  void MaybeObserveSyncStart() {
     syncer::SyncService* service =
         ProfileSyncServiceFactory::GetForProfile(profile_);
     DCHECK(service);
-    if (service->CanSyncFeatureStart()) {
-      done_closure_ = std::move(done_closure);
-      AddObservers();
-    } else {
-      std::move(done_closure).Run();
-      // Note: |this| is deleted here.
+    if (!service->CanSyncFeatureStart()) {
+      Finish();
+      // Note: |this| is deleted.
+      return;
     }
+    AddObservers();
   }
 
   // sync_preferences::PrefServiceSyncableObserver:
@@ -160,12 +199,15 @@ class ExternalPrefLoader::PrioritySyncReadyWaiter
       Finish();
   }
 
- private:
   bool IsPrioritySyncing() {
     sync_preferences::PrefServiceSyncable* prefs =
         PrefServiceSyncableFromProfile(profile_);
     DCHECK(prefs);
-    return prefs->IsPrioritySyncing();
+    // SplitSettingsSync moves prefs like language and keyboard/mouse config to
+    // OS priority prefs.
+    return chromeos::features::IsSplitSettingsSyncEnabled()
+               ? prefs->AreOsPriorityPrefsSyncing()
+               : prefs->IsPrioritySyncing();
   }
 
   void AddObservers() {
@@ -185,6 +227,9 @@ class ExternalPrefLoader::PrioritySyncReadyWaiter
 
   base::OnceClosure done_closure_;
 
+  // Used with SplitSettingsSync to wait for OOBE sync dialog completion.
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
+
   // Used for registering observer for sync_preferences::PrefServiceSyncable.
   ScopedObserver<sync_preferences::PrefServiceSyncable,
                  sync_preferences::PrefServiceSyncableObserver>

chrome/browser/extensions/external_pref_loader_unittest.cc

@@ -4,15 +4,26 @@
 
 #include "chrome/browser/extensions/external_pref_loader.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "components/sync/driver/test_sync_service.h"
+#include "components/sync/model/fake_sync_change_processor.h"
+#include "components/sync/model/sync_error_factory_mock.h"
+#include "components/sync_preferences/pref_model_associator.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/test/browser_task_environment.h"
 #include "extensions/common/extension.h"
@@ -83,15 +94,24 @@ class ExternalPrefLoaderTest : public testing::Test {
   ExternalPrefLoaderTest() {}
   ~ExternalPrefLoaderTest() override {}
 
-  void SetUp() override { profile_ = std::make_unique<TestingProfile>(); }
+  void SetUp() override {
+    profile_ = std::make_unique<TestingProfile>();
+    sync_service_ = static_cast<TestSyncService*>(
+        ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            profile(), base::BindRepeating(&TestingSyncFactoryFunction)));
+    sync_service_->SetFirstSetupComplete(true);
+  }
 
   void TearDown() override { profile_.reset(); }
 
-  Profile* profile() { return profile_.get(); }
+  TestingProfile* profile() { return profile_.get(); }
+
+  TestSyncService* sync_service() { return sync_service_; }
 
  private:
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<TestingProfile> profile_;
+  TestSyncService* sync_service_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(ExternalPrefLoaderTest);
 };
@@ -102,11 +122,6 @@ class ExternalPrefLoaderTest : public testing::Test {
 // Tests that we fire pref reading correctly after priority sync state
 // is resolved by ExternalPrefLoader.
 TEST_F(ExternalPrefLoaderTest, PrefReadInitiatesCorrectly) {
-  TestSyncService* test_service = static_cast<TestSyncService*>(
-      ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-          profile(), base::BindRepeating(&TestingSyncFactoryFunction)));
-  test_service->SetFirstSetupComplete(true);
-
   base::RunLoop run_loop;
   scoped_refptr<ExternalPrefLoader> loader(
       new TestExternalPrefLoader(profile(), run_loop.QuitWhenIdleClosure()));
@@ -117,11 +132,96 @@ TEST_F(ExternalPrefLoaderTest, PrefReadInitiatesCorrectly) {
 
   // Initially CanSyncFeatureStart() returns true, returning false will let
   // |loader| proceed.
-  test_service->SetDisableReasons(
+  sync_service()->SetDisableReasons(
       syncer::SyncService::DISABLE_REASON_USER_CHOICE);
-  ASSERT_FALSE(test_service->CanSyncFeatureStart());
-  test_service->FireOnStateChanged();
+  ASSERT_FALSE(sync_service()->CanSyncFeatureStart());
+  sync_service()->FireOnStateChanged();
+  run_loop.Run();
+}
+
+class ExternalPrefLoaderSplitSettingsSyncTest : public ExternalPrefLoaderTest {
+ public:
+  ExternalPrefLoaderSplitSettingsSyncTest() {
+    feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  }
+  ~ExternalPrefLoaderSplitSettingsSyncTest() override = default;
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(ExternalPrefLoaderSplitSettingsSyncTest, OsSyncEnabled) {
+  base::RunLoop run_loop;
+  scoped_refptr<ExternalPrefLoader> loader =
+      base::MakeRefCounted<TestExternalPrefLoader>(
+          profile(), run_loop.QuitWhenIdleClosure());
+  ExternalProviderImpl provider(
+      /*service=*/nullptr, loader, profile(), Manifest::INVALID_LOCATION,
+      Manifest::INVALID_LOCATION, Extension::NO_FLAGS);
+  provider.VisitRegisteredExtension();
+
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_FALSE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
+
+  // Simulate OOBE screen completion with OS sync enabled.
+  sync_service()->GetUserSettings()->SetOsSyncFeatureEnabled(true);
+  prefs->SetBoolean(chromeos::prefs::kSyncOobeCompleted, true);
+
+  // Simulate OS prefs starting to sync.
+  sync_preferences::PrefServiceSyncable* pref_sync =
+      profile()->GetTestingPrefService();
+  // This is an ugly cast, but it's how other tests do it.
+  sync_preferences::PrefModelAssociator* pref_sync_service =
+      static_cast<sync_preferences::PrefModelAssociator*>(
+          pref_sync->GetSyncableService(syncer::OS_PRIORITY_PREFERENCES));
+  pref_sync_service->MergeDataAndStartSyncing(
+      syncer::OS_PRIORITY_PREFERENCES, syncer::SyncDataList(),
+      std::make_unique<syncer::FakeSyncChangeProcessor>(),
+      std::make_unique<syncer::SyncErrorFactoryMock>());
+
+  run_loop.Run();
+  // |loader| completed loading.
+}
+
+TEST_F(ExternalPrefLoaderSplitSettingsSyncTest, OsSyncDisable) {
+  base::RunLoop run_loop;
+  scoped_refptr<ExternalPrefLoader> loader =
+      base::MakeRefCounted<TestExternalPrefLoader>(
+          profile(), run_loop.QuitWhenIdleClosure());
+  ExternalProviderImpl provider(
+      /*service=*/nullptr, loader, profile(), Manifest::INVALID_LOCATION,
+      Manifest::INVALID_LOCATION, Extension::NO_FLAGS);
+  provider.VisitRegisteredExtension();
+
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_FALSE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
+
+  // Simulate OOBE screen completion with OS sync disabled.
+  sync_service()->GetUserSettings()->SetOsSyncFeatureEnabled(false);
+  prefs->SetBoolean(chromeos::prefs::kSyncOobeCompleted, true);
+
+  // Loader doesn't need to wait, since OS pref sync is disabled.
+  run_loop.Run();
+  // |loader| completed loading.
+}
+
+TEST_F(ExternalPrefLoaderSplitSettingsSyncTest, SyncDisabledByPolicy) {
+  sync_service()->SetDisableReasons(
+      syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY);
+  ASSERT_FALSE(sync_service()->CanSyncFeatureStart());
+
+  base::RunLoop run_loop;
+  scoped_refptr<ExternalPrefLoader> loader =
+      base::MakeRefCounted<TestExternalPrefLoader>(
+          profile(), run_loop.QuitWhenIdleClosure());
+  ExternalProviderImpl provider(
+      /*service=*/nullptr, loader, profile(), Manifest::INVALID_LOCATION,
+      Manifest::INVALID_LOCATION, Extension::NO_FLAGS);
+  provider.VisitRegisteredExtension();
+
+  // Loader doesn't need to wait, because sync will never enable.
   run_loop.Run();
+  // |loader| completed loading.
 }
 
 }  // namespace extensions

chromeos/constants/chromeos_pref_names.cc

@@ -78,6 +78,12 @@ const char kSamlPasswordExpirationTime[] = "saml.password_expiration_time";
 // to the SAML IdP.
 const char kSamlPasswordChangeUrl[] = "saml.password_change_url";
 
+// Boolean pref indicating whether the user has completed (or skipped) the
+// out-of-box experience (OOBE) sync consent screen. Before this pref is set
+// both OS and browser sync will be disabled. After this pref is set it's
+// possible to check sync state to see if the user enabled it.
+const char kSyncOobeCompleted[] = "sync.oobe_completed";
+
 // Boolean pref indicating whether a user has enabled the display password
 // button on the login/lock screen.
 const char kLoginDisplayPasswordButtonEnabled[] =

chromeos/constants/chromeos_pref_names.h

@@ -35,6 +35,8 @@ extern const char kSamlPasswordExpirationTime[];
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 extern const char kSamlPasswordChangeUrl[];
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
+extern const char kSyncOobeCompleted[];
+COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 extern const char kLoginDisplayPasswordButtonEnabled[];
 
 }  // namespace prefs