Revert 148792 - [Sync] Remove CleanupDisabledTypes command and move purge logic into SyncManager.

We were only ever performing a meaningful cleanup on reconfigurations
or restart, so we make that explicit by purging from within the SyncManager's
loading and configuration methods.

BUG=131433, 90868
TEST=manual

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

TBR=zea@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10823061

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148812 0039d316-1c4b-4281-b951-d872f2087c98

sync/engine/cleanup_disabled_types_command.cc

+// Copyright (c) 2012 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 "sync/engine/cleanup_disabled_types_command.h"
+
+#include <algorithm>
+
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/sessions/sync_session.h"
+#include "sync/sessions/sync_session_context.h"
+#include "sync/syncable/directory.h"
+
+namespace syncer {
+
+CleanupDisabledTypesCommand::CleanupDisabledTypesCommand() {}
+CleanupDisabledTypesCommand::~CleanupDisabledTypesCommand() {}
+
+SyncerError CleanupDisabledTypesCommand::ExecuteImpl(
+    sessions::SyncSession* session) {
+  // Because a full directory purge is slow, we avoid purging
+  // undesired types unless we have reason to believe they were
+  // previously enabled.  Because purging could theoretically fail on
+  // the first sync session (when there's no previous routing info) we
+  // pay the full directory scan price once and do a "deep clean" of
+  // types that may potentially need cleanup so that we converge to
+  // the correct state.
+  //
+  //                          in_previous  |   !in_previous
+  //                                       |
+  //   initial_sync_ended     should clean |  may have attempted cleanup
+  //  !initial_sync_ended     should clean |  may have never been enabled, or
+  //                                       |  could have been disabled before
+  //                                       |  initial sync ended and cleanup
+  //                                       |  may not have happened yet
+  //                                       |  (failure, browser restart
+  //                                       |  before another sync session,..)
+
+  const ModelTypeSet enabled_types =
+      GetRoutingInfoTypes(session->routing_info());
+
+  const ModelTypeSet previous_enabled_types =
+      GetRoutingInfoTypes(
+          session->context()->previous_session_routing_info());
+
+  ModelTypeSet to_cleanup = Difference(ModelTypeSet::All(), enabled_types);
+
+  // If |previous_enabled_types| is non-empty (i.e., not the first
+  // sync session), set |to_cleanup| to its intersection with
+  // |previous_enabled_types|.
+  if (!previous_enabled_types.Empty()) {
+    to_cleanup.RetainAll(previous_enabled_types);
+  }
+
+  DVLOG(1) << "enabled_types = " << ModelTypeSetToString(enabled_types)
+           << ", previous_enabled_types = "
+           << ModelTypeSetToString(previous_enabled_types)
+           << ", to_cleanup = " << ModelTypeSetToString(to_cleanup);
+
+  if (to_cleanup.Empty())
+    return SYNCER_OK;
+
+  session->context()->directory()->PurgeEntriesWithTypeIn(to_cleanup);
+  return SYNCER_OK;
+}
+
+}  // namespace syncer

sync/engine/cleanup_disabled_types_command.h

