Merge 165521 - [Sync] Re-reland fix for bug 154940

Speculatively fixed test by making syncer::UserTypes() stack allocated at
a higher scope, instead of within the for loop. I suspect on certain windows
builds the for loop allocation was being dealloc'd early, hence invalidating
the iterator.

Original codereview at http://codereview.chromium.org/11342022/

BUG=158391
TBR=tim@chromium.org

Review URL: https://chromiumcodereview.appspot.com/11365041

TBR=zea@chromium.org
Review URL: https://codereview.chromium.org/11369194

git-svn-id: svn://svn.chromium.org/chrome/branches/1312/src@167222 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 100 insertions, 0 deletions

diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 8cc0408..b4d4805 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/net/chrome_cookie_notification_details.h"
+#include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
@@ -52,6 +53,7 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/time_format.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/notification_details.h"
@@ -216,9 +218,45 @@ void ProfileSyncService::Initialize() {
     DisableForUser();
   }
 
+  TrySyncDatatypePrefRecovery();
+
   TryStart();
 }
 
+void ProfileSyncService::TrySyncDatatypePrefRecovery() {
+  DCHECK(!sync_initialized());
+  if (!HasSyncSetupCompleted())
+    return;
+
+  // There was a bug where OnUserChoseDatatypes was not properly called on
+  // configuration (see crbug.com/154940). We detect this by checking whether
+  // kSyncKeepEverythingSynced has a default value. If so, and sync setup has
+  // completed, it means sync was not properly configured, so we manually
+  // set kSyncKeepEverythingSynced.
+  PrefService* const pref_service = profile_->GetPrefs();
+  if (!pref_service)
+    return;
+  if (sync_prefs_.HasKeepEverythingSynced())
+    return;
+  const syncer::ModelTypeSet registered_types = GetRegisteredDataTypes();
+  if (sync_prefs_.GetPreferredDataTypes(registered_types).Size() > 1)
+    return;
+
+  const PrefService::Preference* keep_everything_synced =
+      pref_service->FindPreference(prefs::kSyncKeepEverythingSynced);
+  // This will be false if the preference was properly set or if it's controlled
+  // by policy.
+  if (!keep_everything_synced->IsDefaultValue())
+    return;
+
+  // kSyncKeepEverythingSynced was not properly set. Set it and the preferred
+  // types now, before we configure.
+  UMA_HISTOGRAM_COUNTS("Sync.DatatypePrefRecovery", 1);
+  sync_prefs_.SetKeepEverythingSynced(true);
+  sync_prefs_.SetPreferredDataTypes(registered_types,
+                                    registered_types);
+}
+
 void ProfileSyncService::TryStart() {
   if (!IsSyncEnabledAndLoggedIn())
     return;
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index e2f0384..7aeea0d 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -648,6 +648,11 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   friend class TestProfileSyncService;
   FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceTest, InitialState);
 
+  // Detects and attempts to recover from a previous improper datatype
+  // configuration where Keep Everything Synced and the preferred types were
+  // not correctly set.
+  void TrySyncDatatypePrefRecovery();
+
   // Starts up sync if it is not suppressed and preconditions are met.
   // Called from Initialize() and UnsuppressAndStart().
   void TryStart();
diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc
index 0109525..f38cc5e 100644
--- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/sync/glue/data_type_manager_mock.h"
 #include "chrome/browser/sync/profile_sync_components_factory_mock.h"
 #include "chrome/browser/sync/profile_sync_test_util.h"
+#include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/browser/sync/test_profile_sync_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
@@ -268,6 +269,62 @@ TEST_F(ProfileSyncServiceStartupTest, StartNormal) {
   service_->Initialize();
 }
 
+// Test that we can recover from a case where a bug in the code resulted in
+// OnUserChoseDatatypes not being properly called and datatype preferences
+// therefore being left unset.
+TEST_F(ProfileSyncServiceStartupTest, StartRecoverDatatypePrefs) {
+  DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
+  EXPECT_CALL(*data_type_manager, Configure(_, _));
+  EXPECT_CALL(*data_type_manager, state()).
+      WillRepeatedly(Return(DataTypeManager::CONFIGURED));
+  EXPECT_CALL(*data_type_manager, Stop()).Times(1);
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
+
+  // Clear the datatype preference fields (simulating bug 154940).
+  profile_->GetPrefs()->ClearPref(prefs::kSyncKeepEverythingSynced);
+  syncer::ModelTypeSet user_types = syncer::UserTypes();
+  for (syncer::ModelTypeSet::Iterator iter = user_types.First();
+       iter.Good(); iter.Inc()) {
+    profile_->GetPrefs()->ClearPref(
+        browser_sync::SyncPrefs::GetPrefNameForDataType(iter.Get()));
+  }
+
+  // Pre load the tokens
+  TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest(
+      GaiaConstants::kSyncService, "sync_token");
+  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, "test_user");
+  service_->Initialize();
+
+  EXPECT_TRUE(profile_->GetPrefs()->GetBoolean(
+      prefs::kSyncKeepEverythingSynced));
+}
+
+// Verify that the recovery of datatype preferences doesn't overwrite a valid
+// case where only bookmarks are enabled.
+TEST_F(ProfileSyncServiceStartupTest, StartDontRecoverDatatypePrefs) {
+  DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
+  EXPECT_CALL(*data_type_manager, Configure(_, _));
+  EXPECT_CALL(*data_type_manager, state()).
+      WillRepeatedly(Return(DataTypeManager::CONFIGURED));
+  EXPECT_CALL(*data_type_manager, Stop()).Times(1);
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
+
+  // Explicitly set Keep Everything Synced to false and have only bookmarks
+  // enabled.
+  profile_->GetPrefs()->SetBoolean(prefs::kSyncKeepEverythingSynced, false);
+
+  // Pre load the tokens
+  TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest(
+      GaiaConstants::kSyncService, "sync_token");
+  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, "test_user");
+  service_->Initialize();
+
+  EXPECT_FALSE(profile_->GetPrefs()->GetBoolean(
+      prefs::kSyncKeepEverythingSynced));
+}
+
 TEST_F(ProfileSyncServiceStartupTest, ManagedStartup) {
   // Disable sync through policy.
   profile_->GetPrefs()->SetBoolean(prefs::kSyncManaged, true);