sync: add CleanupDisabledTypesCommand to purge data pertaining to previously

synced data types that the user has disabled.

Despite my attempt at simplifying the purge code in Directory[BackingStore], I had to go back to my first attempt with DeleteEntries (which was previously reviewed separately), with a few extra gotchas in Directory::PurgeEntriesWithTypeIn.

BUG=40252
TEST=CleanupDisabledTypesCommandTest, SyncableDirectoryTest, DirectoryBackingStoreTest

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

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

7 files changed, 269 insertions, 45 deletions

diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index 63df81d..b4de3f6 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -250,6 +250,33 @@ void DirectoryBackingStore::EndLoad() {
   load_dbhandle_ = NULL;  // No longer used.
 }
 
+void DirectoryBackingStore::EndSave() {
+  sqlite3_close(save_dbhandle_);
+  save_dbhandle_ = NULL;
+}
+
+bool DirectoryBackingStore::DeleteEntries(const MetahandleSet& handles) {
+  if (handles.empty())
+    return true;
+
+  sqlite3* dbhandle = LazyGetSaveHandle();
+
+  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(dbhandle, query.data(), query.size());
+  if (SQLITE_OK == result)
+    result = statement.step();
+
+  return SQLITE_DONE == result;
+}
+
 bool DirectoryBackingStore::SaveChanges(
     const Directory::SaveChangesSnapshot& snapshot) {
   sqlite3* dbhandle = LazyGetSaveHandle();
@@ -273,6 +300,9 @@ bool DirectoryBackingStore::SaveChanges(
       return false;
   }
 
+  if (!DeleteEntries(snapshot.metahandles_to_purge))
+    return false;
+
   if (save_info) {
     const Directory::PersistedKernelInfo& info = snapshot.kernel_info;
     SQLStatement update;
diff --git a/chrome/browser/sync/syncable/directory_backing_store.h b/chrome/browser/sync/syncable/directory_backing_store.h
index fcbd848..36322e8 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.h
+++ b/chrome/browser/sync/syncable/directory_backing_store.h
@@ -81,6 +81,7 @@ class DirectoryBackingStore {
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, MigrateVersion71To72);
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, ModelTypeIds);
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, Corruption);
+  FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest, DeleteEntries);
   FRIEND_TEST_ALL_PREFIXES(MigrationTest, ToCurrentVersion);
   friend class MigrationTest;
 
@@ -122,6 +123,13 @@ class DirectoryBackingStore {
   bool BeginLoad();
   void EndLoad();
 
+  // Close save_dbhandle_.  Broken out for testing.
+  void EndSave();
+
+  // Removes each entry whose metahandle is in |handles| from the database.
+  // Does synchronous I/O.  Returns false on error.
+  bool DeleteEntries(const MetahandleSet& handles);
+
   // Lazy creation of save_dbhandle_ for use by SaveChanges code path.
   sqlite3* LazyGetSaveHandle();
 
diff --git a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
index 3d6d143..b1f5dc98 100644
--- a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
@@ -1043,4 +1043,49 @@ 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();
+  dbs->EndSave();
+}
+
 }  // namespace syncable
