[Sync] Add test to verify sync DB corruption handling during SaveChanges

sync/syncable/directory_backing_store_unittest.cc

 // Copyright 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 "testing/gtest/include/gtest/gtest.h"
 
 #include <string>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/files/scoped_file.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/test/scoped_error_ignorer.h"
 #include "sql/test/test_helpers.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/node_ordinal.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/directory_backing_store.h"
 #include "sync/syncable/on_disk_directory_backing_store.h"
 #include "sync/syncable/syncable-inl.h"
 #include "sync/test/test_directory_backing_store.h"
 #include "sync/util/time.h"
 #include "testing/gtest/include/gtest/gtest-param-test.h"
 
+namespace syncer {
+namespace syncable {
 namespace {
 
 // A handler that simply sets |catastrophic_error_handler_was_called| to true.
 void CatastrophicErrorHandler(bool* catastrophic_error_handler_was_called) {
   *catastrophic_error_handler_was_called = true;
 }
 
+// Create a dirty EntryKernel with an ID derived from |id|.
+scoped_ptr<EntryKernel> CreateEntry(int id) {
+  scoped_ptr<EntryKernel> entry(new EntryKernel());
+  entry->put(ID, Id::CreateFromClientString(base::Int64ToString(id)));
+  entry->put(META_HANDLE, id);
+  entry->mark_dirty(NULL);
+  return entry;
+}
+
 }  // namespace
 
-namespace syncer {
-namespace syncable {
-
 SYNC_EXPORT_PRIVATE extern const int32 kCurrentDBVersion;
 
 class MigrationTest : public testing::TestWithParam<int> {
  public:
   void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
 
  protected:
   std::string GetUsername() {
     return "nick@chromium.org";
   }
@@ skipping 3963 matching lines @@
   ASSERT_FALSE(dbs->db_->has_error_callback());
   // Set one and see that it was set.
   dbs->SetCatastrophicErrorHandler(
       base::Bind(&CatastrophicErrorHandler, nullptr));
   ASSERT_TRUE(dbs->db_->has_error_callback());
   // Recreate the Connection and see that the handler remains set.
   dbs->ResetAndCreateConnection();
   ASSERT_TRUE(dbs->db_->has_error_callback());
 }
 
-// Verify that database corruption will trigger the catastrohpic error handler.
-TEST_F(DirectoryBackingStoreTest, CatastrophicErrorHandler_Invocation) {
+// Verify that database corruption encountered during Load will trigger the
+// catastrohpic error handler.
+TEST_F(DirectoryBackingStoreTest,
+       CatastrophicErrorHandler_InvocationDuringLoad) {
   bool was_called = false;
   const base::Closure handler =
       base::Bind(&CatastrophicErrorHandler, &was_called);
   {
     scoped_ptr<OnDiskDirectoryBackingStoreForTest> dbs(
         new OnDiskDirectoryBackingStoreForTest(GetUsername(),
                                                GetDatabasePath()));
     dbs->SetCatastrophicErrorHandler(handler);
     ASSERT_TRUE(dbs->db_->has_error_callback());
     // Load the DB, and save one entry.
     ASSERT_TRUE(LoadAndIgnoreReturnedData(dbs.get()));
     ASSERT_FALSE(dbs->DidFailFirstOpenAttempt());
     Directory::SaveChangesSnapshot snapshot;
-    scoped_ptr<EntryKernel> entry(new EntryKernel());
-    entry->put(ID, Id::CreateFromClientString("test_entry"));
-    entry->put(META_HANDLE, 2);
-    entry->mark_dirty(NULL);
-    snapshot.dirty_metas.insert(entry.release());
+    snapshot.dirty_metas.insert(CreateEntry(2).release());
     ASSERT_TRUE(dbs->SaveChanges(snapshot));
   }
 
   base::RunLoop().RunUntilIdle();
   // No catastrophic errors have happened. See that it hasn't be called yet.
   ASSERT_FALSE(was_called);
 
   // Corrupt the DB. Some forms of corruption (like this one) will be detected
   // upon loading the Sync DB.
   ASSERT_TRUE(sql::test::CorruptSizeInHeader(GetDatabasePath()));
@@ skipping 16 matching lines @@
     ASSERT_TRUE(dbs->DidFailFirstOpenAttempt());
   }
 
   // At this point the handler has been posted but not executed.
   ASSERT_FALSE(was_called);
   // Pump the message loop and see that it is executed.
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(was_called);
 }
 
+// Verify that database corruption encountered during SaveChanges will trigger
+// the catastrohpic error handler.
+TEST_F(DirectoryBackingStoreTest,
+       CatastrophicErrorHandler_InvocationDuringSaveChanges) {
+  bool was_called = false;
+  const base::Closure handler =
+      base::Bind(&CatastrophicErrorHandler, &was_called);
+
+  // Create a DB with many entries. The DB must be large enough that sqlite
+  // won't keep it all in memory. If this test fails, you may need to increase

[Nicolas Zea, 2015/04/23 21:53:27]

Hmm, do we control how many entries SQLite will keep in memory, or is that
something it does on its own?

[maniscalco, 2015/04/23 22:57:48]

It's complicated and I don't fully understand it.  Here's what I know.

With 1000 entries, I found that sqlite didn't read the file before writing to it
and so it didn't notice the corruption.  With 2000 entries it did read the file
before writing.  I set the num_entries value to 4000 to be safe.  There is some
threshold here and it's not entirely clear to me on which factors it depends.

I suspect the threshold depends on the DB page size and the cache size
(controlled by a PRAGMA).  Possibly more.

http://www.sqlite.org/sessions/pragma.html#pragma_cache_size

My guess is that with larger cache and larger page size, we'd need a larger
number of entries.

One thing I can do is to add a comment at the ASSERT that would fail if the
current value of num_entries becomes in sufficient.  That might at least help a
future maintainer understand that num_entries may need to be increased.

+  // the number of entries written to the DB.
+  scoped_ptr<OnDiskDirectoryBackingStoreForTest> dbs(
+      new OnDiskDirectoryBackingStoreForTest(GetUsername(), GetDatabasePath()));
+  dbs->SetCatastrophicErrorHandler(handler);
+  ASSERT_TRUE(dbs->db_->has_error_callback());
+  ASSERT_TRUE(LoadAndIgnoreReturnedData(dbs.get()));
+  ASSERT_FALSE(dbs->DidFailFirstOpenAttempt());
+  Directory::SaveChangesSnapshot snapshot;
+  const int num_entries = 4000;
+  for (int i = 0; i < num_entries; ++i) {
+    snapshot.dirty_metas.insert(CreateEntry(i).release());
+  }
+  ASSERT_TRUE(dbs->SaveChanges(snapshot));
+
+  // Corrupt the DB by write a bunch of zeros at the beginning.
+  {
+    // Because the file is already open for writing (see dbs above), it's
+    // important that we open it in a sharing compatible way for platforms that
+    // have the concept of shared/exclusive file access (e.g. Windows).
+    base::ScopedFILE db_file(base::OpenFile(GetDatabasePath(), "wb"));
+    ASSERT_TRUE(db_file.get());
+    const std::string zeros(4096, '\0');
+    ASSERT_EQ(1U, fwrite(zeros.data(), zeros.size(), 1, db_file.get()));
+  }
+
+  // Attempt to save all those entries again. See that it fails (because of the
+  // corruption).
+  ASSERT_FALSE(dbs->SaveChanges(snapshot));
+  // At this point the handler has been posted but not executed.
+  ASSERT_FALSE(was_called);
+  // Pump the message loop and see that it is executed.
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(was_called);
+}
+
 }  // namespace syncable
 }  // namespace syncer