Copy entries of broken (having unrecoverable error) data types to delete journals before purging.

BUG=121928


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

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

11 files changed, 103 insertions, 21 deletions

diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 669bda2..0224709 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -41,6 +41,7 @@
 #include "sync/sessions/sync_session_context.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/nigori_util.h"
+#include "sync/syncable/syncable_delete_journal.h"
 #include "sync/syncable/syncable_read_transaction.h"
 #include "sync/syncable/syncable_util.h"
 #include "sync/syncable/syncable_write_transaction.h"
@@ -1041,7 +1042,8 @@ TEST_F(SyncerTest, TestPurgeWhileUnsynced) {
     parent2.Put(syncable::ID, pref_node_id);
   }
 
-  directory()->PurgeEntriesWithTypeIn(ModelTypeSet(PREFERENCES));
+  directory()->PurgeEntriesWithTypeIn(ModelTypeSet(PREFERENCES),
+                                      ModelTypeSet());
 
   SyncShareNudge();
   ASSERT_EQ(2U, mock_server_->committed_ids().size());
@@ -1076,7 +1078,7 @@ TEST_F(SyncerTest, TestPurgeWhileUnapplied) {
   }
 
   directory()->PurgeEntriesWithTypeIn(
-      ModelTypeSet(BOOKMARKS));
+      ModelTypeSet(BOOKMARKS), ModelTypeSet());
 
   SyncShareNudge();
   directory()->SaveChanges();
@@ -1087,6 +1089,48 @@ TEST_F(SyncerTest, TestPurgeWhileUnapplied) {
   }
 }
 
