sql: Consider poisoned databases closed

This feature is enabled by default, but is gated behind a new base
feature flag. It builds on the changes behind the kClearDbIfCloseFails
flag. Rather than adding a new flag whose value would depend on this
older flag, this CL simply removes the older flag, which has been on
stable for almost two milestones and is therefore removable according
to the flag-guarding guidelines.

Bug: 1441955
Change-Id: Idabc75fc69250976a907dfe4e554cc9f204b21fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4499396
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1184284}

sql/database.cc

@@ -380,7 +380,7 @@ void Database::CloseInternal(bool forced) {
     statement_ref->Close(forced);
   open_statements_.clear();
 
-  if (db_) {
+  if (is_open()) {
     // Call to InitScopedBlockingCall() cannot go at the beginning of the
     // function because Close() must be called from destructor to clean
     // statement_cache_, it won't cause any disk access and it most probably
@@ -414,6 +414,13 @@ void Database::CloseInternal(bool forced) {
   }
 }
 
+bool Database::is_open() const {
+  bool is_closed_due_to_poisoning =
+      poisoned_ && base::FeatureList::IsEnabled(
+                       sql::features::kConsiderPoisonedDatabasesClosed);
+  return static_cast<bool>(db_) && !is_closed_due_to_poisoning;
+}
+
 void Database::Close() {
   TRACE_EVENT0("sql", "Database::Close");
   // If the database was already closed by RazeAndPoison(), then no
@@ -1793,7 +1800,7 @@ bool Database::OpenInternal(const std::string& db_file_path,
         << "Database file path conflicts with SQLite magic identifier";
   }
 
-  if (db_) {
+  if (is_open()) {
     DLOG(DCHECK) << "sql::Database is already open.";
     return false;
   }
@@ -1807,8 +1814,6 @@ bool Database::OpenInternal(const std::string& db_file_path,
   // RazeAndPoison().  Until regular Close() is called, the caller
   // should be treating the database as open, but is_open() currently
   // only considers the sqlite3 handle's state.
-  // TODO(shess): Revise is_open() to consider poisoned_, and review
-  // to see if any non-testing code even depends on it.
   DCHECK(!poisoned_) << "sql::Database is already open.";
   poisoned_ = false;
 
@@ -1851,7 +1856,7 @@ bool Database::OpenInternal(const std::string& db_file_path,
     // if an error occurs (see https://www.sqlite.org/c3ref/open.html).
     // Therefore, we'll clear `db_` immediately - particularly before triggering
     // an error callback which may check whether a database connection exists.
-    if (base::FeatureList::IsEnabled(features::kClearDbIfCloseFails) && db_) {
+    if (db_) {
       // Deallocate resources allocated during the failed open.
       // See https://www.sqlite.org/c3ref/close.html.
       sqlite3_close(db_);

sql/database.h

@@ -391,7 +391,7 @@ class COMPONENT_EXPORT(SQL) Database {
   [[nodiscard]] bool OpenTemporary(base::PassKey<Recovery>);
 
   // Returns true if the database has been successfully opened.
-  bool is_open() const { return static_cast<bool>(db_); }
+  bool is_open() const;
 
   // Closes the database. This is automatically performed on destruction for
   // you, but this allows you to close the database early. You must not call

sql/sql_features.cc

@@ -6,10 +6,11 @@
 
 namespace sql::features {
 
-// Clears the Database.db_ member if sqlite3_open_v2() fails.
+// When enabled, the `Database::is_open()` method return false for poisoned
+// databases.
 // TODO(https://crbug.com/1441955): Remove this flag eventually.
-BASE_FEATURE(kClearDbIfCloseFails,
-             "ClearDbIfCloseFails",
+BASE_FEATURE(kConsiderPoisonedDatabasesClosed,
+             "ConsiderPoisonedDatabasesClosed",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
 // Enable WAL mode for all SQLite databases.

sql/sql_features.h

@@ -14,7 +14,7 @@ namespace sql::features {
 // be documented alongside the definition of their values in the .cc file.
 
 // Alphabetical:
-COMPONENT_EXPORT(SQL) BASE_DECLARE_FEATURE(kClearDbIfCloseFails);
+COMPONENT_EXPORT(SQL) BASE_DECLARE_FEATURE(kConsiderPoisonedDatabasesClosed);
 COMPONENT_EXPORT(SQL) BASE_DECLARE_FEATURE(kEnableWALModeByDefault);
 COMPONENT_EXPORT(SQL) BASE_DECLARE_FEATURE(kUseBuiltInRecoveryIfSupported);