Update webdata files to take advantage of DLOG(FATAL) in

Update webdata files to take advantage of DLOG(FATAL) in
sql/Statement and Connection.

R=shess@chromium.org
BUG=
TEST=webdata/*Test*.*


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

[Scott Hess - ex-Googler, 2011-12-15 23:02:57]

LGTM.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (left):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1029: NOTREACHED() << "Expected item in
trash.";
This looks like they might have originally meant
DCHECK_GT(db_->GetLastChangeCount(), 0);

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1342: return sql::INIT_FAILURE;
I wish this hadn't compiled in the first place ...

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:197: while (s.Step()) {
Step() could return false either because it has reached the end of the results,
or because there was an error fetching the results.  In the error case, numbers
could be empty (if there was corruption accessing an index), or partially-filled
(if there was a corruption reading from the table).  Should these cases be
paying attention to Succeeded() before calling mutating functions?

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1571: return false;
Could be removed?

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/logins_t...
File chrome/browser/webdata/logins_table_win.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/logins_t...
chrome/browser/webdata/logins_table_win.cc:41: s.BindString(0,
WideToUTF8(info.url_hash));
I'd rather this were just BindString16(), as long as you're touching it.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
File chrome/browser/webdata/token_service_table.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
chrome/browser/webdata/token_service_table.cc:46: return false;
Why not remove?

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc
File sql/statement.cc (right):

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc#newcode260
sql/statement.cc:260: // Should there be a DLOG(FATAL) on it?
I maybe don't understand the comment.  I'd be fine with simply stating "Binding
to a position which doesn't exist is almost certainly wrong."

Unless the TODO is to decide whether to make it invalidate the statement?

[Greg Billock, 2011-12-16 17:26:58]

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (left):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1029: NOTREACHED() << "Expected item in
trash.";
On 2011/12/15 23:02:57, shess wrote:
> This looks like they might have originally meant
> DCHECK_GT(db_->GetLastChangeCount(), 0);

Added. I think you're right.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:197: while (s.Step()) {
On 2011/12/15 23:02:57, shess wrote:
> Step() could return false either because it has reached the end of the
results,
> or because there was an error fetching the results.  In the error case,
numbers
> could be empty (if there was corruption accessing an index), or
partially-filled
> (if there was a corruption reading from the table).  Should these cases be
> paying attention to Succeeded() before calling mutating functions?

If it returns false we'll exit the loop, right? But if there's an error, it
should return false. So return s.Succeeded() is the correct behavior here.
Setting the value in that case is a bit cheeky. I'll edit around that...

OK, I've changed quite a few locations. Mostly to move is_valid() checks down
below and do Succeeded() checks. Those are more robust and WLOG. There is a
subtle behavior change, in that some of these will then zero out outparams
whereas before they'd leave them untouched. I'm inclined to believe this will be
OK, but do you think we need to examine that more carefully?

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1571: return false;
On 2011/12/15 23:02:57, shess wrote:
> Could be removed?

Done.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/logins_t...
File chrome/browser/webdata/logins_table_win.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/logins_t...
chrome/browser/webdata/logins_table_win.cc:41: s.BindString(0,
WideToUTF8(info.url_hash));
On 2011/12/15 23:02:57, shess wrote:
> I'd rather this were just BindString16(), as long as you're touching it.

Done.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
File chrome/browser/webdata/token_service_table.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
chrome/browser/webdata/token_service_table.cc:46: return false;
On 2011/12/15 23:02:57, shess wrote:
> Why not remove?

I was being conservative about executing EncryptString, but I that's probably
overthinking it.

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc
File sql/statement.cc (right):

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc#newcode260
sql/statement.cc:260: // Should there be a DLOG(FATAL) on it?
On 2011/12/15 23:02:57, shess wrote:
> I maybe don't understand the comment.  I'd be fine with simply stating
"Binding
> to a position which doesn't exist is almost certainly wrong."
> 
> Unless the TODO is to decide whether to make it invalidate the statement?

Yeah. This is basically a placeholder to move the conversation from the other
review to here.

The more I think about it, the more I'm convinced this should invalidate the
statement. An out-of-range error means, basically, that someone edited the
statement without editing the bind values, which is almost certainly disastrous.
We should think about better checks for this which can be applied cheaply. Does
sqlite already check to make sure all params are bound? Another idea would be to
bind typed params into the query with printf params. That would enlist the
compiler, and also provide a bit of type-safety in the doublecheck sense

[Scott Hess - ex-Googler, 2011-12-16 22:37:08]

lgtm.

Your option on the images vector change mentioned.  It's probably pedantic.

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
File chrome/browser/webdata/token_service_table.cc (right):

http://codereview.chromium.org/8966003/diff/1/chrome/browser/webdata/token_se...
chrome/browser/webdata/token_service_table.cc:46: return false;
On 2011/12/16 17:26:58, Greg Billock wrote:
> On 2011/12/15 23:02:57, shess wrote:
> > Why not remove?
> 
> I was being conservative about executing EncryptString, but I that's probably
> overthinking it.

In this case any savings would only happen when things are horribly awry anyhow.

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc
File sql/statement.cc (right):

http://codereview.chromium.org/8966003/diff/1/sql/statement.cc#newcode260
sql/statement.cc:260: // Should there be a DLOG(FATAL) on it?
On 2011/12/16 17:26:58, Greg Billock wrote:
> The more I think about it, the more I'm convinced this should invalidate the
> statement. An out-of-range error means, basically, that someone edited the
> statement without editing the bind values, which is almost certainly
disastrous.

Agreed.

> We should think about better checks for this which can be applied cheaply.
Does
> sqlite already check to make sure all params are bound?

Nope, they are implicitly bound to NULL, I think.

> Another idea would be to
> bind typed params into the query with printf params. That would enlist the
> compiler, and also provide a bit of type-safety in the doublecheck sense

I'll work up my CL mentioned earlier.  I think our current API is pretty
reasonable WRT simplicity and type safety, and I'd rather not get too funky in
addressing making bindings reliable.  I think simply having the statement track
that parameters are bound in-order and completely would be sufficient.  The few
cases where skipping one can easily be addressed using a null.

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/autofil...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/autofil...
chrome/browser/webdata/autofill_table.cc:157: return false;
Yeah, I think this is a reasonable pattern for the problem.  Fortunately it was
creating locally-scoped things rather than forwarding to the profile live...

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/autofil...
chrome/browser/webdata/autofill_table.cc:1062: return false;
I think that, given that the caller already is committed to accepting the
clear() in case of a successful (but empty) result, having it cleared for a
failure seems reasonable.  That's the kind of subtle thing that the header
should document and unit tests should test, if it's expected.

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/web_app...
File chrome/browser/webdata/web_apps_table.cc (right):

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/web_app...
chrome/browser/webdata/web_apps_table.cc:88: return s.Succeeded();
I think this is right, even though there could be cases where *images is changed
on failure.  WDYT about a local vector<> v, and images->swap(v) in case of
success?

[Greg Billock, 2011-12-21 21:43:03]

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/autofil...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/autofil...
chrome/browser/webdata/autofill_table.cc:1062: return false;
On 2011/12/16 22:37:09, shess wrote:
> I think that, given that the caller already is committed to accepting the
> clear() in case of a successful (but empty) result, having it cleared for a
> failure seems reasonable.  That's the kind of subtle thing that the header
> should document and unit tests should test, if it's expected.

Added documentation for this. Checked caller; it is checking return value and
behaving OK in this case.

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/web_app...
File chrome/browser/webdata/web_apps_table.cc (right):

http://codereview.chromium.org/8966003/diff/15/chrome/browser/webdata/web_app...
chrome/browser/webdata/web_apps_table.cc:88: return s.Succeeded();
On 2011/12/16 22:37:09, shess wrote:
> I think this is right, even though there could be cases where *images is
changed
> on failure.  WDYT about a local vector<> v, and images->swap(v) in case of
> success?

I think in this particular case partially filling it is fine. The code doesn't
empty it, which isn't really documented as part of the interface, but... The
caller doesn't check the return value, anyway. :-( I'll update the header.

[commit-bot: I haz the power, 2011-12-21 21:44:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/8966003/16001

[commit-bot: I haz the power, 2011-12-21 22:51:44]

Try job failure for 8966003-16001 (retry) on linux_rel for step
"test_shell_tests".
It's a second try, previously, step "test_shell_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-21 22:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/8966003/16001

[commit-bot: I haz the power, 2011-12-22 01:11:54]

Try job failure for 8966003-16001 (retry) (previous was lost) (previous was
lost) on linux_rel for step "test_shell_tests".
It's a second try, previously, step "test_shell_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-22 15:20:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/8966003/16001

[commit-bot: I haz the power, 2011-12-22 19:25:53]

Change committed as 115574