sync: Change entry purging approach to rely on SaveChanges, which is more consistent with the way

things work in general, and specifically when things fail as the transaction boundaries are well
defined.

The entries will get deleted from memory after the next successful SaveChanges operation (happens every 10 seconds), and they will get deleted from disk on the next browser restart.

Also clear the metadata bits that track whether there are any entries in the Directory for those
types to keep it in a consistent state.

BUG=40252

Review URL: http://codereview.chromium.org/2873020

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

6 files changed, 91 insertions, 131 deletions

diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index 26bb31f..2a29789 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -581,35 +581,6 @@ bool DirectoryBackingStore::DeleteExtendedAttributeFromDB(
   return true;
 }
 
-bool DirectoryBackingStore::DeleteEntries(const MetahandleSet& handles) {
-  if (handles.empty())
-    return true;
-
-  sqlite3* db_handle = NULL;
-  if (!OpenAndConfigureHandleHelper(&db_handle))
-    return false;
-
-  sqlite_utils::scoped_sqlite_db_ptr scoped_handle(db_handle);
-  string query = "DELETE FROM metas WHERE metahandle IN (";
-  for (MetahandleSet::const_iterator it = handles.begin(); it != handles.end();
-       ++it) {
-    if (it != handles.begin())
-      query.append(",");
-    query.append(Int64ToString(*it));
-  }
-  query.append(")");
-  SQLStatement statement;
-  int result = statement.prepare(scoped_handle.get(), query.data(),
-                                 query.size());
-  if (SQLITE_OK == result) {
-    result = statement.step();
-    if (SQLITE_DONE == result)
-      statement.finalize();
-  }
-
-  return SQLITE_DONE == result;
-}
-
 bool DirectoryBackingStore::DropDeletedEntries() {
   static const char delete_extended_attributes[] =
       "DELETE FROM extended_attributes WHERE metahandle IN "
diff --git a/chrome/browser/sync/syncable/directory_backing_store.h b/chrome/browser/sync/syncable/directory_backing_store.h
index b240ed7..b492e5d 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.h
+++ b/chrome/browser/sync/syncable/directory_backing_store.h
@@ -75,10 +75,6 @@ class DirectoryBackingStore {
   // calls SaveChanges *must* be the thread that owns/destroys |this|.
   virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot);
 
-  // Removes each entry whose metahandle is in |handles| from the database.
-  // Does synchronous I/O.  Returns false on error.
-  bool DeleteEntries(const MetahandleSet& handles);
-
  private:
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, MigrateVersion67To68);
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, MigrateVersion68To69);
diff --git a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
index 971f170..0daed91 100644
--- a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
@@ -906,48 +906,4 @@ TEST_F(DirectoryBackingStoreTest, Corruption) {
   }
 }
 
-TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
-  SetUpCurrentDatabaseAndCheckVersion();
-  scoped_ptr<DirectoryBackingStore> dbs(
-      new DirectoryBackingStore(GetUsername(), GetDatabasePath()));
-  dbs->BeginLoad();
-
-  MetahandlesIndex index;
-  dbs->LoadEntries(&index);
-  size_t initial_size = index.size();
-  ASSERT_LT(0U, initial_size) << "Test requires entries to delete.";
-  int64 first_to_die = (*index.begin())->ref(META_HANDLE);
-  MetahandleSet to_delete;
-  to_delete.insert(first_to_die);
-  EXPECT_TRUE(dbs->DeleteEntries(to_delete));
-
-  index.clear();
-  dbs->LoadEntries(&index);
-
-  EXPECT_EQ(initial_size - 1, index.size());
-  bool delete_failed = false;
-  for (MetahandlesIndex::iterator it = index.begin(); it != index.end();
-       ++it) {
-    if ((*it)->ref(META_HANDLE) == first_to_die) {
-      delete_failed = true;
-      break;
-    }
-  }
-  EXPECT_FALSE(delete_failed);
-
-  to_delete.clear();
-  for (MetahandlesIndex::iterator it = index.begin(); it != index.end();
-       ++it) {
-    to_delete.insert((*it)->ref(META_HANDLE));
-  }
-
-  EXPECT_TRUE(dbs->DeleteEntries(to_delete));
-
-  index.clear();
-  dbs->LoadEntries(&index);
-  EXPECT_EQ(0U, index.size());
-
-  dbs->EndLoad();
-}
-
 }  // namespace syncable
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index a8133c9..17176fd 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -611,38 +611,41 @@ void Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
 }
 
 void Directory::PurgeEntriesWithTypeIn(const std::set<ModelType>& types) {
-  DCHECK_EQ(0U, types.count(UNSPECIFIED));
-  DCHECK_EQ(0U, types.count(TOP_LEVEL_FOLDER));
+  if (types.count(UNSPECIFIED) != 0U || types.count(TOP_LEVEL_FOLDER) != 0U) {
+    NOTREACHED() << "Don't support purging unspecified or top level entries.";
+    return;
+  }
 
   if (types.empty())
     return;
 
-  ScopedKernelLock lock(this);
-  MetahandleSet to_delete;
-  MetahandlesIndex::iterator it = kernel_->metahandles_index->begin();
-  while (it != kernel_->metahandles_index->end()) {
-    const sync_pb::EntitySpecifics& local_specifics = (*it)->ref(SPECIFICS);
-    const sync_pb::EntitySpecifics& server_specifics =
-        (*it)->ref(SERVER_SPECIFICS);
-    ModelType local_type = GetModelTypeFromSpecifics(local_specifics);
-    ModelType server_type = GetModelTypeFromSpecifics(server_specifics);
-
-    // Note the dance around incrementing |it|, since we sometimes use erase().
-    if (types.count(local_type) > 0 || types.count(server_type) > 0) {
-      to_delete.insert((*it)->ref(META_HANDLE));
-      size_t num_erased = 0;
-      num_erased = kernel_->ids_index->erase(*it);
-      DCHECK_EQ(1u, num_erased);
-      num_erased = kernel_->client_tag_index->erase(*it);
-      DCHECK_EQ((*it)->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
-      kernel_->metahandles_index->erase(it++);
-    } else {
-      ++it;
+  {
+    ScopedKernelLock lock(this);
+    for (MetahandlesIndex::iterator it = kernel_->metahandles_index->begin();
+         it != kernel_->metahandles_index->end(); ++it) {
+      const sync_pb::EntitySpecifics& local_specifics = (*it)->ref(SPECIFICS);
+      const sync_pb::EntitySpecifics& server_specifics =
+          (*it)->ref(SERVER_SPECIFICS);
+      ModelType local_type = GetModelTypeFromSpecifics(local_specifics);
+      ModelType server_type = GetModelTypeFromSpecifics(server_specifics);
+
+      if (types.count(local_type) > 0 || types.count(server_type) > 0) {
+        // Set conditions for permanent deletion.
+        (*it)->put(IS_DEL, true);
+        (*it)->put(IS_UNSYNCED, false);
+        (*it)->put(IS_UNAPPLIED_UPDATE, false);
+        (*it)->mark_dirty(kernel_->dirty_metahandles);
+        DCHECK(!SafeToPurgeFromMemory(*it));
+      }
     }
-  }
 
-  // Synchronously wipe these entries from disk.
-  store_->DeleteEntries(to_delete);
+    // Ensure meta tracking for these data types reflects the deleted state.
+    for (std::set<ModelType>::const_iterator it = types.begin();
+         it != types.end(); ++it) {
+      set_initial_sync_ended_for_type_unsafe(*it, false);
+      set_last_download_timestamp_unsafe(*it, 0);
+    }
+  }
 }
 
 void Directory::HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot) {
@@ -682,10 +685,7 @@ int64 Directory::last_download_timestamp(ModelType model_type) const {
 void Directory::set_last_download_timestamp(ModelType model_type,
     int64 timestamp) {
   ScopedKernelLock lock(this);
-  if (kernel_->persisted_info.last_download_timestamp[model_type] == timestamp)
-    return;
-  kernel_->persisted_info.last_download_timestamp[model_type] = timestamp;
-  kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
+  set_last_download_timestamp_unsafe(model_type, timestamp);
 }
 
 bool Directory::initial_sync_ended_for_type(ModelType type) const {
@@ -695,12 +695,25 @@ bool Directory::initial_sync_ended_for_type(ModelType type) const {
 
 void Directory::set_initial_sync_ended_for_type(ModelType type, bool x) {
   ScopedKernelLock lock(this);
+  set_initial_sync_ended_for_type_unsafe(type, x);
+}
+
+void Directory::set_initial_sync_ended_for_type_unsafe(ModelType type,
+                                                       bool x) {
   if (kernel_->persisted_info.initial_sync_ended[type] == x)
     return;
   kernel_->persisted_info.initial_sync_ended.set(type, x);
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+void Directory::set_last_download_timestamp_unsafe(ModelType model_type,
+                                                   int64 timestamp) {
+  if (kernel_->persisted_info.last_download_timestamp[model_type] == timestamp)
+    return;
+  kernel_->persisted_info.last_download_timestamp[model_type] = timestamp;
+  kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
+}
+
 string Directory::store_birthday() const {
   ScopedKernelLock lock(this);
   return kernel_->persisted_info.store_birthday;
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index bbc0e52..c3ad2e9 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -936,6 +936,12 @@ class Directory {
                              BaseTransaction* trans,
                              const Id& parent_id);
 
+  // Internal setters that do not acquire a lock internally.  These are unsafe
+  // on their own; caller must guarantee exclusive access manually by holding
+  // a ScopedKernelLock.
+  void set_initial_sync_ended_for_type_unsafe(ModelType type, bool x);
+  void set_last_download_timestamp_unsafe(ModelType model_type, int64 x);
+
   Directory& operator = (const Directory&);
 
   // TODO(sync):  If lookups and inserts in these sets become
diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc
index bc22a0c..43bb5a9 100644
--- a/chrome/browser/sync/syncable/syncable_unittest.cc
+++ b/chrome/browser/sync/syncable/syncable_unittest.cc
@@ -301,6 +301,38 @@ class SyncableDirectoryTest : public testing::Test {
     return 1 == dir_->kernel_->dirty_metahandles->count(metahandle);
   }
 
+  bool IsSafeToPermanentlyDelete(Entry* e) {
+    return e->Get(IS_DEL) && !e->Get(IS_UNSYNCED) &&
+           !e->Get(IS_UNAPPLIED_UPDATE);
+  }
+
+  void CheckPurgeEntriesWithTypeInSucceeded(const ModelTypeSet& types_to_purge,
+                                            bool before_reload) {
+    {
+      ReadTransaction trans(dir_.get(), __FILE__, __LINE__);
+      MetahandleSet all_set;
+      dir_->GetAllMetaHandles(&trans, &all_set);
+      EXPECT_EQ(before_reload ? 7U : 3U, all_set.size());
+      for (MetahandleSet::iterator iter = all_set.begin();
+           iter != all_set.end(); ++iter) {
+        Entry e(&trans, GET_BY_HANDLE, *iter);
+        if ((types_to_purge.count(e.GetModelType()) ||
+             types_to_purge.count(e.GetServerModelType())) &&
+             (!before_reload || !IsSafeToPermanentlyDelete(&e))) {
+          FAIL() << "Illegal type should have been deleted.";
+        }
+      }
+    }
+
+    EXPECT_FALSE(dir_->initial_sync_ended_for_type(PREFERENCES));
+    EXPECT_FALSE(dir_->initial_sync_ended_for_type(AUTOFILL));
+    EXPECT_TRUE(dir_->initial_sync_ended_for_type(BOOKMARKS));
+
+    EXPECT_EQ(0, dir_->last_download_timestamp(PREFERENCES));
+    EXPECT_EQ(0, dir_->last_download_timestamp(AUTOFILL));
+    EXPECT_EQ(1, dir_->last_download_timestamp(BOOKMARKS));
+  }
+
   scoped_ptr<Directory> dir_;
   FilePath file_path_;
 
@@ -395,6 +427,13 @@ TEST_F(SyncableDirectoryTest, TestPurgeEntriesWithTypeIn) {
   AddDefaultExtensionValue(BOOKMARKS, &bookmark_specs);
   AddDefaultExtensionValue(PREFERENCES, &preference_specs);
   AddDefaultExtensionValue(AUTOFILL, &autofill_specs);
+  dir_->set_initial_sync_ended_for_type(BOOKMARKS, true);
+  dir_->set_last_download_timestamp(BOOKMARKS, 1);
+  dir_->set_initial_sync_ended_for_type(PREFERENCES, true);
+  dir_->set_last_download_timestamp(PREFERENCES, 1);
+  dir_->set_initial_sync_ended_for_type(AUTOFILL, true);
+  dir_->set_last_download_timestamp(AUTOFILL, 1);
+
 
   std::set<ModelType> types_to_purge;
   types_to_purge.insert(PREFERENCES);
@@ -453,30 +492,9 @@ TEST_F(SyncableDirectoryTest, TestPurgeEntriesWithTypeIn) {
 
   // We first query the in-memory data, and then reload the directory (without
   // saving) to verify that disk does not still have the data.
-  int test_scan_run = 0;
-  do {
-    string message = "Testing ";
-    message += test_scan_run == 0 ? "before " : "after ";
-    message += "directory reload.";
-    SCOPED_TRACE(testing::Message(message.c_str()));
-    {
-      ReadTransaction trans(dir_.get(), __FILE__, __LINE__);
-      MetahandleSet all_set;
-      dir_->GetAllMetaHandles(&trans, &all_set);
-      EXPECT_EQ(3U, all_set.size());
-      for (MetahandleSet::iterator iter = all_set.begin();
-           iter != all_set.end(); ++iter) {
-        Entry e(&trans, GET_BY_HANDLE, *iter);
-        if (types_to_purge.count(e.GetModelType()) ||
-            types_to_purge.count(e.GetServerModelType())) {
-          FAIL() << "Illegal type should have been deleted.";
-        }
-      }
-    }
-    if (test_scan_run == 0)
-      ReloadDir();
-    test_scan_run++;
-  } while (test_scan_run < 2);
+  CheckPurgeEntriesWithTypeInSucceeded(types_to_purge, true);
+  SaveAndReloadDir();
+  CheckPurgeEntriesWithTypeInSucceeded(types_to_purge, false);
 }
 
 TEST_F(SyncableDirectoryTest, TakeSnapshotGetsOnlyDirtyHandlesTest) {