diff --git a/chrome/browser/sync/syncable/directory_manager.h b/chrome/browser/sync/syncable/directory_manager.h
index 61f303e..103ec30 100644
--- a/chrome/browser/sync/syncable/directory_manager.h
+++ b/chrome/browser/sync/syncable/directory_manager.h
@@ -54,7 +54,7 @@ class DirectoryManager {
 
   // root_path specifies where db is stored.
   explicit DirectoryManager(const FilePath& root_path);
-  ~DirectoryManager();
+  virtual ~DirectoryManager();
 
   static const FilePath GetSyncDataDatabaseFilename();
   const FilePath GetSyncDataDatabasePath() const;
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 3e93d40..2f11806 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -157,6 +157,11 @@ bool LessPathNames::operator() (const string& a, const string& b) const {
 static const DirectoryChangeEvent kShutdownChangesEvent =
     { DirectoryChangeEvent::SHUTDOWN, 0, 0 };
 
+void Directory::init_kernel(const std::string& name) {
+  DCHECK(kernel_ == NULL);
+  kernel_ = new Kernel(FilePath(), name, KernelLoadInfo());
+}
+
 Directory::Kernel::Kernel(const FilePath& db_path,
                           const string& name,
                           const KernelLoadInfo& info)
@@ -170,6 +175,7 @@ Directory::Kernel::Kernel(const FilePath& db_path,
       unapplied_update_metahandles(new MetahandleSet),
       unsynced_metahandles(new MetahandleSet),
       dirty_metahandles(new MetahandleSet),
+      metahandles_to_purge(new MetahandleSet),
       channel(new Directory::Channel(syncable::DIRECTORY_DESTROYED)),
       info_status(Directory::KERNEL_SHARE_INFO_VALID),
       persisted_info(info.kernel_info),
@@ -197,6 +203,7 @@ Directory::Kernel::~Kernel() {
   delete unsynced_metahandles;
   delete unapplied_update_metahandles;
   delete dirty_metahandles;
+  delete metahandles_to_purge;
   delete parent_id_child_index;
   delete client_tag_index;
   delete ids_index;
@@ -522,6 +529,10 @@ void Directory::TakeSnapshotForSaveChanges(SaveChangesSnapshot* snapshot) {
   }
   ClearDirtyMetahandles();
 
+  // Set purged handles.
+  DCHECK(snapshot->metahandles_to_purge.empty());
+  snapshot->metahandles_to_purge.swap(*(kernel_->metahandles_to_purge));
+
   // Fill kernel_info_status and kernel_info.
   snapshot->kernel_info = kernel_->persisted_info;
   // To avoid duplicates when the process crashes, we record the next_id to be
@@ -579,6 +590,7 @@ void Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
       // Might not be in it
       num_erased = kernel_->client_tag_index->erase(entry);
       DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
+      DCHECK(!kernel_->parent_id_child_index->count(entry));
       delete entry;
     }
   }
@@ -594,30 +606,42 @@ void Directory::PurgeEntriesWithTypeIn(const std::set<ModelType>& types) {
     return;
 
   {
-    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));
+    WriteTransaction trans(this, PURGE_ENTRIES, __FILE__, __LINE__);
+    {
+      ScopedKernelLock lock(this);
+      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 erase().
+        if (types.count(local_type) > 0 || types.count(server_type) > 0) {
+          UnlinkEntryFromOrder(*it, NULL, &lock);
+
+          kernel_->metahandles_to_purge->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);
+          num_erased = kernel_->parent_id_child_index->erase(*it);
+          DCHECK_EQ((*it)->ref(IS_DEL), !num_erased);
+          kernel_->metahandles_index->erase(it++);
+        } else {
+          ++it;
+        }
       }
-    }
 
-    // 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);
+      // 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);
+      }
     }
   }
 }
@@ -640,6 +664,9 @@ void Directory::HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot) {
       (*found)->mark_dirty(kernel_->dirty_metahandles);
     }
   }
