[themes] Keep local custom ntp background upon GM2 color remote theme

Faulty scenario:
Before MoveThemePrefsToSpecifics, clients sync ntp backgrounds via prefs
and GM2 color theme via ThemeSpecifics. With the flag enabled, the ntp
background is removed upon receiving the GM2 color theme from remote,
since the ThemeSpecifics does not have the ntp_background field
populated yet.
This scenario should only occur for ntp background + GM2 color combo,
since the other combos which use prefs and ThemeSpecifics to sync cannot
co-exist.

Fix: Avoid removing ntp background upon receiving an old ThemeSpecifics
from remote. This covers the case where the specifics is missing the
background not because the ThemeSpecifics didn't have the new fields.

Bug: 391114025
Change-Id: Iecccad3859fb7a015b1910df345e594ad3af7fc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6177275
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Ankush Singh <ankushkush@google.com>
Cr-Commit-Position: refs/heads/main@{#1408693}

chrome/browser/themes/theme_syncable_service.cc

@@ -509,11 +509,14 @@ ThemeSyncableService::ThemeSyncState ThemeSyncableService::MaybeSetTheme(
 
   const bool use_new_fields =
       base::FeatureList::IsEnabled(syncer::kMoveThemePrefsToSpecifics);
+  // Whether the ThemeSpecifics is from a client which commits all theme
+  // attributes via ThemeSpecifics.
+  const bool has_all_theme_attributes = new_specs.has_browser_color_scheme();
   // The new specifics will always include `browser_color_scheme` field. If it
   // is absent and the theme specifics is the default theme, avoid setting to
   // default theme. This is because the old clients can send such specifics upon
   // any change to theme sent via preferences which the new clients do not read.
-  if (use_new_fields && !new_specs.has_browser_color_scheme() &&
+  if (use_new_fields && !has_all_theme_attributes &&
       !HasNonDefaultTheme(new_specs)) {
     DVLOG(1) << "Skip setting default theme from old clients";
     return ThemeSyncState::kApplied;
@@ -593,6 +596,8 @@ ThemeSyncableService::ThemeSyncState ThemeSyncableService::MaybeSetTheme(
     DVLOG(1) << "Switch to use system theme";
     theme_service_->UseSystemTheme();
   } else {
+    // NOTE: No need to check for `is_new_specifics` before setting to default
+    // theme. Empty incoming themes are ignored in MergeDataAndStartSyncing().
     DVLOG(1) << "Switch to use default theme";
     theme_service_->UseDefaultTheme();
   }
@@ -609,8 +614,11 @@ ThemeSyncableService::ThemeSyncState ThemeSyncableService::MaybeSetTheme(
         // of setting the pref directly.
         prefs->SetDict(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse,
                        std::move(*dict));
-      } else {
+      } else if (has_all_theme_attributes) {
         // Clear the current ntp background if none received from remote.
+        // NOTE: Ntp background is only cleared if the incoming ThemeSpecifics
+        // is the new one and is missing the ntp_background field because it was
+        // committed by an old client.
         DVLOG(1) << "Removing custom NTP background";
         prefs->ClearPref(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse);
       }

chrome/browser/themes/theme_syncable_service_unittest.cc

@@ -2354,7 +2354,11 @@ TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
   theme_specifics.set_use_custom_theme(false);
   theme_specifics.set_browser_color_scheme(
       ::sync_pb::ThemeSpecifics_BrowserColorScheme_SYSTEM);
-  theme_specifics.mutable_autogenerated_color_theme()->set_color(SK_ColorBLUE);
+  sync_pb::ThemeSpecifics::UserColorTheme* user_color_theme =
+      theme_specifics.mutable_user_color_theme();
+  user_color_theme->set_color(SK_ColorRED);
+  user_color_theme->set_browser_color_variant(
+      sync_pb::ThemeSpecifics_UserColorTheme_BrowserColorVariant_TONAL_SPOT);
 
   // Start syncing.
   std::optional<syncer::ModelError> error =
@@ -2369,6 +2373,287 @@ TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
       prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
 }
 
+// Regression test for crbug.com/391114025.
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       KeepLocalNtpBackgroundUponNonDefaultOldThemeSpecifics) {
+  // Set local ntp background.
+  base::Value::Dict new_value =
+      base::Value::Dict()
+          .Set(kNtpCustomBackgroundURL, kTestUrl)
+          .Set(kNtpCustomBackgroundAttributionLine1, "attribution_line_1")
+          .Set(kNtpCustomBackgroundAttributionLine2, "attribution_line_2")
+          .Set(kNtpCustomBackgroundAttributionActionURL,
+               "attribution_action_url")
+          .Set(kNtpCustomBackgroundCollectionId, "collection_id")
+          .Set(kNtpCustomBackgroundResumeToken, "resume_token")
+          .Set(kNtpCustomBackgroundRefreshTimestamp, 1234567890)
+          .Set(kNtpCustomBackgroundMainColor, static_cast<int>(SK_ColorRED));
+
+  profile()->GetPrefs()->Set(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse,
+                             base::Value(new_value.Clone()));
+
+  // Remote theme does not contain new fields, thus an old ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.mutable_autogenerated_color_theme()->set_color(SK_ColorBLUE);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local ntp background is still there. The remote theme was produced by an
+  // old client which didn't know about the new ThemeSpecifics fields. It didn't
+  // intentionally clear the background, just left it unset.
+  EXPECT_TRUE(theme_service()->UsingAutogeneratedTheme());
+  EXPECT_EQ(theme_service()->GetAutogeneratedThemeColor(), SK_ColorBLUE);
+  EXPECT_TRUE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+  EXPECT_EQ(profile()->GetPrefs()->GetDict(
+                prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse),
+            new_value);
+}
+
+// Regression test for crbug.com/389026436.
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       KeepLocalNtpBackgroundUponDefaultOldThemeSpecifics) {
+  // Set local ntp background.
+  base::Value::Dict new_value =
+      base::Value::Dict()
+          .Set(kNtpCustomBackgroundURL, kTestUrl)
+          .Set(kNtpCustomBackgroundAttributionLine1, "attribution_line_1")
+          .Set(kNtpCustomBackgroundAttributionLine2, "attribution_line_2")
+          .Set(kNtpCustomBackgroundAttributionActionURL,
+               "attribution_action_url")
+          .Set(kNtpCustomBackgroundCollectionId, "collection_id")
+          .Set(kNtpCustomBackgroundResumeToken, "resume_token")
+          .Set(kNtpCustomBackgroundRefreshTimestamp, 1234567890)
+          .Set(kNtpCustomBackgroundMainColor, static_cast<int>(SK_ColorRED));
+
+  profile()->GetPrefs()->Set(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse,
+                             base::Value(new_value.Clone()));
+
+  // Remote theme does not contain new fields, thus an old ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local ntp background is still there since default remote themes are ignored
+  // in the initial update.
+  EXPECT_TRUE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+  EXPECT_EQ(profile()->GetPrefs()->GetDict(
+                prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse),
+            new_value);
+}
+
+// Regression test for crbug.com/389026436.
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       ClearLocalNtpBackgroundUponNonDefaultNewThemeSpecifics) {
+  // Set local ntp background.
+  base::Value::Dict new_value =
+      base::Value::Dict()
+          .Set(kNtpCustomBackgroundURL, kTestUrl)
+          .Set(kNtpCustomBackgroundAttributionLine1, "attribution_line_1")
+          .Set(kNtpCustomBackgroundAttributionLine2, "attribution_line_2")
+          .Set(kNtpCustomBackgroundAttributionActionURL,
+               "attribution_action_url")
+          .Set(kNtpCustomBackgroundCollectionId, "collection_id")
+          .Set(kNtpCustomBackgroundResumeToken, "resume_token")
+          .Set(kNtpCustomBackgroundRefreshTimestamp, 1234567890)
+          .Set(kNtpCustomBackgroundMainColor, static_cast<int>(SK_ColorRED));
+
+  profile()->GetPrefs()->Set(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse,
+                             base::Value(new_value.Clone()));
+
+  // Remote theme contains new fields, thus a new ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.set_browser_color_scheme(
+      ::sync_pb::ThemeSpecifics_BrowserColorScheme_SYSTEM);
+  theme_specifics.mutable_autogenerated_color_theme()->set_color(SK_ColorBLUE);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local ntp background is cleared, because the remote client must have
+  // explicitly cleared it.
+  EXPECT_TRUE(theme_service()->UsingAutogeneratedTheme());
+  EXPECT_FALSE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+}
+
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       KeepLocalNtpBackgroundUponDefaultNewThemeSpecifics) {
+  // Set local ntp background.
+  base::Value::Dict new_value =
+      base::Value::Dict()
+          .Set(kNtpCustomBackgroundURL, kTestUrl)
+          .Set(kNtpCustomBackgroundAttributionLine1, "attribution_line_1")
+          .Set(kNtpCustomBackgroundAttributionLine2, "attribution_line_2")
+          .Set(kNtpCustomBackgroundAttributionActionURL,
+               "attribution_action_url")
+          .Set(kNtpCustomBackgroundCollectionId, "collection_id")
+          .Set(kNtpCustomBackgroundResumeToken, "resume_token")
+          .Set(kNtpCustomBackgroundRefreshTimestamp, 1234567890)
+          .Set(kNtpCustomBackgroundMainColor, static_cast<int>(SK_ColorRED));
+
+  profile()->GetPrefs()->Set(prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse,
+                             base::Value(new_value.Clone()));
+
+  // Remote theme contains new fields, thus a new ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.set_browser_color_scheme(
+      ::sync_pb::ThemeSpecifics_BrowserColorScheme_SYSTEM);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local ntp background is still there since default remote themes are ignored
+  // in the initial update.
+  EXPECT_TRUE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+  EXPECT_EQ(profile()->GetPrefs()->GetDict(
+                prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse),
+            new_value);
+}
+
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       ClearLocalUserColorUponNonDefaultOldThemeSpecifics) {
+  // Set local user color.
+  theme_service()->SetUserColorAndBrowserColorVariant(
+      SK_ColorBLUE, ui::mojom::BrowserColorVariant::kNeutral);
+  ASSERT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  ASSERT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+
+  // Remote theme does not contain new fields, thus an old ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.mutable_autogenerated_color_theme()->set_color(SK_ColorBLUE);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local user color is cleared because user color and autogenerated color
+  // cannot co-exist.
+  EXPECT_TRUE(theme_service()->UsingAutogeneratedTheme());
+  EXPECT_NE(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  EXPECT_FALSE(theme_service()->GetUserColor());
+}
+
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       KeepLocalUserColorUponDefaultOldThemeSpecifics) {
+  // Set local user color.
+  theme_service()->SetUserColorAndBrowserColorVariant(
+      SK_ColorBLUE, ui::mojom::BrowserColorVariant::kNeutral);
+  ASSERT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  ASSERT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+
+  // Remote theme does not contain new fields, thus an old ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local user color is still there since default remote themes are ignored in
+  // the initial update.
+  EXPECT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  EXPECT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+}
+
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       ClearLocalUserColorUponNonDefaultNewThemeSpecifics) {
+  // Set local user color.
+  theme_service()->SetUserColorAndBrowserColorVariant(
+      SK_ColorBLUE, ui::mojom::BrowserColorVariant::kNeutral);
+  ASSERT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  ASSERT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+
+  // Remote theme contains new fields, thus a new ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.set_browser_color_scheme(
+      ::sync_pb::ThemeSpecifics_BrowserColorScheme_SYSTEM);
+  theme_specifics.mutable_ntp_background()->set_url(kTestUrl);
+  ASSERT_FALSE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  EXPECT_TRUE(profile()->GetPrefs()->GetUserPrefValue(
+      prefs::kNonSyncingNtpCustomBackgroundDictDoNotUse));
+  // Local user color is cleared since the remote client must have explicitly
+  // cleared it.
+  EXPECT_NE(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  EXPECT_FALSE(theme_service()->GetUserColor());
+}
+
+TEST_F(ThemeSyncableServiceWithMigrationFlagEnabledTest,
+       KeepLocalUserColorUponDefaultNewThemeSpecifics) {
+  // Set local user color.
+  theme_service()->SetUserColorAndBrowserColorVariant(
+      SK_ColorBLUE, ui::mojom::BrowserColorVariant::kNeutral);
+  ASSERT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  ASSERT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+
+  // Remote theme contains new fields, thus a new ThemeSpecifics.
+  sync_pb::ThemeSpecifics theme_specifics;
+  theme_specifics.set_use_custom_theme(false);
+  theme_specifics.set_browser_color_scheme(
+      ::sync_pb::ThemeSpecifics_BrowserColorScheme_SYSTEM);
+
+  // Start syncing.
+  std::optional<syncer::ModelError> error =
+      theme_sync_service()->MergeDataAndStartSyncing(
+          syncer::THEMES, MakeThemeDataList(theme_specifics),
+          std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+              fake_change_processor()));
+  ASSERT_FALSE(error.has_value()) << error.value().message();
+
+  // Local user color is still there since default remote themes are ignored in
+  // the initial update.
+  EXPECT_EQ(theme_service()->GetThemeID(), ThemeService::kUserColorThemeID);
+  EXPECT_EQ(theme_service()->GetUserColor(), SK_ColorBLUE);
+}
+
 class ThemeSyncableServiceVerifyFinalStateTest
     : public ThemeSyncableServiceWithMigrationFlagEnabledTest,
       public testing::WithParamInterface<sync_pb::ThemeSpecifics> {