+// Copyright (c) 2012 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 SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_
+#define SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_
+
+#include "base/compiler_specific.h"
+#include "sync/engine/syncer_command.h"
+
+namespace syncer {
+
+// A syncer command that purges (from memory and disk) entries belonging to
+// a ModelType or ServerModelType that the user has not elected to sync.
+//
+// This is done as part of a session to 1) ensure it does not block the UI,
+// and 2) avoid complicated races that could arise between a) deleting
+// things b) a sync session trying to use these things c) and the potential
+// re-enabling of the data type by the user before some scheduled deletion
+// took place.  Here, we are safe to perform I/O synchronously and we know it
+// is a safe time to delete as we are in the only active session.
+//
+// The removal from memory is done synchronously, while the disk purge is left
+// to an asynchronous SaveChanges operation.  However, all the updates for
+// meta data fields (such as initial_sync_ended) as well as the actual entry
+// deletions will be committed in a single sqlite transaction.  Thus it is
+// possible that disabled types re-appear (in the sync db) after a reboot,
+// but things will remain in a consistent state.  This kind of error case is
+// cared for in this command by retrying; see ExecuteImpl.
+class CleanupDisabledTypesCommand : public SyncerCommand {
+ public:
+  CleanupDisabledTypesCommand();
+  virtual ~CleanupDisabledTypesCommand();
+
+  // SyncerCommand implementation.
+  virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CleanupDisabledTypesCommand);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_

sync/engine/cleanup_disabled_types_command_unittest.cc

+// Copyright (c) 2012 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 <vector>
+
+#include "sync/engine/cleanup_disabled_types_command.h"
+
+#include "sync/internal_api/public/base/model_type_test_util.h"
+#include "sync/sessions/sync_session.h"
+#include "sync/test/engine/syncer_command_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+using testing::_;
+
+class CleanupDisabledTypesCommandTest : public MockDirectorySyncerCommandTest {
+ public:
+  CleanupDisabledTypesCommandTest() {}
+
+  virtual void SetUp() {
+    mutable_routing_info()->clear();
+    (*mutable_routing_info())[BOOKMARKS] = GROUP_PASSIVE;
+    MockDirectorySyncerCommandTest::SetUp();
+  }
+};
+
+// TODO(tim): Add syncer test to verify previous routing info is set.
+TEST_F(CleanupDisabledTypesCommandTest, NoPreviousRoutingInfo) {
+  CleanupDisabledTypesCommand command;
+  ModelTypeSet expected = ModelTypeSet::All();
+  expected.Remove(BOOKMARKS);
+  EXPECT_CALL(*mock_directory(),
+              PurgeEntriesWithTypeIn(HasModelTypes(expected)));
+  command.ExecuteImpl(session());
+}
+
+TEST_F(CleanupDisabledTypesCommandTest, NoPurge) {
+  CleanupDisabledTypesCommand command;
+  EXPECT_CALL(*mock_directory(), PurgeEntriesWithTypeIn(_)).Times(0);
+
+  ModelSafeRoutingInfo prev(routing_info());
+  session()->context()->set_previous_session_routing_info(prev);
+  (*mutable_routing_info())[AUTOFILL] = GROUP_PASSIVE;
+  command.ExecuteImpl(session());
+
+  prev = routing_info();
+  command.ExecuteImpl(session());
+}
+
+TEST_F(CleanupDisabledTypesCommandTest, TypeDisabled) {
+  CleanupDisabledTypesCommand command;
+
+  (*mutable_routing_info())[AUTOFILL] = GROUP_PASSIVE;
+  (*mutable_routing_info())[THEMES] = GROUP_PASSIVE;
+  (*mutable_routing_info())[EXTENSIONS] = GROUP_PASSIVE;
+
+  ModelSafeRoutingInfo prev(routing_info());
+  prev[PASSWORDS] = GROUP_PASSIVE;
+  prev[PREFERENCES] = GROUP_PASSIVE;
+  session()->context()->set_previous_session_routing_info(prev);
+
+  const ModelTypeSet expected(PASSWORDS, PREFERENCES);
+  EXPECT_CALL(*mock_directory(),
+              PurgeEntriesWithTypeIn(HasModelTypes(expected)));
+  command.ExecuteImpl(session());
+}
+
+}  // namespace
+
+}  // namespace syncer

sync/engine/sync_scheduler_impl.cc

@@ -132,6 +132,7 @@ const char* SyncSchedulerImpl::SyncSessionJob::GetPurposeString(
     ENUM_CASE(POLL);
     ENUM_CASE(NUDGE);
     ENUM_CASE(CONFIGURATION);
+    ENUM_CASE(CLEANUP_DISABLED_TYPES);
   }
   NOTREACHED();
   return "";
@@ -353,6 +354,17 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
                        &restricted_workers);
   session_context_->set_routing_info(params.routing_info);
 
