Adding validity checks to sql statements in AutocompleteActionPredictorTable.

Adding validity checks to sql statements in AutocompleteActionPredictorTable.


BUG=131697
TEST=None.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141753

[Shishir, 2012-06-12 18:52:35]

Please take a look.

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

why?

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:103: if
(!statement.is_valid())
have you seen this happen?

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:188:
sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
this doesn't change through the loop - which is why i had it outside. is there a
reason to bring the scope tighter?

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:192: if
(!statement.is_valid()) {
how can the statement be valid before it's bound?

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:214: if
(!statement.is_valid())
have you seen this happen?

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

Please see bug 131697. I am adding the checks to narrow the issue.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:103: if
(!statement.is_valid())
On 2012/06/12 19:11:06, dominich wrote:
> have you seen this happen?

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.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:188:
sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
On 2012/06/12 19:11:06, dominich wrote:
> this doesn't change through the loop - which is why i had it outside. is there
a
> reason to bring the scope tighter?

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.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:192: if
(!statement.is_valid()) {
On 2012/06/12 19:11:06, dominich wrote:
> how can the statement be valid before it's bound?

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

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:214: if
(!statement.is_valid())
On 2012/06/12 19:11:06, dominich wrote:
> have you seen this happen?

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]

LGTM

I'm suspicious that there's an underlying issue with Profile destruction and
sqlite transactions, but this should help.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:103: if
(!statement.is_valid())
On 2012/06/12 19:35:22, Shishir wrote:
> On 2012/06/12 19:11:06, dominich wrote:
> > have you seen this happen?
> 
> 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.

Done.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:188:
sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
On 2012/06/12 19:35:22, Shishir wrote:
> On 2012/06/12 19:11:06, dominich wrote:
> > this doesn't change through the loop - which is why i had it outside. is
there
> a
> > reason to bring the scope tighter?
> 
> 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.

Done.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:192: if
(!statement.is_valid()) {
On 2012/06/12 19:35:22, Shishir wrote:
> On 2012/06/12 19:11:06, dominich wrote:
> > how can the statement be valid before it's bound?
> 
> This function does not check SQL validity (IsSQLValid does that). It checks
that
> the Statement has a non-NULL underlying sql_statement object.

Done.

https://chromiumcodereview.appspot.com/10546129/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:214: if
(!statement.is_valid())
On 2012/06/12 19:35:22, Shishir wrote:
> On 2012/06/12 19:11:06, dominich wrote:
> > have you seen this happen?
> 
> 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.

Done.

[commit-bot: I haz the power, 2012-06-12 20:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10546129/1

[commit-bot: I haz the power, 2012-06-12 21:54:30]

Change committed as 141753

[Scott Hess - ex-Googler, 2012-06-12 22:30:14]

I think this CL is mis-understanding the API.  Looping in gbillock for more
extensive explanations if needed (I'm not supposed to be looking at this stuff
right now).

The gist of things is that an invalid statement should short-circuit everything,
so you should only need to check is_valid() if you're going to have side
effects.  You'll hit DCHECK in debug builds, because, broadly put, the only ways
statements are invalid is if the developer is doing things wrong, or if the
database is corrupt.  It is assumed that developers never have corrupt
databases.