Handle double-error edge case in sql::Database::Open()

Currently, Database::Open() incorrectly returns true when both attempts
at opening the database fail. We're chalking this behavior up to a
simple oversight in Database::OpenInternal()'s error handling. See the
linked crbug for more details.

This CL addresses the root cause by unifying Database::Open()'s error
handling and retry logic. In doing so, it resolves a preexisting TODO.
As a side effect, Database::Open() no longer invokes the error callback
6 times in the double-error scenario, which was why I originally filed
the crbug.

Bug: 1509891
Change-Id: Ib0b2f295a2932cf8612f74763db11896d11d6227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5141149
Reviewed-by: Nan Lin <linnan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Angela Yoeurng <yoangela@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1242432}

chrome/browser/diagnostics/diagnostics_controller_unittest.cc

@@ -136,7 +136,7 @@ TEST_F(DiagnosticsControllerTest, RecoverFromNssCertDbFailure) {
   EXPECT_TRUE(
       results.GetTestInfo(DIAGNOSTICS_SQLITE_INTEGRITY_NSS_CERT_TEST, &info));
   EXPECT_EQ(DiagnosticsModel::TEST_FAIL_CONTINUE, info->GetResult());
-  EXPECT_EQ(DIAG_SQLITE_ERROR_HANDLER_CALLED, info->GetOutcomeCode());
+  EXPECT_EQ(DIAG_SQLITE_CANNOT_OPEN_DB, info->GetOutcomeCode());
 
   DiagnosticsController::GetInstance()->RunRecovery(cmdline_, writer_.get());
   EXPECT_EQ(DiagnosticsModel::RECOVERY_OK, info->GetResult());
@@ -158,7 +158,7 @@ TEST_F(DiagnosticsControllerTest, RecoverFromNssKeyDbFailure) {
   EXPECT_TRUE(
       results.GetTestInfo(DIAGNOSTICS_SQLITE_INTEGRITY_NSS_KEY_TEST, &info));
   EXPECT_EQ(DiagnosticsModel::TEST_FAIL_CONTINUE, info->GetResult());
-  EXPECT_EQ(DIAG_SQLITE_ERROR_HANDLER_CALLED, info->GetOutcomeCode());
+  EXPECT_EQ(DIAG_SQLITE_CANNOT_OPEN_DB, info->GetOutcomeCode());
 
   DiagnosticsController::GetInstance()->RunRecovery(cmdline_, writer_.get());
   EXPECT_EQ(DiagnosticsModel::RECOVERY_OK, info->GetResult());

components/favicon/core/favicon_database_unittest.cc

@@ -1096,7 +1096,7 @@ TEST_F(FaviconDatabaseTest, Recovery) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      EXPECT_TRUE(raw_db.Open(file_name_));
+      EXPECT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));
@@ -1191,7 +1191,7 @@ TEST_F(FaviconDatabaseTest, Recovery7) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      ASSERT_TRUE(raw_db.Open(file_name_));
+      ASSERT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));
@@ -1232,7 +1232,7 @@ TEST_F(FaviconDatabaseTest, Recovery6) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      EXPECT_TRUE(raw_db.Open(file_name_));
+      EXPECT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));
@@ -1278,7 +1278,7 @@ TEST_F(FaviconDatabaseTest, Recovery5) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      EXPECT_TRUE(raw_db.Open(file_name_));
+      EXPECT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));

components/history/core/browser/top_sites_database_unittest.cc

@@ -167,7 +167,7 @@ TEST_F(TopSitesDatabaseTest, Recovery1) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      ASSERT_TRUE(raw_db.Open(file_name_));
+      ASSERT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));
@@ -202,7 +202,7 @@ TEST_F(TopSitesDatabaseTest, Recovery2) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      ASSERT_TRUE(raw_db.Open(file_name_));
+      ASSERT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));
@@ -235,7 +235,7 @@ TEST_F(TopSitesDatabaseTest, Recovery4_CorruptHeader) {
     {
       sql::test::ScopedErrorExpecter expecter;
       expecter.ExpectError(SQLITE_CORRUPT);
-      ASSERT_TRUE(raw_db.Open(file_name_));
+      ASSERT_FALSE(raw_db.Open(file_name_));
       EXPECT_TRUE(expecter.SawExpectedErrors());
     }
     EXPECT_EQ("ok", sql::test::IntegrityCheck(raw_db));

components/omnibox/browser/shortcuts_database_unittest.cc

@@ -381,7 +381,7 @@ TEST(ShortcutsDatabaseMigrationTest, Recovery1) {
     expecter.ExpectError(SQLITE_CORRUPT);
 
     sql::Database connection;
-    ASSERT_TRUE(connection.Open(db_path));
+    ASSERT_FALSE(connection.Open(db_path));
     sql::Statement statement(connection.GetUniqueStatement(kCountSql));
     ASSERT_FALSE(statement.is_valid());
 

content/browser/aggregation_service/aggregation_service_storage_sql_unittest.cc

@@ -1126,7 +1126,7 @@ TEST_F(AggregationServiceStorageSqlTest,
       "PrivacySandbox.AggregationService.Storage.Sql.Error",
       base::checked_cast<base::HistogramBase::Sample>(
           sql::SqliteLoggedResultCode::kCorrupt),
-      /*expected_bucket_count=*/6);
+      /*expected_bucket_count=*/1);
 
   CloseDatabase();
 }

