[sql] WARN_UNUSED_RESULT on Execute().

Goal is to encourage callers to handle errors, especially in cases
like schema changes, where dropped errors can result in broken
databases.

Many CREATE INDEX calls where ignoring errors rather than checking if
the index already existed before creating it.  Recoded those to CREATE
INDEX IF NOT EXISTS, which is for exactly this purpose.

BUG=none
TEST=none


Review URL: http://codereview.chromium.org/9005036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@115725 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 100 insertions, 86 deletions

diff --git a/chrome/browser/history/history_database.cc b/chrome/browser/history/history_database.cc
index 18bb489..7a37245 100644
--- a/chrome/browser/history/history_database.cc
+++ b/chrome/browser/history/history_database.cc
@@ -130,7 +130,7 @@ sql::InitStatus HistoryDatabase::Init(const FilePath& history_name,
 void HistoryDatabase::BeginExclusiveMode() {
   // We can't use set_exclusive_locking() since that only has an effect before
   // the DB is opened.
-  db_.Execute("PRAGMA locking_mode=EXCLUSIVE");
+  ignore_result(db_.Execute("PRAGMA locking_mode=EXCLUSIVE"));
 }
 
 // static
@@ -172,7 +172,7 @@ bool HistoryDatabase::RecreateAllTablesButURL() {
 void HistoryDatabase::Vacuum() {
   DCHECK_EQ(0, db_.transaction_nesting()) <<
       "Can not have a transaction when vacuuming.";
-  db_.Execute("VACUUM");
+  ignore_result(db_.Execute("VACUUM"));
 }
 
 void HistoryDatabase::ThumbnailMigrationDone() {
@@ -323,18 +323,18 @@ void HistoryDatabase::MigrateTimeEpoch() {
   // Update all the times in the URLs and visits table in the main database.
   // For visits, clear the indexed flag since we'll delete the FTS databases in
   // the next step.
-  db_.Execute(
+  ignore_result(db_.Execute(
       "UPDATE urls "
       "SET last_visit_time = last_visit_time + 11644473600000000 "
-      "WHERE id IN (SELECT id FROM urls WHERE last_visit_time > 0);");
-  db_.Execute(
+      "WHERE id IN (SELECT id FROM urls WHERE last_visit_time > 0);"));
+  ignore_result(db_.Execute(
       "UPDATE visits "
       "SET visit_time = visit_time + 11644473600000000, is_indexed = 0 "
-      "WHERE id IN (SELECT id FROM visits WHERE visit_time > 0);");
-  db_.Execute(
+      "WHERE id IN (SELECT id FROM visits WHERE visit_time > 0);"));
+  ignore_result(db_.Execute(
       "UPDATE segment_usage "
       "SET time_slot = time_slot + 11644473600000000 "
-      "WHERE id IN (SELECT id FROM segment_usage WHERE time_slot > 0);");
+      "WHERE id IN (SELECT id FROM segment_usage WHERE time_slot > 0);"));
 
   // Erase all the full text index files. These will take a while to update and
   // are less important, so we just blow them away. Same with the archived
diff --git a/chrome/browser/history/in_memory_database.cc b/chrome/browser/history/in_memory_database.cc
index a2ee461..cf64b5a 100644
--- a/chrome/browser/history/in_memory_database.cc
+++ b/chrome/browser/history/in_memory_database.cc
@@ -29,10 +29,10 @@ bool InMemoryDatabase::InitDB() {
   }
 
   // No reason to leave data behind in memory when rows are removed.
-  db_.Execute("PRAGMA auto_vacuum=1");
+  ignore_result(db_.Execute("PRAGMA auto_vacuum=1"));
 
   // Ensure this is really an in-memory-only cache.
-  db_.Execute("PRAGMA temp_store=MEMORY");
+  ignore_result(db_.Execute("PRAGMA temp_store=MEMORY"));
 
   // Create the URL table, but leave it empty for now.
   if (!CreateURLTable(false)) {
diff --git a/chrome/browser/history/shortcuts_database.cc b/chrome/browser/history/shortcuts_database.cc
index 45e11a9..5b761df 100644
--- a/chrome/browser/history/shortcuts_database.cc
+++ b/chrome/browser/history/shortcuts_database.cc
@@ -168,17 +168,10 @@ bool ShortcutsDatabase::DeleteShortcutsWithUrl(
 }
 
 bool ShortcutsDatabase::DeleteAllShortcuts() {
-  sql::Statement s(db_.GetCachedStatement(SQL_FROM_HERE,
-      "DROP TABLE " kShortcutsDBName));
-  if (!s) {
-    NOTREACHED() << "Statement prepare failed";
+  if (!db_.Execute("DELETE FROM " kShortcutsDBName))
     return false;
-  }
 
-  if (!s.Run())
-    return false;
-  EnsureTable();
-  db_.Execute("VACUUM");
+  ignore_result(db_.Execute("VACUUM"));
   return true;
 }
 
diff --git a/chrome/browser/history/text_database.cc b/chrome/browser/history/text_database.cc
index c5da03c..310f52b 100644
--- a/chrome/browser/history/text_database.cc
+++ b/chrome/browser/history/text_database.cc
@@ -201,10 +201,8 @@ bool TextDatabase::CreateTables() {
       return false;
   }
 
-  // Create the index. This will fail when the index already exists, so we just
-  // ignore the error.
-  db_.Execute("CREATE INDEX info_time ON info(time)");
-  return true;
+  // Create the index.
+  return db_.Execute("CREATE INDEX IF NOT EXISTS info_time ON info(time)");
 }
 
 bool TextDatabase::AddPageData(base::Time time,
diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc
index 82aed1a..6827077 100644
--- a/chrome/browser/history/thumbnail_database.cc
+++ b/chrome/browser/history/thumbnail_database.cc
@@ -85,12 +85,12 @@ sql::InitStatus ThumbnailDatabase::Init(
                         kCompatibleVersionNumber) ||
       !InitThumbnailTable() ||
       !InitFaviconsTable(&db_, false) ||
-      !InitIconMappingTable(&db_, false)) {
+      !InitFaviconsIndex() ||
+      !InitIconMappingTable(&db_, false) ||
+      !InitIconMappingIndex()) {
     db_.Close();
     return sql::INIT_FAILURE;
   }
-  InitFaviconsIndex();
-  InitIconMappingIndex();
 
   // Version check. We should not encounter a database too old for us to handle
   // in the wild, so we try to continue in that case.
@@ -219,10 +219,10 @@ bool ThumbnailDatabase::InitFaviconsTable(sql::Connection* db,
   return true;
 }
 
-void ThumbnailDatabase::InitFaviconsIndex() {
-  // Add an index on the url column. We ignore errors. Since this is always
-  // called during startup, the index will normally already exist.
-  db_.Execute("CREATE INDEX favicons_url ON favicons(url)");
+bool ThumbnailDatabase::InitFaviconsIndex() {
+  // Add an index on the url column.
+  return
+      db_.Execute("CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)");
 }
 
 void ThumbnailDatabase::BeginTransaction() {
@@ -236,7 +236,7 @@ void ThumbnailDatabase::CommitTransaction() {
 void ThumbnailDatabase::Vacuum() {
   DCHECK(db_.transaction_nesting() == 0) <<
       "Can not have a transaction when vacuuming.";
-  db_.Execute("VACUUM");
+  ignore_result(db_.Execute("VACUUM"));
 }
 
 void ThumbnailDatabase::SetPageThumbnail(
@@ -605,9 +605,7 @@ bool ThumbnailDatabase::CommitTemporaryIconMappingTable() {
     return false;
 
   // The renamed table needs the index (the temporary table doesn't have one).
-  InitIconMappingIndex();
-
-  return true;
+  return InitIconMappingIndex();
 }
 
 FaviconID ThumbnailDatabase::CopyToTemporaryFaviconTable(FaviconID source) {
@@ -635,8 +633,7 @@ bool ThumbnailDatabase::CommitTemporaryFaviconTable() {
     return false;
 
   // The renamed table needs the index (the temporary table doesn't have one).
-  InitFaviconsIndex();
-  return true;
+  return InitFaviconsIndex();
 }
 
 bool ThumbnailDatabase::NeedsMigrationToTopSites() {
@@ -713,7 +710,8 @@ bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
   if (!meta_table_.Init(&db_, kCurrentVersionNumber, kCompatibleVersionNumber))
     return false;
 
-  InitFaviconsIndex();
+  if (!InitFaviconsIndex())
+    return false;
 
   // Reopen the transaction.
   BeginTransaction();
@@ -738,12 +736,13 @@ bool ThumbnailDatabase::InitIconMappingTable(sql::Connection* db,
   return true;
 }
 
-void ThumbnailDatabase::InitIconMappingIndex() {
-  // Add an index on the url column. We ignore errors. Since this is always
-  // called during startup, the index will normally already exist.
-  db_.Execute("CREATE INDEX icon_mapping_page_url_idx"
-              " ON icon_mapping(page_url)");
-  db_.Execute("CREATE INDEX icon_mapping_icon_id_idx ON icon_mapping(icon_id)");
+bool ThumbnailDatabase::InitIconMappingIndex() {
+  // Add an index on the url column.
+  return
+      db_.Execute("CREATE INDEX IF NOT EXISTS icon_mapping_page_url_idx"
+                  " ON icon_mapping(page_url)") &&
+      db_.Execute("CREATE INDEX IF NOT EXISTS icon_mapping_icon_id_idx"
+                  " ON icon_mapping(icon_id)");
 }
 
 IconMappingID ThumbnailDatabase::AddIconMapping(const GURL& page_url,
diff --git a/chrome/browser/history/thumbnail_database.h b/chrome/browser/history/thumbnail_database.h
index 0611e53..fff1fc5 100644
--- a/chrome/browser/history/thumbnail_database.h
+++ b/chrome/browser/history/thumbnail_database.h
@@ -267,7 +267,7 @@ class ThumbnailDatabase {
   // initialization after the table is created. This is a separate function
   // because it is used by SwapFaviconTables to create an index over the
   // newly-renamed favicons table (formerly the temporary table with no index).
-  void InitFaviconsIndex();
+  bool InitFaviconsIndex();
 
   // Creates the icon_map table, return true if the table already exists or was
   // successfully created.
@@ -278,7 +278,7 @@ class ThumbnailDatabase {
   // because it is used by CommitTemporaryIconMappingTable to create an index
   // over the newly-renamed icon_mapping table (formerly the temporary table
   // with no index).
-  void InitIconMappingIndex();
+  bool InitIconMappingIndex();
 
   // Adds a mapping between the given page_url and icon_id; The mapping will be
   // added to temp_icon_mapping table if is_temporary is true.
diff --git a/chrome/browser/history/url_database.cc b/chrome/browser/history/url_database.cc
index 834ece4..4a020b9 100644
--- a/chrome/browser/history/url_database.cc
+++ b/chrome/browser/history/url_database.cc
@@ -406,14 +406,21 @@ bool URLDatabase::InitKeywordSearchTermsTable() {
   return true;
 }
 
-void URLDatabase::CreateKeywordSearchTermsIndices() {
+bool URLDatabase::CreateKeywordSearchTermsIndices() {
   // For searching.
-  GetDB().Execute("CREATE INDEX keyword_search_terms_index1 ON "
-                  "keyword_search_terms (keyword_id, lower_term)");
+  if (!GetDB().Execute(
+          "CREATE INDEX IF NOT EXISTS keyword_search_terms_index1 ON "
+          "keyword_search_terms (keyword_id, lower_term)")) {
+    return false;
+  }
 
   // For deletion.
-  GetDB().Execute("CREATE INDEX keyword_search_terms_index2 ON "
-                  "keyword_search_terms (url_id)");
+  if (!GetDB().Execute(
+          "CREATE INDEX IF NOT EXISTS keyword_search_terms_index2 ON "
+          "keyword_search_terms (url_id)")) {
+    return false;
+  }
+  return true;
 }
 
 bool URLDatabase::DropKeywordSearchTermsTable() {
@@ -569,10 +576,10 @@ bool URLDatabase::CreateURLTable(bool is_temporary) {
   return GetDB().Execute(sql.c_str());
 }
 
-void URLDatabase::CreateMainURLIndex() {
-  // Index over URLs so we can quickly look up based on URL.  Ignore errors as
-  // this likely already exists (and the same below).
-  GetDB().Execute("CREATE INDEX urls_url_index ON urls (url)");
+bool URLDatabase::CreateMainURLIndex() {
+  // Index over URLs so we can quickly look up based on URL.
+  return GetDB().Execute(
+      "CREATE INDEX IF NOT EXISTS urls_url_index ON urls (url)");
 }
 
 }  // namespace history
diff --git a/chrome/browser/history/url_database.h b/chrome/browser/history/url_database.h
index 837a957..4d19ff3 100644
--- a/chrome/browser/history/url_database.h
+++ b/chrome/browser/history/url_database.h
@@ -252,13 +252,13 @@ class URLDatabase {
   bool CreateURLTable(bool is_temporary);
   // We have two tiers of indices for the URL table. The main tier is used by
   // all URL databases, and is an index over the URL itself.
-  void CreateMainURLIndex();
+  bool CreateMainURLIndex();
 
   // Ensures the keyword search terms table exists.
   bool InitKeywordSearchTermsTable();
 
   // Creates the indices used for keyword search terms.
-  void CreateKeywordSearchTermsIndices();
+  bool CreateKeywordSearchTermsIndices();
 
   // Deletes the keyword search terms table.
   bool DropKeywordSearchTermsTable();
diff --git a/chrome/browser/history/visit_database.cc b/chrome/browser/history/visit_database.cc
index 9adfa57..59592a4 100644
--- a/chrome/browser/history/visit_database.cc
+++ b/chrome/browser/history/visit_database.cc
@@ -62,26 +62,33 @@ bool VisitDatabase::InitVisitTable() {
         return false;
   }
 
-  // Index over url so we can quickly find visits for a page. This will just
-  // fail if it already exists and we'll ignore it.
-  GetDB().Execute("CREATE INDEX visits_url_index ON visits (url)");
+  // Index over url so we can quickly find visits for a page.
+  if (!GetDB().Execute(
+          "CREATE INDEX IF NOT EXISTS visits_url_index ON visits (url)"))
+    return false;
 
   // Create an index over from visits so that we can efficiently find
-  // referrers and redirects. Ignore failures because it likely already exists.
-  GetDB().Execute("CREATE INDEX visits_from_index ON visits (from_visit)");
+  // referrers and redirects.
+  if (!GetDB().Execute(
+          "CREATE INDEX IF NOT EXISTS visits_from_index ON "
+          "visits (from_visit)"))
+    return false;
 
   // Create an index over time so that we can efficiently find the visits in a
-  // given time range (most history views are time-based). Ignore failures
-  // because it likely already exists.
-  GetDB().Execute("CREATE INDEX visits_time_index ON visits (visit_time)");
+  // given time range (most history views are time-based).
+  if (!GetDB().Execute(
+          "CREATE INDEX IF NOT EXISTS visits_time_index ON "
+          "visits (visit_time)"))
+    return false;
 
   return true;
 }
 
 bool VisitDatabase::DropVisitTable() {
-  GetDB().Execute("DROP TABLE visit_source");
   // This will also drop the indices over the table.
-  return GetDB().Execute("DROP TABLE visits");
+  return
+      GetDB().Execute("DROP TABLE IF EXISTS visit_source") &&
+      GetDB().Execute("DROP TABLE visits");
 }
 
 // Must be in sync with HISTORY_VISIT_ROW_FIELDS.
diff --git a/chrome/browser/history/visitsegment_database.cc b/chrome/browser/history/visitsegment_database.cc
index ac2afe8..c8bbfd3 100644
--- a/chrome/browser/history/visitsegment_database.cc
+++ b/chrome/browser/history/visitsegment_database.cc
@@ -60,7 +60,9 @@ bool VisitSegmentDatabase::InitSegmentTables() {
 
   // This was added later, so we need to try to create it even if the table
   // already exists.
-  GetDB().Execute("CREATE INDEX segments_url_id ON segments(url_id)");
+  if (!GetDB().Execute("CREATE INDEX IF NOT EXISTS segments_url_id ON "
+                       "segments(url_id)"))
+    return false;
 
   // Segment usage table.
   if (!GetDB().DoesTableExist("segment_usage")) {
@@ -81,8 +83,9 @@ bool VisitSegmentDatabase::InitSegmentTables() {
   }
 
   // Added in a later version, so we always need to try to creat this index.
-  GetDB().Execute("CREATE INDEX segments_usage_seg_id "
-                  "ON segment_usage(segment_id)");
+  if (!GetDB().Execute("CREATE INDEX IF NOT EXISTS segments_usage_seg_id "
+                       "ON segment_usage(segment_id)"))
+    return false;
 
   // Presentation index table.
   //
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index 1c1f5da..de1b849 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -294,11 +294,13 @@ bool InitTable(sql::Connection* db) {
   }
 
   // Try to create the index every time. Older versions did not have this index,
-  // so we want those people to get it. Ignore errors, since it may exist.
-  db->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
-              " (creation_utc)");
+  // so we want those people to get it.
+  if (!db->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
+                   " (creation_utc)"))
+    return false;
 
-  db->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies(host_key)");
+  if (!db->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies(host_key)"))
+    return false;
 
   return true;
 }
@@ -669,24 +671,24 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
     sql::Transaction transaction(db_.get());
     transaction.Begin();
 #if !defined(OS_WIN)
-    db_->Execute(
+    ignore_result(db_->Execute(
         "UPDATE cookies "
         "SET creation_utc = creation_utc + 11644473600000000 "
         "WHERE rowid IN "
         "(SELECT rowid FROM cookies WHERE "
-          "creation_utc > 0 AND creation_utc < 11644473600000000)");
-    db_->Execute(
+          "creation_utc > 0 AND creation_utc < 11644473600000000)"));
+    ignore_result(db_->Execute(
         "UPDATE cookies "
         "SET expires_utc = expires_utc + 11644473600000000 "
         "WHERE rowid IN "
         "(SELECT rowid FROM cookies WHERE "
-          "expires_utc > 0 AND expires_utc < 11644473600000000)");
-    db_->Execute(
+          "expires_utc > 0 AND expires_utc < 11644473600000000)"));
+    ignore_result(db_->Execute(
         "UPDATE cookies "
         "SET last_access_utc = last_access_utc + 11644473600000000 "
         "WHERE rowid IN "
         "(SELECT rowid FROM cookies WHERE "
-          "last_access_utc > 0 AND last_access_utc < 11644473600000000)");
+          "last_access_utc > 0 AND last_access_utc < 11644473600000000)"));
 #endif
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
diff --git a/sql/connection.h b/sql/connection.h
index 39b999b..9b81804b 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -11,6 +11,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/time.h"
 #include "sql/sql_export.h"
@@ -149,12 +150,12 @@ class SQL_EXPORT Connection {
 
   // Initializes the SQL connection for the given file, returning true if the
   // file could be opened. You can call this or OpenInMemory.
-  bool Open(const FilePath& path);
+  bool Open(const FilePath& path) WARN_UNUSED_RESULT;
 
   // Initializes the SQL connection for a temporary in-memory database. There
   // will be no associated file on disk, and the initial database will be
   // empty. You can call this or Open.
-  bool OpenInMemory();
+  bool OpenInMemory() WARN_UNUSED_RESULT;
 
   // Returns trie if the database has been successfully opened.
   bool is_open() const { return !!db_; }
@@ -204,11 +205,15 @@ class SQL_EXPORT Connection {
   // Executes the given SQL string, returning true on success. This is
   // normally used for simple, 1-off statements that don't take any bound
   // parameters and don't return any data (e.g. CREATE TABLE).
+  //
   // This will DCHECK if the |sql| contains errors.
-  bool Execute(const char* sql);
+  //
+  // Do not use ignore_result() to ignore all errors.  Use
+  // ExecuteAndReturnErrorCode() and ignore only specific errors.
+  bool Execute(const char* sql) WARN_UNUSED_RESULT;
 
   // Like Execute(), but returns the error code given by SQLite.
-  int ExecuteAndReturnErrorCode(const char* sql);
+  int ExecuteAndReturnErrorCode(const char* sql) WARN_UNUSED_RESULT;
 
   // Returns true if we have a statement with the given identifier already
   // cached. This is normally not necessary to call, but can be useful if the
@@ -360,7 +365,8 @@ class SQL_EXPORT Connection {
   int OnSqliteError(int err, Statement* stmt);
 
   // Like |Execute()|, but retries if the database is locked.
-  bool ExecuteWithTimeout(const char* sql, base::TimeDelta ms_timeout);
+  bool ExecuteWithTimeout(const char* sql, base::TimeDelta ms_timeout)
+      WARN_UNUSED_RESULT;
 
   // The actual sqlite database. Will be NULL before Init has been called or if
   // Init resulted in an error.
diff --git a/webkit/quota/quota_database_unittest.cc b/webkit/quota/quota_database_unittest.cc
index 7045f7f..a653e32 100644
--- a/webkit/quota/quota_database_unittest.cc
+++ b/webkit/quota/quota_database_unittest.cc
@@ -417,8 +417,7 @@ class QuotaDatabaseTest : public testing::Test {
 
   bool OpenDatabase(sql::Connection* db, const FilePath& kDbFile) {
     if (kDbFile.empty()) {
-      db->OpenInMemory();
-      return true;
+      return db->OpenInMemory();
     }
     if (!file_util::CreateDirectory(kDbFile.DirName()))
       return false;