Remove ability to enable virtual tables from sql::DatabaseOptions

This change almost allows us to remove FTS3 support from the SQLite
build (saving ~64kb of binary size on Android), as it is only needed by
WebSQL, which is in the process of being removed, but WebSQL is still
enabled for WebView.

As a result, in order to retain test coverage of FTS3 availability,
we add a test-only private field to allow sql::Database to continue
enabling virtual tables. This field will be removed once we are sure
that WebSQL and therefore FTS3 support can be removed as well.

Bug: 340805983
Change-Id: I7ac2f7aeb35bd230194ac224a9413a47a7343988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5539299
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301504}

sql/README.md

@@ -443,38 +443,6 @@ Direct `PRAGMA` use limits our ability to customize and secure our SQLite build.
 Furthermore, some `PRAGMA` statements invalidate previously compiled queries,
 reducing the efficiency of Chrome's compiled query cache.
 
-#### Virtual tables {#no-virtual-tables}
-
-[`CREATE VIRTUAL TABLE` statements](https://www.sqlite.org/vtab.html) should not
-be used. The desired functionality should be implemented in C++, and access
-storage using standard SQL statements.
-
-Virtual tables are [SQLite's module system](https://www.sqlite.org/vtab.html).
-SQL statements on virtual tables are essentially running arbitrary code, which
-makes them very difficult to reason about and maintain. Furthermore, the virtual
-table implementations don't receive the same level of fuzzing coverage as the
-SQLite core.
-
-Access to virtual tables is disabled by default for SQLite databases opened with
-Chrome's `sql::Database` infrastructure. This is intended to steer feature
-developers away from the discouraged feature.
-
-Chrome's SQLite build has virtual table functionality reduced to the minimum
-needed to support [FTS3](https://www.sqlite.org/fts3.html) in WebSQL, and an
-internal feature.
-[SQLite's run-time loading mechanism](https://www.sqlite.org/loadext.html) is
-disabled, and most
-[built-in virtual tables](https://www.sqlite.org/vtablist.html) are disabled as
-well.
-
-Ideally we would disable SQLite's virtual table support using
-[SQLITE_OMIT_VIRTUALTABLE](https://sqlite.org/compile.html#omit_virtualtable)
-once [WebSQL](https://www.w3.org/TR/webdatabase/) is removed from Chrome, but
-virtual table support is required to use SQLite's [built-in corruption recovery
-module](https://www.sqlite.org/recovery.html). The [SQLITE_DBPAGE virtual
-table](https://www.sqlite.org/dbpage.html) is also enabled only for corruption
-recovery and should not be used in Chrome.
-
 #### Foreign key constraints {#no-foreign-keys}
 
 [SQL foreign key constraints](https://sqlite.org/foreignkeys.html) should not be
@@ -670,3 +638,30 @@ model of query performance.
 We currently think that this maintenance overhead of window functions exceeds
 any convenience and performance benefits (compared to simpler queries
 coordinated in C++).
+
+#### Virtual tables {#no-virtual-tables}
+
+[`CREATE VIRTUAL TABLE` statements](https://www.sqlite.org/vtab.html) are
+disabled. The desired functionality should be implemented in C++, and access
+storage using standard SQL statements.
+
+Virtual tables are [SQLite's module system](https://www.sqlite.org/vtab.html).
+SQL statements on virtual tables are essentially running arbitrary code, which
+makes them very difficult to reason about and maintain. Furthermore, the virtual
+table implementations don't receive the same level of fuzzing coverage as the
+SQLite core.
+
+Chrome's SQLite build has virtual table functionality reduced to the minimum
+needed to support an internal feature.
+[SQLite's run-time loading mechanism](https://www.sqlite.org/loadext.html) is
+disabled, and most
+[built-in virtual tables](https://www.sqlite.org/vtablist.html) are disabled as
+well.
+
+Ideally we would disable SQLite's virtual table support using
+[SQLITE_OMIT_VIRTUALTABLE](https://sqlite.org/compile.html#omit_virtualtable)
+now that [WebSQL](https://www.w3.org/TR/webdatabase/) has been removed from
+Chrome, but virtual table support is required to use SQLite's
+[built-in corruption recovery module](https://www.sqlite.org/recovery.html). The
+[SQLITE_DBPAGE virtual table](https://www.sqlite.org/dbpage.html) is also
+enabled only for corruption recovery and should not be used in Chrome.

sql/database.cc

@@ -921,8 +921,7 @@ size_t Database::ComputeMmapSizeForOpen() {
 }
 
 int Database::SqlitePrepareFlags() const {
-  return options_.enable_virtual_tables_discouraged ? 0
-                                                    : SQLITE_PREPARE_NO_VTAB;
+  return enable_virtual_tables_ ? 0 : SQLITE_PREPARE_NO_VTAB;
 }
 
 sqlite3_file* Database::GetSqliteVfsFile() {
@@ -1009,8 +1008,6 @@ bool Database::Raze() {
       .page_size = options_.page_size,
       .cache_size = 0,
       .enable_views_discouraged = options_.enable_views_discouraged,
-      .enable_virtual_tables_discouraged =
-          options_.enable_virtual_tables_discouraged,
   });
   if (!null_db.OpenInMemory()) {
     DLOG(FATAL) << "Unable to open in-memory database.";

sql/database.h

@@ -202,15 +202,6 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
   // If this option is false, CREATE VIEW and DROP VIEW succeed, but SELECT
   // statements targeting views fail.
   bool enable_views_discouraged = false;
-
-  // If true, enables virtual tables (a discouraged feature) for this database.
-  //
-  // The use of virtual tables is discouraged for Chrome code. See README.md for
-  // details and recommended replacements.
-  //
-  // If this option is false, CREATE VIRTUAL TABLE and DROP VIRTUAL TABLE
-  // succeed, but statements targeting virtual tables fail.
-  bool enable_virtual_tables_discouraged = false;
 };
 
 // Holds database diagnostics in a structured format.
@@ -725,6 +716,7 @@ class COMPONENT_EXPORT(SQL) Database {
   FRIEND_TEST_ALL_PREFIXES(SQLDatabaseTest, ComputeMmapSizeForOpenAltStatus);
   FRIEND_TEST_ALL_PREFIXES(SQLDatabaseTest, OnMemoryDump);
   FRIEND_TEST_ALL_PREFIXES(SQLDatabaseTest, RegisterIntentToUpload);
+  FRIEND_TEST_ALL_PREFIXES(SQLiteFeaturesTest, FTS3_Prefix);
   FRIEND_TEST_ALL_PREFIXES(SQLiteFeaturesTest, WALNoClose);
   FRIEND_TEST_ALL_PREFIXES(SQLEmptyPathDatabaseTest, EmptyPathTest);
 
@@ -937,6 +929,10 @@ class COMPONENT_EXPORT(SQL) Database {
   // This method must only be called while the database is successfully opened.
   sqlite3_file* GetSqliteVfsFile();
 
+  void SetEnableVirtualTablesForTesting(bool enable) {
+    enable_virtual_tables_ = enable;
+  }
+
   // Will eventually be checked on all methods. See https://crbug.com/1306694
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -948,6 +944,10 @@ class COMPONENT_EXPORT(SQL) Database {
   // setters.
   DatabaseOptions options_;
 
+  // TODO(crbug.com/340805983): Remove this once virtual tables are no longer needed for
+  // WebSQL, which requires them for fts3 support.
+  bool enable_virtual_tables_ = false;
+
   // Holds references to all cached statements so they remain active.
   //
   // flat_map is appropriate here because the codebase has ~400 cached

sql/database_options_unittest.cc

@@ -271,55 +271,6 @@ TEST_P(DatabaseOptionsTest, EnableViewsDiscouraged_True) {
   EXPECT_TRUE(db.Execute("DROP VIEW IF EXISTS view"));
 }
 
-TEST_P(DatabaseOptionsTest, EnableVirtualTablesDiscouraged_FalseByDefault) {
-  DatabaseOptions options = {
-      .exclusive_locking = exclusive_locking(),
-      .wal_mode = wal_mode(),
-  };
-  EXPECT_FALSE(options.enable_virtual_tables_discouraged)
-      << "Invalid test assumption";
-
-  Database db(options);
-  OpenDatabase(db);
-
-  // sqlite3_prepare_v3() currently only disables accessing virtual tables.
-  // Schema operations on virtual tables are still allowed.
-  ASSERT_TRUE(db.Execute(
-      "CREATE VIRTUAL TABLE fts_table USING fts3(data_table, content TEXT)"));
-
-  {
-    sql::test::ScopedErrorExpecter expecter;
-    expecter.ExpectError(SQLITE_ERROR);
-    Statement select_from_vtable(db.GetUniqueStatement(
-        "SELECT content FROM fts_table WHERE content MATCH 'pattern'"));
-    EXPECT_FALSE(select_from_vtable.is_valid());
-    EXPECT_TRUE(expecter.SawExpectedErrors());
-  }
-
-  // sqlite3_prepare_v3() currently only disables accessing virtual tables.
-  // Schema operations on virtual tables are still allowed.
-  EXPECT_TRUE(db.Execute("DROP TABLE IF EXISTS fts_table"));
-}
-
-TEST_P(DatabaseOptionsTest, EnableVirtualTablesDiscouraged_True) {
-  Database db(DatabaseOptions{
-      .exclusive_locking = exclusive_locking(),
-      .wal_mode = wal_mode(),
-      .enable_virtual_tables_discouraged = true,
-  });
-  OpenDatabase(db);
-
-  ASSERT_TRUE(db.Execute(
-      "CREATE VIRTUAL TABLE fts_table USING fts3(data_table, content TEXT)"));
-
-  Statement select_from_vtable(db.GetUniqueStatement(
-      "SELECT content FROM fts_table WHERE content MATCH 'pattern'"));
-  ASSERT_TRUE(select_from_vtable.is_valid());
-  EXPECT_FALSE(select_from_vtable.Step());
-
-  EXPECT_TRUE(db.Execute("DROP TABLE IF EXISTS fts_table"));
-}
-
 INSTANTIATE_TEST_SUITE_P(
     ,
     DatabaseOptionsTest,

sql/sqlite_features_unittest.cc

@@ -93,7 +93,8 @@ TEST_F(SQLiteFeaturesTest, FTS3) {
 // "*"}.  Test that fts3 works correctly.
 TEST_F(SQLiteFeaturesTest, FTS3_Prefix) {
   db_.Close();
-  sql::Database db({.enable_virtual_tables_discouraged = true});
+  sql::Database db;
+  db.SetEnableVirtualTablesForTesting(true);
   ASSERT_TRUE(db.Open(db_path_));
 
   static constexpr char kCreateSql[] =