+  // We rely on this not failing, so don't need to worry about checking for
+  // success. In addition, this will be removed as part of crbug.com/131433.
+  SyncSessionJob cleanup_job(
+      SyncSessionJob::CLEANUP_DISABLED_TYPES,
+      TimeTicks::Now(),
+      make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
+      false,
+      ConfigurationParams(),
+      FROM_HERE);
+  DoSyncSessionJob(cleanup_job);
+
   if (params.keystore_key_status == ConfigurationParams::KEYSTORE_KEY_NEEDED) {
     // TODO(zea): implement in such a way that we can handle failures and the
     // subsequent retrys the scheduler might perform. See crbug.com/129665.
@@ -399,6 +411,7 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
+  DCHECK_NE(job.purpose, SyncSessionJob::CLEANUP_DISABLED_TYPES);
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
@@ -431,6 +444,8 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(
 SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  if (job.purpose == SyncSessionJob::CLEANUP_DISABLED_TYPES)
+    return CONTINUE;
 
   // See if our type is throttled.
   ModelTypeSet throttled_types =
@@ -524,6 +539,8 @@ bool SyncSchedulerImpl::ShouldRunJob(const SyncSessionJob& job) {
 
 void SyncSchedulerImpl::SaveJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  // TODO(sync): Should we also check that job.purpose !=
+  // CLEANUP_DISABLED_TYPES?  (See http://crbug.com/90868.)
   if (job.purpose == SyncSessionJob::NUDGE) {
     SDVLOG(2) << "Saving a nudge job";
     InitOrCoalescePendingJob(job);
@@ -680,6 +697,10 @@ void SyncSchedulerImpl::SetSyncerStepsForPurpose(
       *start = SYNCER_BEGIN;
       *end = SYNCER_END;
       return;
+    case SyncSessionJob::CLEANUP_DISABLED_TYPES:
+      *start = CLEANUP_DISABLED_TYPES;
+      *end = CLEANUP_DISABLED_TYPES;
+      return;
     default:
       NOTREACHED();
       *start = SYNCER_END;
@@ -784,6 +805,27 @@ void SyncSchedulerImpl::DoSyncSessionJob(const SyncSessionJob& job) {
   FinishSyncSessionJob(job);
 }
 
+void SyncSchedulerImpl::UpdateCarryoverSessionState(
+    const SyncSessionJob& old_job) {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
+    // Whatever types were part of a configuration task will have had updates
+    // downloaded.  For that reason, we make sure they get recorded in the
+    // event that they get disabled at a later time.
+    ModelSafeRoutingInfo r(session_context_->previous_session_routing_info());
+    if (!r.empty()) {
+      ModelSafeRoutingInfo temp_r;
+      ModelSafeRoutingInfo old_info(old_job.session->routing_info());
+      std::set_union(r.begin(), r.end(), old_info.begin(), old_info.end(),
+          std::insert_iterator<ModelSafeRoutingInfo>(temp_r, temp_r.begin()));
+      session_context_->set_previous_session_routing_info(temp_r);
+    }
+  } else {
+    session_context_->set_previous_session_routing_info(
+        old_job.session->routing_info());
+  }
+}
+
 void SyncSchedulerImpl::FinishSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   // Update timing information for how often datatypes are triggering nudges.
@@ -813,6 +855,7 @@ void SyncSchedulerImpl::FinishSyncSessionJob(const SyncSessionJob& job) {
   ServerConnectionManager* scm = session_context_->connection_manager();
   UpdateServerConnectionManagerStatus(scm->server_status());
 
+  UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced()) {
     SDVLOG(2) << "We are currently throttled; not scheduling the next sync.";
     // TODO(sync): Investigate whether we need to check job.purpose

sync/engine/sync_scheduler_impl.h

@@ -99,8 +99,12 @@ class SyncSchedulerImpl : public SyncScheduler {
       // a sync.  The source is inferable from |session.source()|.
       NUDGE,
       // Typically used for fetching updates for a subset of the enabled types
-      // during initial sync or reconfiguration.
+      // during initial sync or reconfiguration.  We don't run all steps of
+      // the sync cycle for these (e.g. CleanupDisabledTypes is skipped).
       CONFIGURATION,
+      // The user disabled some types and we have to clean up the data
+      // for those.
+      CLEANUP_DISABLED_TYPES,
     };
     SyncSessionJob();
     SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start,
@@ -216,6 +220,10 @@ class SyncSchedulerImpl : public SyncScheduler {
   // reset our state.
   void FinishSyncSessionJob(const SyncSessionJob& job);
 
+  // Record important state that might be needed in future syncs, such as which
+  // data types may require cleanup.
+  void UpdateCarryoverSessionState(const SyncSessionJob& old_job);
+
   // Helper to FinishSyncSessionJob to schedule the next sync operation.
   void ScheduleNextSync(const SyncSessionJob& old_job);
 

sync/engine/sync_scheduler_unittest.cc

@@ -300,7 +300,9 @@ TEST_F(SyncSchedulerTest, Config) {
   SyncShareRecords records;
   const ModelTypeSet model_types(BOOKMARKS);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(),
+              SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
@@ -331,7 +333,9 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   SyncShareRecords records;
   const ModelTypeSet model_types(BOOKMARKS);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(),
+              SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
                       WithArg<0>(RecordSyncShare(&records))))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
@@ -705,14 +709,19 @@ TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) {
   EXPECT_TRUE(RunAndGetBackoff());
 }
 
-// Test that no syncing occurs when throttled.
+// Test that no syncing occurs when throttled (although CleanupDisabledTypes
+// is allowed).
 TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   const ModelTypeSet types(BOOKMARKS);
   TimeDelta poll(TimeDelta::FromMilliseconds(5));
   TimeDelta throttle(TimeDelta::FromMinutes(10));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  EXPECT_CALL(*syncer(), SyncShare(_,Not(CLEANUP_DISABLED_TYPES),
+                                     Not(CLEANUP_DISABLED_TYPES)))
       .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle)))
       .WillRepeatedly(AddFailureAndQuitLoopNow());
 
@@ -765,6 +774,7 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
   SyncShareRecords records;
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
@@ -889,6 +899,10 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
             r.snapshots[1].source().updates_source);
 