+
+  kernel_->metahandles_to_purge->insert(snapshot.metahandles_to_purge.begin(),
+                                        snapshot.metahandles_to_purge.end());
 }
 
 int64 Directory::last_download_timestamp(ModelType model_type) const {
@@ -1276,32 +1303,44 @@ bool MutableEntry::Put(IndexedBitField field, bool value) {
 }
 
 void MutableEntry::UnlinkFromOrder() {
-  Id old_previous = Get(PREV_ID);
-  Id old_next = Get(NEXT_ID);
+  ScopedKernelLock lock(dir());
+  dir()->UnlinkEntryFromOrder(kernel_, write_transaction(), &lock);
+}
 
-  // Self-looping signifies that this item is not in the order.  If we were to
-  // set these to 0, we could get into trouble because this node might look
-  // like the first node in the ordering.
-  Put(NEXT_ID, Get(ID));
-  Put(PREV_ID, Get(ID));
+void Directory::UnlinkEntryFromOrder(EntryKernel* entry,
+                                     WriteTransaction* trans,
+                                     ScopedKernelLock* lock) {
+  CHECK(!trans || this == trans->directory());
+  Id old_previous = entry->ref(PREV_ID);
+  Id old_next = entry->ref(NEXT_ID);
+
+  entry->put(NEXT_ID, entry->ref(ID));
+  entry->put(PREV_ID, entry->ref(ID));
+  entry->mark_dirty(kernel_->dirty_metahandles);
 
   if (!old_previous.IsRoot()) {
     if (old_previous == old_next) {
       // Note previous == next doesn't imply previous == next == Get(ID). We
       // could have prev==next=="c-XX" and Get(ID)=="sX..." if an item was added
       // and deleted before receiving the server ID in the commit response.
-      CHECK((old_next == Get(ID)) || !old_next.ServerKnows());
+      CHECK((old_next == entry->ref(ID)) || !old_next.ServerKnows());
       return;  // Done if we were already self-looped (hence unlinked).
     }
-    MutableEntry previous_entry(write_transaction(), GET_BY_ID, old_previous);
-    CHECK(previous_entry.good());
-    previous_entry.Put(NEXT_ID, old_next);
+    EntryKernel* previous_entry = GetEntryById(old_previous, lock);
+    CHECK(previous_entry);
+    if (trans)
+      trans->SaveOriginal(previous_entry);
+    previous_entry->put(NEXT_ID, old_next);
+    previous_entry->mark_dirty(kernel_->dirty_metahandles);
   }
 
   if (!old_next.IsRoot()) {
-    MutableEntry next_entry(write_transaction(), GET_BY_ID, old_next);
-    CHECK(next_entry.good());
-    next_entry.Put(PREV_ID, old_previous);
+    EntryKernel* next_entry = GetEntryById(old_next, lock);
+    CHECK(next_entry);
+    if (trans)
+      trans->SaveOriginal(next_entry);
+    next_entry->put(PREV_ID, old_previous);
+    next_entry->mark_dirty(kernel_->dirty_metahandles);
   }
 }
 
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index fc5f33f..8a651a9 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -570,7 +570,13 @@ struct MultiTypeTimeStamp {
 // the write. This is defined up here since DirectoryChangeEvent also contains
 // one.
 enum WriterTag {
-  INVALID, SYNCER, AUTHWATCHER, UNITTEST, VACUUM_AFTER_SAVE, SYNCAPI
+  INVALID,
+  SYNCER,
+  AUTHWATCHER,
+  UNITTEST,
+  VACUUM_AFTER_SAVE,
+  PURGE_ENTRIES,
+  SYNCAPI
 };
 
 // A separate Event type and channel for very frequent changes, caused
@@ -644,6 +650,8 @@ class Directory {
   FRIEND_TEST_ALL_PREFIXES(SyncableDirectoryTest,
                            TakeSnapshotGetsOnlyDirtyHandlesTest);
   FRIEND_TEST_ALL_PREFIXES(SyncableDirectoryTest, TestPurgeEntriesWithTypeIn);
+  FRIEND_TEST_ALL_PREFIXES(SyncableDirectoryTest,
+                           TakeSnapshotGetsMetahandlesToPurge);
 
  public:
   class EventListenerHookup;
@@ -692,6 +700,7 @@ class Directory {
     KernelShareInfoStatus kernel_info_status;
     PersistedKernelInfo kernel_info;
     OriginalEntries dirty_metas;
+    MetahandleSet metahandles_to_purge;
     SaveChangesSnapshot() : kernel_info_status(KERNEL_SHARE_INFO_INVALID) {
     }
   };
@@ -773,6 +782,9 @@ class Directory {
   // The semantic checking is implemented higher up.
   void Undelete(EntryKernel* const entry);
   void Delete(EntryKernel* const entry);
+  void UnlinkEntryFromOrder(EntryKernel* entry,
+                            WriteTransaction* trans,
+                            ScopedKernelLock* lock);
 
   // Overridden by tests.
   virtual DirectoryBackingStore* CreateBackingStore(
@@ -862,7 +874,7 @@ class Directory {
   // 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.
-  void PurgeEntriesWithTypeIn(const std::set<ModelType>& types);
+  virtual void PurgeEntriesWithTypeIn(const std::set<ModelType>& types);
 
  private:
   // Helper to prime ids_index, parent_id_and_names_index, unsynced_metahandles
@@ -926,6 +938,10 @@ class Directory {
   typedef std::set<EntryKernel*,
                    LessField<StringField, UNIQUE_CLIENT_TAG> > ClientTagIndex;
 
+ protected:
+  // Used by tests.
+  void init_kernel(const std::string& name);
+
  private:
 
   struct Kernel {
@@ -970,6 +986,10 @@ class Directory {
     // necessarily).  Dirtyness is confirmed in TakeSnapshotForSaveChanges().
     MetahandleSet* const dirty_metahandles;
 
+    // When a purge takes place, we remove items from all our indices and stash
+    // them in here so that SaveChanges can persist their permanent deletion.
+    MetahandleSet* const metahandles_to_purge;
+
     // TODO(ncarter): Figure out what the hell this is, and comment it.
     Channel* const channel;
 
diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc
index 809a47c..f5a9fa7 100644
--- a/chrome/browser/sync/syncable/syncable_unittest.cc
+++ b/chrome/browser/sync/syncable/syncable_unittest.cc
@@ -300,24 +300,25 @@ 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);
+  bool IsInMetahandlesToPurge(int64 metahandle) {
+    return 1 == dir_->kernel_->metahandles_to_purge->count(metahandle);
   }
 
   void CheckPurgeEntriesWithTypeInSucceeded(const ModelTypeSet& types_to_purge,
                                             bool before_reload) {
+    SCOPED_TRACE(testing::Message("Before reload: ") << 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());
+      EXPECT_EQ(3U, all_set.size());
+      if (before_reload)
+        EXPECT_EQ(4U, dir_->kernel_->metahandles_to_purge->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))) {
+             types_to_purge.count(e.GetServerModelType()))) {
           FAIL() << "Illegal type should have been deleted.";
         }
       }
@@ -359,6 +360,49 @@ class SyncableDirectoryTest : public testing::Test {
       bool set_server_fields, bool is_dir, bool add_to_lru, int64 *meta_handle);
 };
 
+TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) {
+  const int metas_to_create = 50;
+  MetahandleSet expected_purges;
+  MetahandleSet all_handles;
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    for (int i = 0; i < metas_to_create; i++) {
+      MutableEntry e(&trans, CREATE, trans.root_id(), "foo");
+      e.Put(IS_UNSYNCED, true);
+      sync_pb::EntitySpecifics specs;
+      if (i % 2 == 0) {
+        AddDefaultExtensionValue(BOOKMARKS, &specs);
+        expected_purges.insert(e.Get(META_HANDLE));
+        all_handles.insert(e.Get(META_HANDLE));
+      } else {
+        AddDefaultExtensionValue(PREFERENCES, &specs);
+        all_handles.insert(e.Get(META_HANDLE));
+      }
+      e.Put(SPECIFICS, specs);
+      e.Put(SERVER_SPECIFICS, specs);
+    }
+  }
+
+  ModelTypeSet to_purge;
+  to_purge.insert(BOOKMARKS);
+  dir_->PurgeEntriesWithTypeIn(to_purge);
+
+  Directory::SaveChangesSnapshot snapshot1;
+  AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
+  dir_->TakeSnapshotForSaveChanges(&snapshot1);
+  EXPECT_TRUE(expected_purges == snapshot1.metahandles_to_purge);
+
+  to_purge.clear();
+  to_purge.insert(PREFERENCES);
+  dir_->PurgeEntriesWithTypeIn(to_purge);
+
+  dir_->HandleSaveChangesFailure(snapshot1);
+
+  Directory::SaveChangesSnapshot snapshot2;
+  dir_->TakeSnapshotForSaveChanges(&snapshot2);
+  EXPECT_TRUE(all_handles == snapshot2.metahandles_to_purge);
+}
+
 TEST_F(SyncableDirectoryTest, TakeSnapshotGetsAllDirtyHandlesTest) {
   const int metahandles_to_create = 100;
   std::vector<int64> expected_dirty_metahandles;
@@ -1155,6 +1199,44 @@ TEST_F(SyncableDirectoryTest, TestSaveChangesFailure) {
   }
 }
 
+TEST_F(SyncableDirectoryTest, TestSaveChangesFailureWithPurge) {
+  int64 handle1 = 0;
+  // Set up an item using a regular, saveable directory.
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+
+    MutableEntry e1(&trans, CREATE, trans.root_id(), "aguilera");
+    ASSERT_TRUE(e1.good());
+    EXPECT_TRUE(e1.GetKernelCopy().is_dirty());
+    handle1 = e1.Get(META_HANDLE);
+    e1.Put(BASE_VERSION, 1);
+    e1.Put(IS_DIR, true);
+    e1.Put(ID, TestIdFactory::FromNumber(101));
+    sync_pb::EntitySpecifics bookmark_specs;
+    AddDefaultExtensionValue(BOOKMARKS, &bookmark_specs);
+    e1.Put(SPECIFICS, bookmark_specs);
+    e1.Put(SERVER_SPECIFICS, bookmark_specs);
+    e1.Put(ID, TestIdFactory::FromNumber(101));
+    EXPECT_TRUE(e1.GetKernelCopy().is_dirty());
+    EXPECT_TRUE(IsInDirtyMetahandles(handle1));
+  }
+  ASSERT_TRUE(dir_->SaveChanges());
+
+  // Now do some operations using a directory for which SaveChanges will
+  // always fail.
+  dir_.reset(new TestUnsaveableDirectory());
+  ASSERT_TRUE(dir_.get());
+  ASSERT_TRUE(OPENED == dir_->Open(FilePath(kFilePath), kName));
+  ASSERT_TRUE(dir_->good());
+
+  ModelTypeSet set;
+  set.insert(BOOKMARKS);
+  dir_->PurgeEntriesWithTypeIn(set);
+  EXPECT_TRUE(IsInMetahandlesToPurge(handle1));
+  ASSERT_FALSE(dir_->SaveChanges());
+  EXPECT_TRUE(IsInMetahandlesToPurge(handle1));
+}
+
 // Create items of each model type, and check that GetModelType and
 // GetServerModelType return the right value.
 TEST_F(SyncableDirectoryTest, GetModelType) {