[conversions] Lazily create and open sql database

What: Defer creation/opening of the sql database until there is a
storage operation which needs it.

Why: We unconditionally create the database for every storage partition
whether the API is being used which is unnecessary. This has the added
benefit of not forcing the database to be initialized in every
browsertest once  the feature is enabled by default.

How: Use the presence of the database file as a signal for creation.
If the file path doesn't exist, we can defer creation until an
impression is stored. No other operations need to create the database
as it is empty and would be a no-op regardless.

If the database path does exist, we defer opening the database until
any operation is called, as there may be conversions that need to be
sent or impression to convert etc.

Change-Id: I25446805a991f04fcbf9aa237195950d055558aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360471
Commit-Queue: John Delaney <johnidel@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799370}

content/browser/conversions/conversion_storage.h

@@ -18,7 +18,9 @@
 namespace content {
 
 // This class provides an interface for persisting impression/conversion data to
-// disk, and performing queries on it.
+// disk, and performing queries on it. ConversionStorage should initialize
+// itself. Calls to a ConversionStorage instance that failed to initialize
+// properly should result in no-ops.
 class ConversionStorage {
  public:
   // Storage delegate that can supplied to extend basic conversion storage
@@ -64,10 +66,6 @@ class ConversionStorage {
   // When adding a new method, also add it to
   // ConversionStorageTest.StorageUsedAfterFailedInitilization_FailsSilently.
 
-  // Initializes the storage. Returns true on success, otherwise the storage
-  // should not be used.
-  virtual bool Initialize() = 0;
-
   // Add |impression| to storage. Two impressions are considered
   // matching when they share a <reporting_origin, conversion_origin> pair. When
   // an impression is stored, all matching impressions that have

content/browser/conversions/conversion_storage_context.cc

@@ -18,19 +18,14 @@ ConversionStorageContext::ConversionStorageContext(
       storage_(new ConversionStorageSql(user_data_directory,
                                         std::move(delegate),
                                         clock),
-               base::OnTaskRunnerDeleter(storage_task_runner_)) {
-  // Unretained is safe when posting to |storage_task_runner_| because any task
-  // to delete |storage_| will be posted after this one.
-  storage_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(base::IgnoreResult(&ConversionStorage::Initialize),
-                     base::Unretained(storage_.get())));
-}
+               base::OnTaskRunnerDeleter(storage_task_runner_)) {}
 
 ConversionStorageContext::~ConversionStorageContext() = default;
 
 void ConversionStorageContext::StoreImpression(
     const StorableImpression& impression) {
+  // Unretained is safe when posting to |storage_task_runner_| because any task
+  // to delete |storage_| will be posted after this one.
   storage_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&ConversionStorage::StoreImpression,
                                 base::Unretained(storage_.get()), impression));

content/browser/conversions/conversion_storage_sql.cc

@@ -75,51 +75,26 @@ ConversionStorageSql::ConversionStorageSql(
       weak_factory_(this) {
   DCHECK(delegate_);
   DETACH_FROM_SEQUENCE(sequence_checker_);
-}
-
-ConversionStorageSql::~ConversionStorageSql() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-}
-
-bool ConversionStorageSql::Initialize() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  db_ = std::make_unique<sql::Database>();
-  db_->set_histogram_tag("Conversions");
-
-  // Supply this callback with a weak_ptr to avoid calling the error callback
-  // after |this| has been deleted.
-  db_->set_error_callback(
-      base::BindRepeating(&ConversionStorageSql::DatabaseErrorCallback,
-                          weak_factory_.GetWeakPtr()));
-  db_->set_page_size(4096);
-  db_->set_cache_size(32);
-  db_->set_exclusive_locking();
 
-  const base::FilePath& dir = path_to_database_.DirName();
-  bool opened = false;
-  if (path_to_database_.value() == kInMemoryPath) {
-    opened = db_->OpenInMemory();
-  } else if (base::DirectoryExists(dir) || base::CreateDirectory(dir)) {
-    opened = db_->Open(path_to_database_);
+  if (g_run_in_memory_) {
+    db_init_status_ = DbStatus::kDeferringCreation;
   } else {
-    DLOG(ERROR) << "Failed to create directory for Conversion database";
-  }
-
-  if (!opened || !InitializeSchema()) {
-    db_.reset();
-    db_is_open_ = false;
-    return false;
+    db_init_status_ = base::PathExists(path_to_database_)
+                          ? DbStatus::kDeferringOpen
+                          : DbStatus::kDeferringCreation;
   }
+}
 
-  db_is_open_ = true;
-  return true;
+ConversionStorageSql::~ConversionStorageSql() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 void ConversionStorageSql::StoreImpression(
     const StorableImpression& impression) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  // Force the creation of the database if it doesn't exist, as we need to
+  // persist the impression.
+  if (!LazyInit(DbCreationPolicy::kCreateIfAbsent))
     return;
 
   // Cleanup any impression that may be expired by this point. This is done when
@@ -181,7 +156,7 @@ void ConversionStorageSql::StoreImpression(
 int ConversionStorageSql::MaybeCreateAndStoreConversionReports(
     const StorableConversion& conversion) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return 0;
 
   const url::Origin& conversion_origin = conversion.conversion_origin();
@@ -306,7 +281,7 @@ int ConversionStorageSql::MaybeCreateAndStoreConversionReports(
 std::vector<ConversionReport> ConversionStorageSql::GetConversionsToReport(
     base::Time max_report_time) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return {};
 
   // Get all entries in the conversions table with a |report_time| less than
@@ -367,7 +342,7 @@ std::vector<ConversionReport> ConversionStorageSql::GetConversionsToReport(
 
 std::vector<StorableImpression> ConversionStorageSql::GetActiveImpressions() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return {};
 
   const char kGetImpressionsSql[] =
@@ -402,7 +377,7 @@ std::vector<StorableImpression> ConversionStorageSql::GetActiveImpressions() {
 
 int ConversionStorageSql::DeleteExpiredImpressions() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return 0;
 
   // Delete all impressions that have no associated conversions and are past
@@ -434,7 +409,7 @@ int ConversionStorageSql::DeleteExpiredImpressions() {
 
 bool ConversionStorageSql::DeleteConversion(int64_t conversion_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return false;
 
   // Delete the row identified by |conversion_id|.
@@ -455,7 +430,7 @@ void ConversionStorageSql::ClearData(
     base::Time delete_end,
     base::RepeatingCallback<bool(const url::Origin&)> filter) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!db_is_open_)
+  if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent))
     return;
 
   SCOPED_UMA_HISTOGRAM_TIMER("Conversions.ClearDataTime");
@@ -650,6 +625,55 @@ bool ConversionStorageSql::HasCapacityForStoringConversion(
   return count < delegate_->GetMaxConversionsPerOrigin();
 }
 
+bool ConversionStorageSql::LazyInit(DbCreationPolicy creation_policy) {
+  switch (db_init_status_) {
+    // If the database file has not been created, we defer creation until
+    // storage needs to be used for an operation which needs to operate even on
+    // an empty database.
+    case DbStatus::kDeferringCreation:
+      if (creation_policy == DbCreationPolicy::kIgnoreIfAbsent)
+        return false;
+      break;
+    case DbStatus::kDeferringOpen:
+      break;
+    case DbStatus::kClosed:
+      return false;
+    case DbStatus::kOpen:
+      return true;
+  }
+
+  db_ = std::make_unique<sql::Database>();
+  db_->set_histogram_tag("Conversions");
+
+  // Supply this callback with a weak_ptr to avoid calling the error callback
+  // after |this| has been deleted.
+  db_->set_error_callback(
+      base::BindRepeating(&ConversionStorageSql::DatabaseErrorCallback,
+                          weak_factory_.GetWeakPtr()));
+  db_->set_page_size(4096);
+  db_->set_cache_size(32);
+  db_->set_exclusive_locking();
+
+  const base::FilePath& dir = path_to_database_.DirName();
+  bool opened = false;
+  if (path_to_database_.value() == kInMemoryPath) {
+    opened = db_->OpenInMemory();
+  } else if (base::DirectoryExists(dir) || base::CreateDirectory(dir)) {
+    opened = db_->Open(path_to_database_);
+  } else {
+    DLOG(ERROR) << "Failed to create directory for Conversion database";
+  }
+
+  if (!opened || !InitializeSchema()) {
+    db_.reset();
+    db_init_status_ = DbStatus::kClosed;
+    return false;
+  }
+
+  db_init_status_ = DbStatus::kOpen;
+  return true;
+}
+
 bool ConversionStorageSql::InitializeSchema() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(johnidel, csharrison): Many impressions will share a target origin and
@@ -780,7 +804,7 @@ void ConversionStorageSql::DatabaseErrorCallback(int extended_error,
 
   // Consider the  database closed if we did not attempt to recover so we did
   // not produce further errors.
-  db_is_open_ = false;
+  db_init_status_ = DbStatus::kClosed;
 }
 
 }  // namespace content

content/browser/conversions/conversion_storage_sql.h

@@ -42,8 +42,26 @@ class CONTENT_EXPORT ConversionStorageSql : public ConversionStorage {
   }
 
  private:
+  enum class DbStatus {
+    // The database has never been created, i.e. there is no database file at
+    // all.
+    kDeferringCreation,
+    // The database exists but is not open yet.
+    kDeferringOpen,
+    // The database initialization failed, or the db suffered from an
+    // unrecoverable error.
+    kClosed,
+    kOpen,
+  };
+
+  enum class DbCreationPolicy {
+    // Create the db if it does not exist.
+    kCreateIfAbsent,
+    // Do not create the db if it does not exist.
+    kIgnoreIfAbsent,
+  };
+
   // ConversionStorage
-  bool Initialize() override;
   void StoreImpression(const StorableImpression& impression) override;
   int MaybeCreateAndStoreConversionReports(
       const StorableConversion& conversion) override;
@@ -64,6 +82,10 @@ class CONTENT_EXPORT ConversionStorageSql : public ConversionStorage {
   bool HasCapacityForStoringImpression(const std::string& serialized_origin);
   bool HasCapacityForStoringConversion(const std::string& serialized_origin);
 
+  // Initializes the database if necessary, and returns whether the database is
+  // open. |should_create| indicates whether the database should be created if
+  // it is not already.
+  bool LazyInit(DbCreationPolicy creation_policy);
   bool InitializeSchema();
 
   void DatabaseErrorCallback(int extended_error, sql::Statement* stmt);
@@ -75,9 +97,10 @@ class CONTENT_EXPORT ConversionStorageSql : public ConversionStorage {
 
   const base::FilePath path_to_database_;
 
-  // Whether the db is open and should be accessed. False if database
-  // initialization failed, or if the db suffered from an unrecoverable error.
-  bool db_is_open_ = false;
+  // Current status of the database initialization. Tracks what stage |this| is
+  // at for lazy initialization, and used as a signal for if the database is
+  // closed.
+  DbStatus db_init_status_;
 
   // May be null if the database:
   //  - could not be opened

content/browser/conversions/conversion_storage_sql_unittest.cc

@@ -36,7 +36,6 @@ class ConversionStorageSqlTest : public testing::Test {
     delegate_ = delegate.get();
     storage_ = std::make_unique<ConversionStorageSql>(
         temp_directory_.GetPath(), std::move(delegate), &clock_);
-    EXPECT_TRUE(storage_->Initialize());
   }
 
   void CloseDatabase() { storage_.reset(); }
@@ -66,19 +65,38 @@ class ConversionStorageSqlTest : public testing::Test {
 };
 
 TEST_F(ConversionStorageSqlTest,
-       DatabaseInitialized_TablesAndIndexesInitialized) {
+       DatabaseInitialized_TablesAndIndexesLazilyInitialized) {
   OpenDatabase();
   CloseDatabase();
-  sql::Database raw_db;
-  EXPECT_TRUE(raw_db.Open(db_path()));
 
-  // [impressions] and [conversions].
-  EXPECT_EQ(2u, sql::test::CountSQLTables(&raw_db));
+  // An unused ConversionStorageSql instance should not create the database.
+  EXPECT_FALSE(base::PathExists(db_path()));
+
+  // Operations which don't need to run on an empty database should not create
+  // the database.
+  OpenDatabase();
+  EXPECT_EQ(0u, storage()->GetConversionsToReport(clock()->Now()).size());
+  CloseDatabase();
+
+  EXPECT_FALSE(base::PathExists(db_path()));
+
+  // Storing an impression should create and initialize the database.
+  OpenDatabase();
+  storage()->StoreImpression(ImpressionBuilder(clock()->Now()).Build());
+  CloseDatabase();
+
+  {
+    sql::Database raw_db;
+    EXPECT_TRUE(raw_db.Open(db_path()));
 
-  // [conversion_origin_idx], [impression_expiry_idx],
-  // [impression_origin_idx], [conversion_report_time_idx],
-  // [conversion_impression_id_idx].
-  EXPECT_EQ(5u, sql::test::CountSQLIndices(&raw_db));
+    // [impressions] and [conversions].
+    EXPECT_EQ(2u, sql::test::CountSQLTables(&raw_db));
+
+    // [conversion_origin_idx], [impression_expiry_idx],
+    // [impression_origin_idx], [conversion_report_time_idx],
+    // [conversion_impression_id_idx].
+    EXPECT_EQ(5u, sql::test::CountSQLIndices(&raw_db));
+  }
 }
 
 TEST_F(ConversionStorageSqlTest, DatabaseReopened_DataPersisted) {
@@ -270,10 +288,12 @@ TEST_F(ConversionStorageSqlTest, CantOpenDb_FailsSilentlyInRelease) {
       std::make_unique<ConfigurableStorageDelegate>(), clock());
   sql_storage->set_ignore_errors_for_testing(true);
 
-  // Initialize() is private on ConserionStorageSql, so convert to
-  // ConversionStorage.
   std::unique_ptr<ConversionStorage> storage = std::move(sql_storage);
-  EXPECT_FALSE(storage->Initialize());
+
+  // These calls should be no-ops.
+  storage->StoreImpression(ImpressionBuilder(clock()->Now()).Build());
+  EXPECT_EQ(0,
+            storage->MaybeCreateAndStoreConversionReports(DefaultConversion()));
 }
 
 TEST_F(ConversionStorageSqlTest, DatabaseDirDoesExist_CreateDirAndOpenDB) {
@@ -285,7 +305,9 @@ TEST_F(ConversionStorageSqlTest, DatabaseDirDoesExist_CreateDirAndOpenDB) {
           std::make_unique<ConfigurableStorageDelegate>(), clock());
 
   // The directory should be created, and the database opened.
-  EXPECT_TRUE(storage->Initialize());
+  storage->StoreImpression(ImpressionBuilder(clock()->Now()).Build());
+  EXPECT_EQ(1,
+            storage->MaybeCreateAndStoreConversionReports(DefaultConversion()));
 }
 
 }  // namespace content

content/browser/conversions/conversion_storage_unittest.cc

@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -56,7 +57,6 @@ class ConversionStorageTest : public testing::Test {
     delegate_ = delegate.get();
     storage_ = std::make_unique<ConversionStorageSql>(
         dir_.GetPath(), std::move(delegate), &clock_);
-    EXPECT_TRUE(storage_->Initialize());
   }
 
   // Given a |conversion|, returns the expected conversion report properties at
@@ -99,11 +99,16 @@ class ConversionStorageTest : public testing::Test {
 
 TEST_F(ConversionStorageTest,
        StorageUsedAfterFailedInitilization_FailsSilently) {
-  // We fake a failed initialization by never initializing |storage|.
+  // We create a failed initialization by writing a dir to the database file
+  // path.
+  base::CreateDirectoryAndGetError(
+      dir_.GetPath().Append(FILE_PATH_LITERAL("Conversions")), nullptr);
   std::unique_ptr<ConversionStorage> storage =
       std::make_unique<ConversionStorageSql>(
           dir_.GetPath(), std::make_unique<ConfigurableStorageDelegate>(),
           clock());
+  static_cast<ConversionStorageSql*>(storage.get())
+      ->set_ignore_errors_for_testing(true);
 
   // Test all public methods on ConversionStorage.
   EXPECT_NO_FATAL_FAILURE(