sql/database.cc

@@ -338,7 +338,18 @@ bool Database::Open(const base::FilePath& path) {
   DCHECK_NE(path_string, kSqliteOpenInMemoryPath)
       << "Path conflicts with SQLite magic identifier";
 
-  return OpenInternal(path_string, OpenMode::kRetryOnPoision);
+  if (OpenInternal(path_string, OpenMode::kNone)) {
+    return true;
+  }
+  // OpenInternal() may have run the error callback before returning false. If
+  // the error callback poisoned `this`, the database may have been recovered or
+  // razed, so a second attempt may succeed.
+  if (poisoned_) {
+    Close();
+    return OpenInternal(path_string, OpenMode::kNone);
+  }
+  // Otherwise, do not attempt to reopen.
+  return false;
 }
 
 bool Database::OpenInMemory() {
@@ -1837,7 +1848,7 @@ bool Database::OpenInternal(const std::string& db_file_path,
   std::string uri_file_path = db_file_path;
   if (options_.exclusive_database_file_lock) {
 #if BUILDFLAG(IS_WIN)
-    if (mode == OpenMode::kNone || mode == OpenMode::kRetryOnPoision) {
+    if (mode == OpenMode::kNone) {
       // Do not allow query injection.
       if (base::Contains(db_file_path, '?')) {
         return false;
@@ -1867,11 +1878,6 @@ bool Database::OpenInternal(const std::string& db_file_path,
 
     OnSqliteError(ToSqliteErrorCode(sqlite_result_code), nullptr,
                   "-- sqlite3_open_v2()");
-    bool was_poisoned = poisoned_;
-    Close();
-
-    if (was_poisoned && mode == OpenMode::kRetryOnPoision)
-      return OpenInternal(db_file_path, OpenMode::kNone);
     return false;
   }
 
@@ -1915,16 +1921,7 @@ bool Database::OpenInternal(const std::string& db_file_path,
   if (sqlite_result_code != SqliteResultCode::kOk) {
     OnSqliteError(ToSqliteErrorCode(sqlite_result_code), nullptr,
                   "-- sqlite3_table_column_metadata()");
-
-    // Retry or bail out if the error handler poisoned the handle.
-    // TODO(shess): Move this handling to one place (see also sqlite3_open).
-    //              Possibly a wrapper function?
-    if (poisoned_) {
-      Close();
-      if (mode == OpenMode::kRetryOnPoision)
-        return OpenInternal(db_file_path, OpenMode::kNone);
-      return false;
-    }
+    return false;
   }
 
   const base::TimeDelta kBusyTimeout = base::Seconds(kBusyTimeoutSeconds);

sql/database.h

@@ -376,7 +376,10 @@ class COMPONENT_EXPORT(SQL) Database {
   // associated with the database (rollback journal, write-ahead log,
   // shared-memory file) may be created.
   //
-  // Returns true in case of success, false in case of failure.
+  // Returns true in case of success, false in case of failure. If an error
+  // occurs, this function will invoke the error callback if it is present and
+  // then may attempt to open the database a second time. If the second attempt
+  // succeeds, it will return true.
   [[nodiscard]] bool Open(const base::FilePath& db_file_path);
 
   // Alternative to Open() that creates an in-memory database.
@@ -732,17 +735,13 @@ class COMPONENT_EXPORT(SQL) Database {
   // Enables a special behavior for OpenInternal().
   enum class OpenMode {
     // No special behavior.
-    kNone = 0,
-
-    // Retry if the database error handler is invoked and closes the database.
-    // Database error handlers that call RazeAndPoison() take advantage of this.
-    kRetryOnPoision = 1,
+    kNone,
 
     // Open an in-memory database. Used by OpenInMemory().
-    kInMemory = 2,
+    kInMemory,
 
     // Open a temporary database. Used by OpenTemporary().
-    kTemporary = 3,
+    kTemporary,
   };
 
   // Implements Open(), OpenInMemory(), and OpenTemporary().

sql/database_unittest.cc

@@ -6,7 +6,11 @@
 
 #include <stddef.h>
 #include <stdint.h>
-#include <cstdint>
+
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
 
 #include "base/containers/contains.h"
 #include "base/files/file.h"
@@ -27,6 +31,7 @@
 #include "build/build_config.h"
 #include "sql/database_memory_dump_provider.h"
 #include "sql/meta_table.h"
+#include "sql/recovery.h"
 #include "sql/sql_features.h"
 #include "sql/statement.h"
 #include "sql/test/database_test_peer.h"
@@ -99,10 +104,22 @@ class SQLDatabaseTest : public testing::Test,
   ~SQLDatabaseTest() override = default;
 
   void SetUp() override {
-    db_ = std::make_unique<Database>(GetDBOptions());
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     db_path_ = temp_dir_.GetPath().AppendASCII("database_test.sqlite");
+    CreateFreshDB();
+  }
+
+  // Resets the database handle and deletes the backing file. On return, `db_`
+  // has just been opened on a fresh temp file named by `db_path_`.
+  void CreateFreshDB() {
+    ASSERT_FALSE(db_path_.empty());
+
+    db_.reset();
+    ASSERT_TRUE(base::DeleteFile(db_path_));
+
+    db_ = std::make_unique<Database>(GetDBOptions());
     ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_TRUE(base::PathExists(db_path_));
   }
 
   DatabaseOptions GetDBOptions() {
@@ -402,7 +419,7 @@ TEST_P(SQLDatabaseTest, SchemaIntrospectionUsesErrorExpecter) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_FALSE(db_->Open(db_path_));
     ASSERT_FALSE(db_->DoesTableExist("bar"));
     ASSERT_FALSE(db_->DoesTableExist("foo"));
     ASSERT_FALSE(db_->DoesColumnExist("foo", "id"));
@@ -972,7 +989,7 @@ TEST_P(SQLDatabaseTest, RazeNOTADB) {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_NOTADB);
 
-    EXPECT_TRUE(db_->Open(db_path_));
+    EXPECT_FALSE(db_->Open(db_path_));
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
   EXPECT_TRUE(db_->Raze());
@@ -998,7 +1015,7 @@ TEST_P(SQLDatabaseTest, RazeNOTADB2) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_NOTADB);
-    EXPECT_TRUE(db_->Open(db_path_));
+    EXPECT_FALSE(db_->Open(db_path_));
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
   EXPECT_TRUE(db_->Raze());
@@ -1027,7 +1044,7 @@ TEST_P(SQLDatabaseTest, RazeCallbackReopen) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_FALSE(db_->Open(db_path_));
     ASSERT_FALSE(db_->Execute("PRAGMA auto_vacuum"));
     db_->Close();
     ASSERT_TRUE(expecter.SawExpectedErrors());
@@ -2141,11 +2158,53 @@ TEST_P(SQLDatabaseTest, OpenFailsAfterCorruptSizeInHeader) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_FALSE(db_->Open(db_path_));
     EXPECT_TRUE(expecter.SawExpectedErrors());
   }
 }
 
+TEST_P(SQLDatabaseTest, OpenWithRecoveryHandlesCorruption) {
+  for (const bool corrupt_after_recovery : {false, true}) {
+    SCOPED_TRACE(::testing::Message()
+                 << "corrupt_after_recovery: " << corrupt_after_recovery);
+    // Ensure that `db_` is fresh in this iteration.
+    CreateFreshDB();
+    // The database file ends up empty if we don't create at least one table.
+    ASSERT_TRUE(
+        db_->Execute("CREATE TABLE rows(i INTEGER PRIMARY KEY NOT NULL)"));
+    db_->Close();
+
+    ASSERT_TRUE(sql::test::CorruptSizeInHeader(db_path_));
+
+    size_t error_count = 0;
+    auto callback = base::BindLambdaForTesting([&](int error, Statement* stmt) {
+      error_count++;
+      ASSERT_TRUE(BuiltInRecovery::RecoverIfPossible(
+          db_.get(), error, sql::BuiltInRecovery::Strategy::kRecoverOrRaze));
+      if (corrupt_after_recovery) {
+        // Corrupt the file again after temporarily recovering it.
+        ASSERT_TRUE(sql::test::CorruptSizeInHeader(db_path_));
+      }
+    });
+    db_->set_error_callback(std::move(callback));
+
+    {
+      sql::test::ScopedErrorExpecter expecter;
+      expecter.ExpectError(SQLITE_CORRUPT);
+
+      // When `corrupt_after_recovery` is true, `Database::Open()` will return
+      // false because both attempts at opening the database will fail. When the
+      // database is *not* corrupted after recovery, recovery will succeed and
+      // thus `Database::Open()`'s second attempt at opening the database will
+      // succeed.
+      ASSERT_EQ(db_->Open(db_path_), !corrupt_after_recovery);
+      EXPECT_TRUE(expecter.SawExpectedErrors());
+    }
+    EXPECT_EQ(error_count, 1u);
+    EXPECT_FALSE(db_->has_error_callback());
+  }
+}
+
 TEST_P(SQLDatabaseTest, ExecuteFailsAfterCorruptSizeInHeader) {
   ASSERT_TRUE(
       db_->Execute("CREATE TABLE rows(i INTEGER PRIMARY KEY NOT NULL)"));
@@ -2158,7 +2217,7 @@ TEST_P(SQLDatabaseTest, ExecuteFailsAfterCorruptSizeInHeader) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_FALSE(db_->Open(db_path_));
     EXPECT_TRUE(expecter.SawExpectedErrors())
         << "Database::Open() did not encounter SQLITE_CORRUPT";
   }
@@ -2182,7 +2241,7 @@ TEST_P(SQLDatabaseTest, SchemaFailsAfterCorruptSizeInHeader) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(db_->Open(db_path_));
+    ASSERT_FALSE(db_->Open(db_path_));
     EXPECT_TRUE(expecter.SawExpectedErrors())
         << "Database::Open() did not encounter SQLITE_CORRUPT";
   }

sql/recovery_unittest.cc

@@ -508,7 +508,7 @@ TEST_P(SqlRecoveryTest, RecoverCorruptTable) {
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
-    ASSERT_TRUE(Reopen());
+    ASSERT_FALSE(Reopen());
     EXPECT_TRUE(expecter.SawExpectedErrors());
     // PRAGMAs executed inside Database::Open() will error out.
   }
@@ -1141,6 +1141,21 @@ TEST_P(SqlRecoveryTest, RecoverIfPossibleMeta) {
                       std::move(run_recovery));
 }
 
+TEST_P(SqlRecoveryTest, RecoverIfPossibleWithoutErrorCallback) {
+  auto run_recovery = base::BindLambdaForTesting([&]() {
+    // `RecoverIfPossible()` should not set an error callback.
+    EXPECT_FALSE(db_.has_error_callback());
+    bool recovery_was_attempted = BuiltInRecovery::RecoverIfPossible(
+        &db_, SQLITE_CORRUPT,
+        BuiltInRecovery::Strategy::kRecoverWithMetaVersionOrRaze);
+    EXPECT_TRUE(recovery_was_attempted);
+    EXPECT_FALSE(db_.has_error_callback());
+  });
+
+  TestRecoverDatabase(db_, db_path_, /*with_meta=*/true,
+                      std::move(run_recovery));
+}
+
 TEST_P(SqlRecoveryTest, RecoverIfPossibleWithErrorCallback) {
   auto run_recovery = base::BindLambdaForTesting([&]() {
     db_.set_error_callback(base::DoNothing());
@@ -1266,7 +1281,7 @@ TEST_P(SqlRecoveryTest, RecoverDatabaseDelete) {
     expecter.ExpectError(SQLITE_NOTADB);
 
     // Reopen() here because it will see SQLITE_NOTADB.
-    ASSERT_TRUE(Reopen());
+    ASSERT_FALSE(Reopen());
 
     // This should "recover" the database by making it valid, but empty.
     if (UseBuiltIn()) {
@@ -1365,7 +1380,7 @@ TEST_P(SqlRecoveryTest, AttachFailure) {
     expecter.ExpectError(SQLITE_NOTADB);
 
     // Reopen() here because it will see SQLITE_NOTADB.
-    ASSERT_TRUE(Reopen());
+    ASSERT_FALSE(Reopen());
 
     // Begin() should fail.
     if (UseBuiltIn()) {

storage/browser/quota/quota_database_unittest.cc

@@ -985,7 +985,7 @@ TEST_F(QuotaDatabaseTest, OpenCorruptedDatabase) {
 
   histograms.ExpectTotalCount("Quota.QuotaDatabaseReset", 1);
   histograms.ExpectBucketCount("Quota.QuotaDatabaseReset",
-                               DatabaseResetReason::kCreateSchema, 1);
+                               DatabaseResetReason::kOpenDatabase, 1);
 }
 
 TEST_F(QuotaDatabaseTest, QuotaDatabasePathMigration) {