[Sync] Update DirectoryBackingStore to detect Sync DB corruption

Leverage sql::Connection's error handling mechanism to detect
"catastrophic" errors like SQLITE_CORRUPT.

Add unit tests that verify behavior of SetCatastrophicErrorHandler and
trigger DB corruption.

Because handler is not installed by default, this change will have no
impact on Sync's behavior. A future change will connect Directory to
DirectoryBackingStore's to ensure Sync is shutdown when DB corruption
occurs.

This change depends on https://codereview.chromium.org/1069313004/.

BUG=470993

Review URL: https://codereview.chromium.org/1072093002

Cr-Commit-Position: refs/heads/master@{#325135}

6 files changed, 150 insertions, 0 deletions

diff --git a/sync/BUILD.gn b/sync/BUILD.gn
index 6e567e0..cd6e0e4 100644
--- a/sync/BUILD.gn
+++ b/sync/BUILD.gn
@@ -643,6 +643,7 @@ test("sync_unit_tests") {
     "//net",
     "//net:test_support",
     "//sql",
+    "//sql:test_support",
     "//sync",
     "//sync/internal_api/attachments/proto",
     "//sync/protocol",
diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi
index 3093e42..33594a1 100644
--- a/sync/sync_tests.gypi
+++ b/sync/sync_tests.gypi
@@ -249,6 +249,7 @@
         '../net/net.gyp:net',
         '../net/net.gyp:net_test_support',
         '../sql/sql.gyp:sql',
+        '../sql/sql.gyp:test_support_sql',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
         '../third_party/leveldatabase/leveldatabase.gyp:leveldatabase',
diff --git a/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc
index 020eeb4..a62060a 100644
--- a/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc
+++ b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "sync/syncable/deferred_on_disk_directory_backing_store.h"
 #include "sync/syncable/directory.h"
@@ -27,6 +28,7 @@ class DeferredOnDiskDirectoryBackingStoreTest : public testing::Test {
 
   void TearDown() override { STLDeleteValues(&handles_map_); }
 
+  base::MessageLoop message_loop_;
   base::ScopedTempDir temp_dir_;
   base::FilePath db_path_;
   Directory::MetahandlesMap handles_map_;
diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc
index 514ed8d..6bcb96e 100644
--- a/sync/syncable/directory_backing_store.cc
+++ b/sync/syncable/directory_backing_store.cc
@@ -13,9 +13,11 @@
 #include "base/metrics/field_trial.h"
 #include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "sql/connection.h"
+#include "sql/error_delegate_util.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 #include "sync/internal_api/public/base/node_ordinal.h"
@@ -36,6 +38,19 @@ bool IsSyncBackingDatabase32KEnabled() {
   return group_name == "Enabled";
 }
 
+void OnSqliteError(const base::Closure& catastrophic_error_handler,
+                   int err,
+                   sql::Statement* statement) {
+  // An error has been detected. Ignore unless it is catastrophic.
+  if (sql::IsErrorCatastrophic(err)) {
+    // At this point sql::* and DirectoryBackingStore may be on the callstack so
+    // don't invoke the error handler directly. Instead, PostTask to this thread
+    // to avoid potential reentrancy issues.
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           catastrophic_error_handler);
+  }
+}
+
 }  // namespace
 
 namespace syncer {
@@ -185,6 +200,7 @@ DirectoryBackingStore::DirectoryBackingStore(const string& dir_name)
     : dir_name_(dir_name),
       database_page_size_(IsSyncBackingDatabase32KEnabled() ? 32768 : 4096),
       needs_column_refresh_(false) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
   ResetAndCreateConnection();
 }
 
@@ -194,6 +210,7 @@ DirectoryBackingStore::DirectoryBackingStore(const string& dir_name,
       dir_name_(dir_name),
       database_page_size_(IsSyncBackingDatabase32KEnabled() ? 32768 : 4096),
       needs_column_refresh_(false) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 }
 
 DirectoryBackingStore::~DirectoryBackingStore() {
@@ -1641,6 +1658,18 @@ void DirectoryBackingStore::ResetAndCreateConnection() {
   db_->set_exclusive_locking();
   db_->set_cache_size(32);
   db_->set_page_size(database_page_size_);
+  if (!catastrophic_error_handler_.is_null())
+    SetCatastrophicErrorHandler(catastrophic_error_handler_);
+}
+
+void DirectoryBackingStore::SetCatastrophicErrorHandler(
+    const base::Closure& catastrophic_error_handler) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!catastrophic_error_handler.is_null());
+  catastrophic_error_handler_ = catastrophic_error_handler;
+  sql::Connection::ErrorCallback error_callback =
+      base::Bind(&OnSqliteError, catastrophic_error_handler_);
+  db_->set_error_callback(error_callback);
 }
 
 }  // namespace syncable
diff --git a/sync/syncable/directory_backing_store.h b/sync/syncable/directory_backing_store.h
index 81add28..bb3641f 100644
--- a/sync/syncable/directory_backing_store.h
+++ b/sync/syncable/directory_backing_store.h
@@ -47,6 +47,10 @@ class SYNC_EXPORT_PRIVATE DirectoryBackingStore : public base::NonThreadSafe {
   friend class DirectoryBackingStoreTest;
   FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest,
                            IncreaseDatabasePageSizeFrom4KTo32K);