+  // Cleanup is not affected by backoff, but it should not relieve it either.
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*delay(), GetDelay(_)).Times(0);
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
@@ -1062,7 +1076,10 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  // Configuration.
+  // Configuration (always includes a cleanup disabled types).
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
@@ -1081,6 +1098,25 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
+  // Cleanup disabled types. Because no types are being configured, we just
+  // perform the cleanup.
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)).
+      WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
+
+  CallbackCounter counter2;
+  ConfigurationParams params2(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      ModelTypeSet(),
+      ModelSafeRoutingInfo(),
+      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter2)));
+  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params2));
+  ASSERT_EQ(1, counter2.times_called());
+  StopSyncScheduler();
+  Mock::VerifyAndClearExpectations(syncer());
+
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   // Poll.
@@ -1126,4 +1162,24 @@ TEST_F(SyncSchedulerTest, StartWhenNotConnected) {
   MessageLoop::current()->RunAllPending();
 }
 
+TEST_F(SyncSchedulerTest, SetsPreviousRoutingInfo) {
+  ModelSafeRoutingInfo info;
+  EXPECT_TRUE(info == context()->previous_session_routing_info());
+  ModelSafeRoutingInfo expected(context()->routing_info());
+  ASSERT_FALSE(expected.empty());
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1);
+
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  scheduler()->ScheduleNudgeAsync(
+      zero(), NUDGE_SOURCE_LOCAL, ModelTypeSet(), FROM_HERE);
+  PumpLoop();
+  // Pump again to run job.
+  PumpLoop();
+
+  StopSyncScheduler();
+
+  EXPECT_TRUE(expected == context()->previous_session_routing_info());
+}
+
 }  // namespace syncer

sync/engine/syncer.cc

@@ -12,6 +12,7 @@
 #include "build/build_config.h"
 #include "sync/engine/apply_updates_command.h"
 #include "sync/engine/build_commit_command.h"
