Adding validity checks to sql statements in AutocompleteActionPredictorTable.

chrome/browser/predictors/autocomplete_action_predictor_table.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/predictors/autocomplete_action_predictor_table.h"
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
@@ skipping 82 matching lines @@
 void AutocompleteActionPredictorTable::GetAllRows(Rows* row_buffer) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
   if (CantAccessDatabase())
     return;
 
   row_buffer->clear();
 
   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))
     row_buffer->push_back(row);
 }
 
 void AutocompleteActionPredictorTable::AddRow(
     const AutocompleteActionPredictorTable::Row& row) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
   if (CantAccessDatabase())
@@ skipping 20 matching lines @@
 
   if (!DB()->BeginTransaction())
     return;
   for (Rows::const_iterator it = rows_to_add.begin();
        it != rows_to_add.end(); ++it) {
     sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
         base::StringPrintf(
             "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();
       return;
     }
   }
   for (Rows::const_iterator it = rows_to_update.begin();
        it != rows_to_update.end(); ++it) {
     sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
         base::StringPrintf(
             "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();
       return;
     }
     DCHECK_GT(DB()->GetLastChangeCount(), 0);
   }
   DB()->CommitTransaction();
 }
 
 void AutocompleteActionPredictorTable::DeleteRows(
     const std::vector<Row::Id>& id_list) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
   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();
 }
 
 void AutocompleteActionPredictorTable::DeleteAllRows() {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
   if (CantAccessDatabase())
     return;
 
   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();
 }
 
 AutocompleteActionPredictorTable::AutocompleteActionPredictorTable()
     : PredictorTableBase() {
 }
 
 AutocompleteActionPredictorTable::~AutocompleteActionPredictorTable() {
 }
@@ skipping 18 matching lines @@
 }
 
 void AutocompleteActionPredictorTable::LogDatabaseStats()  {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
   if (CantAccessDatabase())
     return;
 
   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));
 }
 
 }  // namespace predictors