+  FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest,
+                           CatastrophicErrorHandler_KeptAcrossReset);
+  FRIEND_TEST_ALL_PREFIXES(DirectoryBackingStoreTest,
+                           CatastrophicErrorHandler_Invocation);
 
  public:
   explicit DirectoryBackingStore(const std::string& dir_name);
@@ -72,8 +76,31 @@ class SYNC_EXPORT_PRIVATE DirectoryBackingStore : public base::NonThreadSafe {
   // opening transactions elsewhere to block on synchronous I/O.
   // DO NOT CALL THIS FROM MORE THAN ONE THREAD EVER.  Also, whichever thread
   // calls SaveChanges *must* be the thread that owns/destroys |this|.
+  //
+  // Returns true if the changes were saved successfully. Returns false if an
+  // error (of any kind) occurred. See also |SetCatastrophicErrorHandler| for a
+  // systematic way of handling underlying DB errors.
   virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot);
 
+  // Set the catastrophic error handler.
+  //
+  // When a catastrophic error is encountered while accessing the underlying DB,
+  // |catastrophic_error_handler| will be invoked (via PostTask) on the thread
+  // on which this DirectoryBackingStore object lives.
+  //
+  // For a definition of what's catastrophic, see sql::IsErrorCatastrophic.
+  //
+  // |catastrophic_error_handler| must be initialized (i.e. !is_null()).
+  //
+  // A single operation (like Load or SaveChanges) may result in the
+  // |catastrophic_error_handler| being invoked several times.
+  //
+  // There can be at most one handler. If this method is invoked when there is
+  // already a handler, the existing handler is overwritten with
+  // |catastrophic_error_handler|.
+  virtual void SetCatastrophicErrorHandler(
+      const base::Closure& catastrophic_error_handler);
+
  protected:
   // For test classes.
   DirectoryBackingStore(const std::string& dir_name,
@@ -206,6 +233,10 @@ class SYNC_EXPORT_PRIVATE DirectoryBackingStore : public base::NonThreadSafe {
   // discarded.
   bool needs_column_refresh_;
 
+  // We keep a copy of the Closure so we reinstall it when the underlying
+  // sql::Connection is destroyed/recreated.
+  base::Closure catastrophic_error_handler_;
+
   DISALLOW_COPY_AND_ASSIGN(DirectoryBackingStore);
 };
 
diff --git a/sync/syncable/directory_backing_store_unittest.cc b/sync/syncable/directory_backing_store_unittest.cc
index ed721e0..a45a25b 100644
--- a/sync/syncable/directory_backing_store_unittest.cc
+++ b/sync/syncable/directory_backing_store_unittest.cc
@@ -10,10 +10,14 @@
 #include "base/files/file_util.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"
@@ -26,6 +30,15 @@
 #include "sync/util/time.h"
 #include "testing/gtest/include/gtest/gtest-param-test.h"
 
+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;
+}
+
+}  // namespace
+
 namespace syncer {
 namespace syncable {
 
@@ -89,6 +102,7 @@ class MigrationTest : public testing::TestWithParam<int> {
   }
 
  private:
+  base::MessageLoop message_loop_;
   base::ScopedTempDir temp_dir_;
 };
 
@@ -3995,5 +4009,77 @@ TEST_F(DirectoryBackingStoreTest, IncreaseDatabasePageSizeFrom4KTo32K) {
   EXPECT_EQ(32768, pageSize);
 }
 
+// See that a catastrophic error handler remains set across instances of the
+// underlying sql:Connection.
+TEST_F(DirectoryBackingStoreTest, CatastrophicErrorHandler_KeptAcrossReset) {
+  scoped_ptr<OnDiskDirectoryBackingStoreForTest> dbs(
+      new OnDiskDirectoryBackingStoreForTest(GetUsername(), GetDatabasePath()));
+  // See that by default there is no catastrophic error handler.
+  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) {
+  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());
+    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()));
+
+  {
+    scoped_ptr<OnDiskDirectoryBackingStoreForTest> dbs(
+        new OnDiskDirectoryBackingStoreForTest(GetUsername(),
+                                               GetDatabasePath()));
+    dbs->SetCatastrophicErrorHandler(handler);
+    ASSERT_TRUE(dbs->db_->has_error_callback());
+    {
+      // The corruption will be detected when we attempt to load the data. Use a
+      // ScopedErrorIgnorer to ensure we don't crash in debug builds.
+      sql::ScopedErrorIgnorer error_ignorer;
+      error_ignorer.IgnoreError(SQLITE_CORRUPT);
+      ASSERT_TRUE(LoadAndIgnoreReturnedData(dbs.get()));
+      ASSERT_TRUE(error_ignorer.CheckIgnoredErrors());
+    }
+    // See that the first open failed as expected.
+    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);
+}
+
 }  // namespace syncable
 }  // namespace syncer