+#include "sync/engine/cleanup_disabled_types_command.h"
 #include "sync/engine/commit.h"
 #include "sync/engine/conflict_resolver.h"
 #include "sync/engine/download_updates_command.h"
@@ -52,6 +53,7 @@ const char* SyncerStepToString(const SyncerStep step)
 {
   switch (step) {
     ENUM_CASE(SYNCER_BEGIN);
+    ENUM_CASE(CLEANUP_DISABLED_TYPES);
     ENUM_CASE(DOWNLOAD_UPDATES);
     ENUM_CASE(PROCESS_CLIENT_COMMAND);
     ENUM_CASE(VERIFY_UPDATES);
@@ -104,8 +106,14 @@ void Syncer::SyncShare(sessions::SyncSession* session,
             PruneUnthrottledTypes(base::TimeTicks::Now());
         session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
 
+        next_step = CLEANUP_DISABLED_TYPES;
+        break;
+      case CLEANUP_DISABLED_TYPES: {
+        CleanupDisabledTypesCommand cleanup;
+        cleanup.Execute(session);
         next_step = DOWNLOAD_UPDATES;
         break;
+      }
       case DOWNLOAD_UPDATES: {
         // TODO(akalin): We may want to propagate this switch up
         // eventually.

sync/engine/syncer.h

@@ -26,6 +26,7 @@ class MutableEntry;
 
 enum SyncerStep {
   SYNCER_BEGIN,
+  CLEANUP_DISABLED_TYPES,
   DOWNLOAD_UPDATES,
   PROCESS_CLIENT_COMMAND,
   VERIFY_UPDATES,

sync/internal_api/sync_manager_impl.cc

@@ -340,19 +340,6 @@ void SyncManagerImpl::ConfigureSyncer(
   DCHECK(!ready_task.is_null());
   DCHECK(!retry_task.is_null());
 
-  // Cleanup any types that might have just been disabled.
-  ModelTypeSet previous_types = ModelTypeSet::All();
-  if (!session_context_->routing_info().empty())
-    previous_types = GetRoutingInfoTypes(session_context_->routing_info());
-  if (!PurgeDisabledTypes(previous_types,
-                          GetRoutingInfoTypes(new_routing_info))) {
-    // We failed to cleanup the types. Invoke the ready task without actually
-    // configuring any types. The caller should detect this as a configuration
-    // failure and act appropriately.
-    ready_task.Run();
-    return;
-  }
-
   // TODO(zea): set this based on whether cryptographer has keystore
   // encryption key or not (requires opening a transaction). crbug.com/129665.
   ConfigurationParams::KeystoreKeyStatus keystore_key_status =
@@ -471,19 +458,7 @@ bool SyncManagerImpl::Init(
     // trigger the migration logic before the backend is initialized, resulting
     // in crashes. We therefore detect and purge any partially synced types as
     // part of initialization.
-    //
-    // Similarly, a type may have been disabled previously, but we didn't
-    // manage to purge. Ensure we cleanup any disabled types before starting up.
-    //
-    // Note: If either of these methods fail, we have directory corruption and
-    // cannot continue.
-    // TODO(rlarocque): remove the PurgeDisabledTypes call once we no longer
-    // initialize the session context with the enabled types (purging disabled
-    // types will be done within ConfigureSyncer).
-    if (!PurgePartiallySyncedTypes() ||
-        !PurgeDisabledTypes(
-            ModelTypeSet::All(),
-            GetRoutingInfoTypes(session_context_->routing_info())))
+    if (!PurgePartiallySyncedTypes())
       success = false;
 
     // Cryptographer should only be accessed while holding a
@@ -729,19 +704,6 @@ bool SyncManagerImpl::PurgePartiallySyncedTypes() {
   return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
 }
 
-bool SyncManagerImpl::PurgeDisabledTypes(
-    ModelTypeSet previously_enabled_types,
-    ModelTypeSet currently_enabled_types) {
-  ModelTypeSet disabled_types = Difference(previously_enabled_types,
-                                           currently_enabled_types);
-  if (disabled_types.Empty())
-    return true;
-
-  DVLOG(1) << "Purging disabled types "
-           << ModelTypeSetToString(disabled_types);
-  return directory()->PurgeEntriesWithTypeIn(disabled_types);
-}
-
 void SyncManagerImpl::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());

sync/internal_api/sync_manager_impl.h

@@ -180,7 +180,6 @@ class SyncManagerImpl : public SyncManager,
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, NudgeDelayTest);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnNotificationStateChange);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnIncomingNotification);
-  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, PurgeDisabledTypes);
 
   struct NotificationInfo {
     NotificationInfo();
@@ -228,11 +227,6 @@ class SyncManagerImpl : public SyncManager,
   // went wrong.
   bool SignIn(const SyncCredentials& credentials);
 
-  // Purge those types from |previously_enabled_types| that are no longer
-  // enabled in |currently_enabled_types|.
-  bool PurgeDisabledTypes(ModelTypeSet previously_enabled_types,
-                          ModelTypeSet currently_enabled_types);
-
   void RequestNudgeForDataTypes(
       const tracked_objects::Location& nudge_location,
       ModelTypeSet type);

sync/internal_api/syncapi_unittest.cc

@@ -759,7 +759,7 @@ class SyncManagerTest : public testing::Test,
                        base::MessageLoopProxy::current(),
                        scoped_ptr<HttpPostProviderFactory>(
                            new TestHttpPostProviderFactory()),
-                       ModelSafeRoutingInfo(), workers,
+                       routing_info, workers,
                        &extensions_activity_monitor_, this,
                        credentials,
                        scoped_ptr<SyncNotifier>(sync_notifier_mock_),
@@ -919,22 +919,6 @@ class SyncManagerTest : public testing::Test,
         REMOTE_NOTIFICATION);
   }
 
-  void SetProgressMarkerForType(ModelType type, bool set) {
-    if (set) {
-      sync_pb::DataTypeProgressMarker marker;
-      marker.set_token("token");
-      marker.set_data_type_id(GetSpecificsFieldNumberFromModelType(type));
-      sync_manager_.directory()->SetDownloadProgress(type, marker);
-    } else {
-      sync_pb::DataTypeProgressMarker marker;
-      sync_manager_.directory()->SetDownloadProgress(type, marker);
-    }
-  }
-
-  void SetInitialSyncEndedForType(ModelType type, bool value) {
-    sync_manager_.directory()->set_initial_sync_ended_for_type(type, value);
-  }
-
  private:
   // Needed by |sync_manager_|.
   MessageLoop message_loop_;
@@ -2518,24 +2502,18 @@ class MockSyncScheduler : public FakeSyncScheduler {
 
 class ComponentsFactory : public TestInternalComponentsFactory {
  public:
-  ComponentsFactory(SyncScheduler* scheduler_to_use,
-                    sessions::SyncSessionContext** session_context)
+  ComponentsFactory(SyncScheduler* scheduler_to_use)
       : TestInternalComponentsFactory(
             TestInternalComponentsFactory::IN_MEMORY),
-        scheduler_to_use_(scheduler_to_use),
-        session_context_(session_context) {}
+        scheduler_to_use_(scheduler_to_use) {}
   virtual ~ComponentsFactory() {}
 
   virtual scoped_ptr<SyncScheduler> BuildScheduler(
       const std::string& name,
       sessions::SyncSessionContext* context) OVERRIDE {
-    *session_context_ = context;
     return scheduler_to_use_.Pass();
   }
-
- private:
   scoped_ptr<SyncScheduler> scheduler_to_use_;
-  sessions::SyncSessionContext** session_context_;
 };
 
 class SyncManagerTestWithMockScheduler : public SyncManagerTest {
@@ -2543,42 +2521,24 @@ class SyncManagerTestWithMockScheduler : public SyncManagerTest {
   SyncManagerTestWithMockScheduler() : scheduler_(NULL) {}
   virtual InternalComponentsFactory* GetFactory() OVERRIDE {
     scheduler_ = new MockSyncScheduler();
-    return new ComponentsFactory(scheduler_, &session_context_);
-  }
-
-  MockSyncScheduler* scheduler() { return scheduler_; }
-  sessions::SyncSessionContext* session_context() {
-      return session_context_;
+    return new ComponentsFactory(scheduler_);
   }
-
- private:
   MockSyncScheduler* scheduler_;
-  sessions::SyncSessionContext* session_context_;
 };
 
 // Test that the configuration params are properly created and sent to
-// ScheduleConfigure. No callback should be invoked. Any disabled datatypes
-// should be purged.
+// ScheduleConfigure. No callback should be invoked.
 TEST_F(SyncManagerTestWithMockScheduler, BasicConfiguration) {
   ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
   ModelTypeSet types_to_download(BOOKMARKS, PREFERENCES);
   ModelSafeRoutingInfo new_routing_info;
   GetModelSafeRoutingInfo(&new_routing_info);
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info);
-  ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
 
   ConfigurationParams params;
-  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
+  EXPECT_CALL(*scheduler_, Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler_, ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
 
-  // Set data for all types.
-  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
-       iter.Inc()) {
-    SetProgressMarkerForType(iter.Get(), true);
-    SetInitialSyncEndedForType(iter.Get(), true);
-  }
-
   CallbackCounter ready_task_counter, retry_task_counter;
   sync_manager_.ConfigureSyncer(
       reason,
@@ -2594,70 +2554,6 @@ TEST_F(SyncManagerTestWithMockScheduler, BasicConfiguration) {
             params.source);
   EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
   EXPECT_EQ(new_routing_info, params.routing_info);
-
-  // Verify all the disabled types were purged.
-  EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
-      enabled_types));
-  EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
-      ModelTypeSet::All()).Equals(disabled_types));
-}
-
-// Test that on a reconfiguration (configuration where the session context
-// already has routing info), only those recently disabled types are purged.
-TEST_F(SyncManagerTestWithMockScheduler, ReConfiguration) {
-  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
-  ModelTypeSet types_to_download(BOOKMARKS, PREFERENCES);
-  ModelTypeSet disabled_types = ModelTypeSet(THEMES, SESSIONS);
-  ModelSafeRoutingInfo old_routing_info;
-  ModelSafeRoutingInfo new_routing_info;
-  GetModelSafeRoutingInfo(&old_routing_info);
-  new_routing_info = old_routing_info;
-  new_routing_info.erase(THEMES);
-  new_routing_info.erase(SESSIONS);
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info);
-
-  ConfigurationParams params;
-  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
-      WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
-
-  // Set data for all types except those recently disabled (so we can verify
-  // only those recently disabled are purged) .
-  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
-       iter.Inc()) {
-    if (!disabled_types.Has(iter.Get())) {
-      SetProgressMarkerForType(iter.Get(), true);
-      SetInitialSyncEndedForType(iter.Get(), true);
-    } else {
-      SetProgressMarkerForType(iter.Get(), false);
-      SetInitialSyncEndedForType(iter.Get(), false);
-    }
-  }
-
-  // Set the context to have the old routing info.
-  session_context()->set_routing_info(old_routing_info);
-
-  CallbackCounter ready_task_counter, retry_task_counter;
-  sync_manager_.ConfigureSyncer(
-      reason,
-      types_to_download,
-      new_routing_info,
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&ready_task_counter)),
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&retry_task_counter)));
-  EXPECT_EQ(0, ready_task_counter.times_called());
-  EXPECT_EQ(0, retry_task_counter.times_called());
-  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
-            params.source);
-  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
-  EXPECT_EQ(new_routing_info, params.routing_info);
-
-  // Verify only the recently disabled types were purged.
-  EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
-      Difference(ModelTypeSet::All(), disabled_types)));
-  EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
-      ModelTypeSet::All()).Equals(disabled_types));
 }
 
 // Test that the retry callback is invoked on configuration failure.
