Mark sql::Transaction::Begin [[nodiscard]]

Assuming that transactions are primarily used for correctness with
respect to write atomicity, callers should be expected to check the
return value of Begin and retry at a higher level if necessary, rather
than silently proceed with non-transactional writes.

Note that we make a slight behavioral change to
chrome/browser/android/explore_sites/get_catalog_task.cc to account for
this, since it has an obvious implementation.

Change-Id: Ifebe9fc56ea9a26c9bcc054cfe1240de4ff79c7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3895361
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Scott Violet <[email protected]>
Commit-Queue: Andrew Paseltiner <[email protected]>
Quick-Run: Andrew Paseltiner <[email protected]>
Reviewed-by: Justin DeWitt <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1048231}

chrome/browser/android/explore_sites/get_catalog_task.cc

@@ -110,7 +110,8 @@ GetCatalogSync(bool update_current, sql::Database* db) {
   if (update_current) {
     DVLOG(1) << "Updating current catalog from " << catalog_version_token;
     sql::Transaction transaction(db);
-    transaction.Begin();
+    if (!transaction.Begin())
+      return std::make_pair(GetCatalogStatus::kFailed, nullptr);
     catalog_version_token =
         UpdateCurrentCatalogIfNewer(&meta_table, catalog_version_token);
     if (catalog_version_token == "")

chrome/browser/extensions/activity_log/database_string_table_unittest.cc

@@ -142,7 +142,7 @@ TEST_F(DatabaseStringTableTest, Prune) {
   // Wrap the lookups in a transaction to improve performance.
   sql::Transaction transaction(&db_);
 
-  transaction.Begin();
+  ASSERT_TRUE(transaction.Begin());
   for (int i = 0; i < 2000; i++) {
     int64_t id;
     ASSERT_TRUE(table.StringToInt(&db_, base::StringPrintf("value-%d", i),

components/history/core/browser/top_sites_database.cc

@@ -335,7 +335,7 @@ bool TopSitesDatabase::InitImpl(const base::FilePath& db_name) {
 void TopSitesDatabase::ApplyDelta(const TopSitesDelta& delta) {
   sql::Transaction transaction(db_.get());
   // TODO: consider returning early if `Begin()` returns false.
-  transaction.Begin();
+  std::ignore = transaction.Begin();
 
   for (const auto& deleted : delta.deleted) {
     if (!RemoveURLNoTransaction(deleted))

components/history/core/browser/top_sites_database_unittest.cc

@@ -123,7 +123,7 @@ TEST_F(TopSitesDatabaseTest, Version4) {
   EXPECT_EQ(kUrl0, urls[0].url);  // [0] because of url_rank.
 
   sql::Transaction transaction(db.db_.get());
-  transaction.Begin();
+  ASSERT_TRUE(transaction.Begin());
   ASSERT_TRUE(db.RemoveURLNoTransaction(urls[1]));
   transaction.Commit();
 

net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

@@ -1177,7 +1177,8 @@ bool AddV9CookiesToDBImpl(sql::Database* db,
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const auto& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(
@@ -1652,7 +1653,8 @@ bool AddV10CookiesToDBImpl(sql::Database* db,
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const auto& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(
@@ -2026,7 +2028,8 @@ bool AddV11CookiesToDB(sql::Database* db) {
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const auto& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(
@@ -2073,7 +2076,8 @@ bool AddV12CookiesToDB(sql::Database* db) {
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const CanonicalCookie& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(
@@ -2121,7 +2125,8 @@ bool AddV13CookiesToDB(sql::Database* db) {
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const CanonicalCookie& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(
@@ -2171,7 +2176,8 @@ bool AddV15CookiesToDB(sql::Database* db) {
   if (!statement.is_valid())
     return false;
   sql::Transaction transaction(db);
-  transaction.Begin();
+  if (!transaction.Begin())
+    return false;
   for (const CanonicalCookie& cookie : cookies) {
     statement.Reset(true);
     statement.BindInt64(

sql/transaction.h

@@ -46,14 +46,14 @@ class COMPONENT_EXPORT(SQL) Transaction {
   // carried out in the transaction.
   //
   // In most cases (no nested transactions), this method issues a BEGIN
-  // statemnent, which invokes SQLite's deferred transaction startup documented
+  // statement, which invokes SQLite's deferred transaction startup documented
   // in https://www.sqlite.org/lang_transaction.html. This means the database
   // lock is not acquired by the time Begin() completes. Instead, the first
   // statement after Begin() will attempt to acquire a read or write lock.
   //
   // This method is not idempotent. Calling Begin() twice on a Transaction will
   // cause a DCHECK crash.
-  bool Begin();
+  [[nodiscard]] bool Begin();
 
   // Explicitly rolls back the transaction. All changes will be forgotten.
   //
@@ -69,9 +69,9 @@ class COMPONENT_EXPORT(SQL) Transaction {
 
   // Commits the transaction. All changes will be persisted in the database.
   //
-  // Returns false in case of failure. The most failure case is a SQLite failure
-  // in committing the transaction. If sql::Database's support for nested
-  // transactions is in use, this method will also fail if any nested
+  // Returns false in case of failure. The most common failure case is a SQLite
+  // failure in committing the transaction. If sql::Database's support for
+  // nested transactions is in use, this method will also fail if any nested
   // transaction has been rolled back.
   //
   // This method is not idempotent. Calling Commit() twice on a Transaction will