Adding validity checks to sql statements in AutocompleteActionPredictorTable.

chrome/browser/predictors/autocomplete_action_predictor_table.cc

Index: chrome/browser/predictors/autocomplete_action_predictor_table.cc
diff --git a/chrome/browser/predictors/autocomplete_action_predictor_table.cc b/chrome/browser/predictors/autocomplete_action_predictor_table.cc
index 0e3a5d4b340bc29cc37d3cff83a39882f1383884..37b30f24f49b098750909398726e1307289e0a0a 100644
--- a/chrome/browser/predictors/autocomplete_action_predictor_table.cc
+++ b/chrome/browser/predictors/autocomplete_action_predictor_table.cc
@@ -100,6 +100,8 @@ void AutocompleteActionPredictorTable::GetAllRows(Rows* row_buffer) {
   sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
       base::StringPrintf(
           "SELECT * FROM %s", kAutocompletePredictorTableName).c_str()));
+  if (!statement.is_valid())

[dominich, 2012/06/12 19:11:06]

have you seen this happen?

[Shishir, 2012/06/12 19:35:22]

Not here, but the API says that you should check validity. In corner cases, it
can be invalid. Through out the code, at places it is checked and at places not.

[dominich, 2012/06/12 19:44:43]

Done.

+    return;
 
   Row row;
   while (StepAndInitializeRow(&statement, &row))
@@ -140,6 +142,11 @@ void AutocompleteActionPredictorTable::AddAndUpdateRows(
             "INSERT INTO %s "
             "(id, user_text, url, number_of_hits, number_of_misses) "
             "VALUES (?,?,?,?,?)", kAutocompletePredictorTableName).c_str()));
+    if (!statement.is_valid()) {
+      DB()->RollbackTransaction();
+      return;
+    }
+
     BindRowToStatement(*it, &statement);
     if (!statement.Run()) {
       DB()->RollbackTransaction();
@@ -153,6 +160,11 @@ void AutocompleteActionPredictorTable::AddAndUpdateRows(
             "UPDATE %s "
             "SET id=?, user_text=?, url=?, number_of_hits=?, number_of_misses=?"
             " WHERE id=?1", kAutocompletePredictorTableName).c_str()));
+    if (!statement.is_valid()) {
+      DB()->RollbackTransaction();
+      return;
+    }
+
     BindRowToStatement(*it, &statement);
     if (!statement.Run()) {
       DB()->RollbackTransaction();
@@ -169,20 +181,24 @@ void AutocompleteActionPredictorTable::DeleteRows(
   if (CantAccessDatabase())
     return;
 
-  sql::Statement statement(DB()->GetUniqueStatement(base::StringPrintf(
-      "DELETE FROM %s WHERE id=?",
-      kAutocompletePredictorTableName).c_str()));
-
   if (!DB()->BeginTransaction())
     return;
   for (std::vector<Row::Id>::const_iterator it = id_list.begin();
        it != id_list.end(); ++it) {
+    sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,

[dominich, 2012/06/12 19:11:06]

this doesn't change through the loop - which is why i had it outside. is there a
reason to bring the scope tighter?

[Shishir, 2012/06/12 19:35:22]

To be consistent with the rest of the code. We do it exactly this way everywhere
else. Also I moved to using a cached statement, so the overhead should be
minimum.

[dominich, 2012/06/12 19:44:43]

Done.

+        base::StringPrintf(
+            "DELETE FROM %s WHERE id=?",
+            kAutocompletePredictorTableName).c_str()));
+    if (!statement.is_valid()) {

[dominich, 2012/06/12 19:11:06]

how can the statement be valid before it's bound?

[Shishir, 2012/06/12 19:35:22]

This function does not check SQL validity (IsSQLValid does that). It checks that
the Statement has a non-NULL underlying sql_statement object.

[dominich, 2012/06/12 19:44:43]

Done.

+      DB()->RollbackTransaction();
+      return;
+    }
+
     statement.BindString(0, *it);
     if (!statement.Run()) {
       DB()->RollbackTransaction();
       return;
     }
-    statement.Reset(true);
   }
   DB()->CommitTransaction();
 }
@@ -195,6 +211,8 @@ void AutocompleteActionPredictorTable::DeleteAllRows() {
   sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
       base::StringPrintf("DELETE FROM %s",
                          kAutocompletePredictorTableName).c_str()));
+  if (!statement.is_valid())

[dominich, 2012/06/12 19:11:06]

have you seen this happen?

[Shishir, 2012/06/12 19:35:22]

Nope, but again the API suggests doing it

From the docs:

  // If the |sql| has an error, an invalid, inert StatementRef is returned (and
  // the code will crash in debug). The caller must deal with this eventuality,
  // either by checking validity of the |sql| before calling, by correctly
  // handling the return of an inert statement, or both.

[dominich, 2012/06/12 19:44:43]

Done.

+    return;
 
   statement.Run();
 }
@@ -233,7 +251,7 @@ void AutocompleteActionPredictorTable::LogDatabaseStats()  {
   sql::Statement count_statement(DB()->GetUniqueStatement(
       base::StringPrintf("SELECT count(id) FROM %s",
                          kAutocompletePredictorTableName).c_str()));
-  if (!count_statement.Step())
+  if (!count_statement.is_valid() || !count_statement.Step())
     return;
   UMA_HISTOGRAM_COUNTS("AutocompleteActionPredictor.DatabaseRowCount",
                        count_statement.ColumnInt(0));