@@ -2668,8 +2564,8 @@ TEST_F(SyncManagerTestWithMockScheduler, ConfigurationRetry) {
   GetModelSafeRoutingInfo(&new_routing_info);
 
   ConfigurationParams params;
-  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
+  EXPECT_CALL(*scheduler_, Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler_, ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(false)));
 
   CallbackCounter ready_task_counter, retry_task_counter;
@@ -2731,44 +2627,4 @@ TEST_F(SyncManagerTest, PurgePartiallySyncedTypes) {
   EXPECT_FALSE(partial_types.Has(PREFERENCES));
 }
 
-// Test CleanipDisabledTypes properly purges all disabled types as specified
-// by the previous and current enabled params. Enabled partial types should not
-// be purged.
-TEST_F(SyncManagerTest, PurgeDisabledTypes) {
-  ModelSafeRoutingInfo routing_info;
-  GetModelSafeRoutingInfo(&routing_info);
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
-  ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
-  ModelTypeSet partial_enabled_types(PASSWORDS);
-
-  // Set data for all non-partial types.
-  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
-       iter.Inc()) {
-    SetProgressMarkerForType(iter.Get(), true);
-    if (!partial_enabled_types.Has(iter.Get()))
-      SetInitialSyncEndedForType(iter.Get(), true);
-  }
-
-  // Verify all the enabled types remain after cleanup, and all the disabled
-  // types were purged.
-  sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types);
-  EXPECT_TRUE(enabled_types.Equals(
-      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
-  EXPECT_TRUE(disabled_types.Equals(
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
-
-  // Disable some more types.
-  disabled_types.Put(BOOKMARKS);
-  disabled_types.Put(PREFERENCES);
-  ModelTypeSet new_enabled_types =
-      Difference(ModelTypeSet::All(), disabled_types);
-
-  // Verify only the non-disabled types remain after cleanup.
-  sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types);
-  EXPECT_TRUE(new_enabled_types.Equals(
-      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
-  EXPECT_TRUE(disabled_types.Equals(
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
-}
-
 }  // namespace

sync/sessions/sync_session_context.h

@@ -110,6 +110,14 @@ class SyncSessionContext {
   }
   int32 max_commit_batch_size() const { return max_commit_batch_size_; }
 
+  const ModelSafeRoutingInfo& previous_session_routing_info() const {
+    return previous_session_routing_info_;
+  }
+
+  void set_previous_session_routing_info(const ModelSafeRoutingInfo& info) {
+    previous_session_routing_info_ = info;
+  }
+
   void NotifyListeners(const SyncEngineEvent& event) {
     FOR_EACH_OBSERVER(SyncEngineEventListener, listeners_,
                       OnSyncEngineEvent(event));
@@ -155,6 +163,10 @@ class SyncSessionContext {
   // The server limits the number of items a client can commit in one batch.
   int max_commit_batch_size_;
 
+  // Some routing info history to help us clean up types that get disabled
+  // by the user.
+  ModelSafeRoutingInfo previous_session_routing_info_;
+
   ThrottledDataTypeTracker* throttled_data_type_tracker_;
 
   // We use this to get debug info to send to the server for debugging

sync/sync.gyp

@@ -76,6 +76,8 @@
         'engine/apply_updates_command.h',
         'engine/build_commit_command.cc',
         'engine/build_commit_command.h',
+        'engine/cleanup_disabled_types_command.cc',
+        'engine/cleanup_disabled_types_command.h',
         'engine/commit.cc',
         'engine/commit.h',
         'engine/conflict_resolver.cc',
@@ -559,6 +561,7 @@
           'internal_api/public/util/immutable_unittest.cc',
           'engine/apply_updates_command_unittest.cc',
           'engine/build_commit_command_unittest.cc',
+          'engine/cleanup_disabled_types_command_unittest.cc',
           'engine/download_updates_command_unittest.cc',
           'engine/model_changing_syncer_command_unittest.cc',
           'engine/process_commit_response_command_unittest.cc',