+TEST_F(SyncerTest, TestPurgeWithJournal) {
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+    MutableEntry parent(&wtrans, syncable::CREATE, BOOKMARKS, wtrans.root_id(),
+                        "Pete");
+    ASSERT_TRUE(parent.good());
+    parent.Put(syncable::IS_DIR, true);
+    parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
+    parent.Put(syncable::BASE_VERSION, 1);
+    parent.Put(syncable::ID, parent_id_);
+    MutableEntry child(&wtrans, syncable::CREATE, BOOKMARKS, parent_id_,
+                       "Pete");
+    ASSERT_TRUE(child.good());
+    child.Put(syncable::ID, child_id_);
+    child.Put(syncable::BASE_VERSION, 1);
+    WriteTestDataToEntry(&wtrans, &child);
+
+    MutableEntry parent2(&wtrans, syncable::CREATE, PREFERENCES,
+                         wtrans.root_id(), "Tim");
+    ASSERT_TRUE(parent2.good());
+    parent2.Put(syncable::IS_DIR, true);
+    parent2.Put(syncable::SPECIFICS, DefaultPreferencesSpecifics());
+    parent2.Put(syncable::BASE_VERSION, 1);
+    parent2.Put(syncable::ID, TestIdFactory::MakeServer("Tim"));
+  }
+
+  directory()->PurgeEntriesWithTypeIn(ModelTypeSet(PREFERENCES, BOOKMARKS),
+                                      ModelTypeSet(BOOKMARKS));
+  {
+    // Verify bookmark nodes are saved in delete journal but not preference
+    // node.
+    syncable::ReadTransaction rt(FROM_HERE, directory());
+    syncable::DeleteJournal* delete_journal = directory()->delete_journal();
+    EXPECT_EQ(2u, delete_journal->GetDeleteJournalSize(&rt));
+    syncable::EntryKernelSet journal_entries;
+    directory()->delete_journal()->GetDeleteJournals(&rt, BOOKMARKS,
+                                                     &journal_entries);
+    EXPECT_EQ(parent_id_, (*journal_entries.begin())->ref(syncable::ID));
+    EXPECT_EQ(child_id_, (*journal_entries.rbegin())->ref(syncable::ID));
+  }
+}
+
 TEST_F(SyncerTest, TestCommitListOrderingTwoItemsTall) {
   CommitOrderingTest items[] = {
     {1, ids_.FromNumber(-1001), ids_.FromNumber(-1000)},
diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index 9ae1940..d7a5ce2 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -362,6 +362,9 @@ class SYNC_EXPORT SyncManager {
   // any configuration tasks needed as determined by the params. Once complete,
   // syncer will remain in CONFIGURATION_MODE until StartSyncingNormally is
   // called.
+  // Data whose types are not in |new_routing_info| are purged from sync
+  // directory. The purged data is backed up in delete journal for recovery in
+  // next session if its type is in |failed_types|.
   // |ready_task| is invoked when the configuration completes.
   // |retry_task| is invoked if the configuration job could not immediately
   //              execute. |ready_task| will still be called when it eventually
@@ -369,6 +372,7 @@ class SYNC_EXPORT SyncManager {
   virtual void ConfigureSyncer(
       ConfigureReason reason,
       ModelTypeSet types_to_config,
+      ModelTypeSet failed_types,
       const ModelSafeRoutingInfo& new_routing_info,
       const base::Closure& ready_task,
       const base::Closure& retry_task) = 0;
diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h
index 88e6360..ecbf3be 100644
--- a/sync/internal_api/public/test/fake_sync_manager.h
+++ b/sync/internal_api/public/test/fake_sync_manager.h
@@ -105,6 +105,7 @@ class FakeSyncManager : public SyncManager {
   virtual void ConfigureSyncer(
       ConfigureReason reason,
       ModelTypeSet types_to_config,
+      ModelTypeSet failed_types,
       const ModelSafeRoutingInfo& new_routing_info,
       const base::Closure& ready_task,
       const base::Closure& retry_task) OVERRIDE;
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 874444b..72f096f 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -297,6 +297,7 @@ ModelTypeSet SyncManagerImpl::GetTypesWithEmptyProgressMarkerToken(
 void SyncManagerImpl::ConfigureSyncer(
     ConfigureReason reason,
     ModelTypeSet types_to_config,
+    ModelTypeSet failed_types,
     const ModelSafeRoutingInfo& new_routing_info,
     const base::Closure& ready_task,
     const base::Closure& retry_task) {
@@ -309,7 +310,8 @@ void SyncManagerImpl::ConfigureSyncer(
   if (!session_context_->routing_info().empty())
     previous_types = GetRoutingInfoTypes(session_context_->routing_info());
   if (!PurgeDisabledTypes(previous_types,
-                          GetRoutingInfoTypes(new_routing_info))) {
+                          GetRoutingInfoTypes(new_routing_info),
+                          failed_types)) {
     // 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.
@@ -578,12 +580,14 @@ bool SyncManagerImpl::PurgePartiallySyncedTypes() {
                        partially_synced_types.Size());
   if (partially_synced_types.Empty())
     return true;
-  return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
+  return directory()->PurgeEntriesWithTypeIn(partially_synced_types,
+                                             ModelTypeSet());
 }
 
 bool SyncManagerImpl::PurgeDisabledTypes(
     ModelTypeSet previously_enabled_types,
-    ModelTypeSet currently_enabled_types) {
+    ModelTypeSet currently_enabled_types,
+    ModelTypeSet failed_types) {
   ModelTypeSet disabled_types = Difference(previously_enabled_types,
                                            currently_enabled_types);
   if (disabled_types.Empty())
@@ -591,7 +595,7 @@ bool SyncManagerImpl::PurgeDisabledTypes(
 
   DVLOG(1) << "Purging disabled types "
            << ModelTypeSetToString(disabled_types);
-  return directory()->PurgeEntriesWithTypeIn(disabled_types);
+  return directory()->PurgeEntriesWithTypeIn(disabled_types, failed_types);
 }
 
 void SyncManagerImpl::UpdateCredentials(
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index 8942201..bbcf794 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -101,6 +101,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl :
   virtual void ConfigureSyncer(
       ConfigureReason reason,
       ModelTypeSet types_to_config,
+      ModelTypeSet failed_types,
       const ModelSafeRoutingInfo& new_routing_info,
       const base::Closure& ready_task,
       const base::Closure& retry_task) OVERRIDE;
@@ -238,7 +239,8 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl :
   // 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);
+                          ModelTypeSet currently_enabled_types,
+                          ModelTypeSet failed_types);
 
   void RequestNudgeForDataTypes(
       const tracked_objects::Location& nudge_location,
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index aed4432..b796679 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -2833,6 +2833,7 @@ TEST_F(SyncManagerTestWithMockScheduler, MAYBE_BasicConfiguration) {
   sync_manager_.ConfigureSyncer(
       reason,
       types_to_download,
+      ModelTypeSet(),
       new_routing_info,
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&ready_task_counter)),
@@ -2889,6 +2890,7 @@ TEST_F(SyncManagerTestWithMockScheduler, ReConfiguration) {
   sync_manager_.ConfigureSyncer(
       reason,
       types_to_download,
+      ModelTypeSet(),
       new_routing_info,
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&ready_task_counter)),
@@ -2922,6 +2924,7 @@ TEST_F(SyncManagerTestWithMockScheduler, ConfigurationRetry) {
   sync_manager_.ConfigureSyncer(
       reason,
       types_to_download,
+      ModelTypeSet(),
       new_routing_info,
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&ready_task_counter)),
@@ -3033,7 +3036,8 @@ TEST_F(SyncManagerTest, MAYBE_PurgeDisabledTypes) {
 
   // Verify all the enabled types remain after cleanup, and all the disabled
   // types were purged.
-  sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types);
+  sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types,
+                                   ModelTypeSet());
   EXPECT_TRUE(enabled_types.Equals(sync_manager_.InitialSyncEndedTypes()));
   EXPECT_TRUE(disabled_types.Equals(
       sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
@@ -3045,7 +3049,8 @@ TEST_F(SyncManagerTest, MAYBE_PurgeDisabledTypes) {
       Difference(ModelTypeSet::All(), disabled_types);
 
   // Verify only the non-disabled types remain after cleanup.
-  sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types);
+  sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types,
+                                   ModelTypeSet());
   EXPECT_TRUE(new_enabled_types.Equals(sync_manager_.InitialSyncEndedTypes()));
   EXPECT_TRUE(disabled_types.Equals(
       sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc
index 0556cc9..e07c8e7 100644
--- a/sync/internal_api/test/fake_sync_manager.cc
+++ b/sync/internal_api/test/fake_sync_manager.cc
@@ -181,6 +181,7 @@ void FakeSyncManager::StartSyncingNormally(
 void FakeSyncManager::ConfigureSyncer(
     ConfigureReason reason,
     ModelTypeSet types_to_config,
+    ModelTypeSet failed_types,
     const ModelSafeRoutingInfo& new_routing_info,
     const base::Closure& ready_task,
     const base::Closure& retry_task) {
@@ -196,7 +197,7 @@ void FakeSyncManager::ConfigureSyncer(
 
   // Update our fake directory by clearing and fake-downloading as necessary.
   UserShare* share = GetUserShare();
-  share->directory->PurgeEntriesWithTypeIn(disabled_types);
+  share->directory->PurgeEntriesWithTypeIn(disabled_types, ModelTypeSet());
   for (ModelTypeSet::Iterator it = success_types.First(); it.Good(); it.Inc()) {
     // We must be careful to not create the same root node twice.
     if (!initial_sync_ended_types_.Has(it.Get())) {
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 69edfb0..7b58364 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -561,12 +561,19 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
   return true;
 }
 
-bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types) {
+bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types,
+                                       ModelTypeSet types_to_journal) {
+  DCHECK(types.HasAll(types_to_journal));
+
   if (types.Empty())
     return true;
 
   {
     WriteTransaction trans(FROM_HERE, PURGE_ENTRIES, this);
+
+    EntryKernelSet entries_to_journal;
+    STLElementDeleter<EntryKernelSet> journal_deleter(&entries_to_journal);
+
     {
       ScopedKernelLock lock(this);
       MetahandlesIndex::iterator it = kernel_->metahandles_index->begin();
@@ -600,12 +607,22 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types) {
           num_erased = kernel_->parent_id_child_index->erase(entry);
           DCHECK_EQ(entry->ref(IS_DEL), !num_erased);
           kernel_->metahandles_index->erase(it++);
-          delete entry;
+
+          if ((types_to_journal.Has(local_type) ||
+              types_to_journal.Has(server_type)) &&
+              (delete_journal_->IsDeleteJournalEnabled(local_type) ||
+                  delete_journal_->IsDeleteJournalEnabled(server_type))) {
+            entries_to_journal.insert(entry);
+          } else {
+            delete entry;
+          }
         } else {
           ++it;
         }
       }
 
+      delete_journal_->AddJournalBatch(&trans, entries_to_journal);
+
       // Ensure meta tracking for these data types reflects the deleted state.
       for (ModelTypeSet::Iterator it = types.First();
            it.Good(); it.Inc()) {
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index 6aa10b2..f55ba4e 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -442,14 +442,18 @@ class SYNC_EXPORT Directory {
   bool FullyCheckTreeInvariants(BaseTransaction *trans);
 
   // Purges all data associated with any entries whose ModelType or
-  // ServerModelType is found in |types|, from _both_ memory and disk.
-  // Only  valid, "real" model types are allowed in |types| (see model_type.h
-  // for definitions).  "Purge" is just meant to distinguish from "deleting"
+  // ServerModelType is found in |types|, from sync directory _both_ in memory
+  // and on disk. |types_to_journal| should be subset of |types| and data
+  // of |types_to_journal| are saved in delete journal to help prevent
+  // back-from-dead problem due to offline delete in next sync session. Only
+  // valid, "real" model types are allowed in |types| (see model_type.h for
+  // definitions).  "Purge" is just meant to distinguish from "deleting"
   // entries, which means something different in the syncable namespace.
   // WARNING! This can be real slow, as it iterates over all entries.
   // WARNING! Performs synchronous I/O.
   // Returns: true on success, false if an error was encountered.
-  virtual bool PurgeEntriesWithTypeIn(ModelTypeSet types);
+  virtual bool PurgeEntriesWithTypeIn(ModelTypeSet types,
+                                      ModelTypeSet types_to_journal);
 
  private:
   // A helper that implements the logic of checking tree invariants.
diff --git a/sync/syncable/syncable_mock.h b/sync/syncable/syncable_mock.h
index 089bd31..55068ed 100644
--- a/sync/syncable/syncable_mock.h
+++ b/sync/syncable/syncable_mock.h
@@ -28,7 +28,7 @@ class MockDirectory : public Directory {
   MOCK_METHOD1(GetEntryByClientTag,
                syncable::EntryKernel*(const std::string&));
 
-  MOCK_METHOD1(PurgeEntriesWithTypeIn, bool(ModelTypeSet));
+  MOCK_METHOD2(PurgeEntriesWithTypeIn, bool(ModelTypeSet, ModelTypeSet));
 
  private:
   syncable::NullDirectoryChangeDelegate delegate_;
diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc
index 444e9e3..b6f767a 100644
--- a/sync/syncable/syncable_unittest.cc
+++ b/sync/syncable/syncable_unittest.cc
@@ -561,7 +561,7 @@ TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) {
   }
 
   ModelTypeSet to_purge(BOOKMARKS);
-  dir_->PurgeEntriesWithTypeIn(to_purge);
+  dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet());
 
   Directory::SaveChangesSnapshot snapshot1;
   base::AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
@@ -570,7 +570,7 @@ TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) {
 
   to_purge.Clear();
   to_purge.Put(PREFERENCES);
-  dir_->PurgeEntriesWithTypeIn(to_purge);
+  dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet());
 
   dir_->HandleSaveChangesFailure(snapshot1);
 
@@ -1738,7 +1738,7 @@ TEST_F(OnDiskSyncableDirectoryTest, TestPurgeEntriesWithTypeIn) {
     ASSERT_EQ(10U, all_set.size());
   }
 
-  dir_->PurgeEntriesWithTypeIn(types_to_purge);
+  dir_->PurgeEntriesWithTypeIn(types_to_purge, ModelTypeSet());
 
   // We first query the in-memory data, and then reload the directory (without
   // saving) to verify that disk does not still have the data.
@@ -1999,7 +1999,7 @@ TEST_F(OnDiskSyncableDirectoryTest, TestSaveChangesFailureWithPurge) {
   ASSERT_TRUE(dir_->good());
 
   ModelTypeSet set(BOOKMARKS);
-  dir_->PurgeEntriesWithTypeIn(set);
+  dir_->PurgeEntriesWithTypeIn(set, ModelTypeSet());
   EXPECT_TRUE(IsInMetahandlesToPurge(handle1));
   ASSERT_FALSE(dir_->SaveChanges());
   EXPECT_TRUE(IsInMetahandlesToPurge(handle1));