Delete the cookie DB when an unrecoverable error is detected.

Delete the cookie DB when an unrecoverable error is detected.

The in-progress session will proceed without persisting cookies. The database will be re-established on disk at the beginning of the next session.

R=shess@chromium.org
BUG=141591
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157011

[erikwright (departed), 2012-09-13 17:40:04]

shess, PTAL.

[Scott Hess - ex-Googler, 2012-09-14 17:01:34]

Since you want to get this landed, I'll post my first pass from last night
before embarking on a deeper review.  Really only the Raze/Delete() comment is
actionable.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:307: case SQLITE_ERROR:
This an SQLITE_CONSTRAINT are _probably_ cases where we made a mistake.  Like we
did a schema upgrade incorrectly, or have incorrect SQLite code.  So killing the
database is likely to mask real problems in our code.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:313: break;
Dammit, I wish I remember where I was seeing this, once.  I see a very old note
WRT Gears where SQLITE_CANTOPEN was associated with anti-virus checkers, but I
think there was something more recent.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:318: break;
It is possible to write the code such that SQLITE_BUSY is impossible.

SQLITE_IOERR is tricky.  If the file contains broken blocks, deleting it may
actually not be a bad idea.  But I'm not sure how often these kinds of things
are transient, like a write error.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:329: break;
We might be able to delete a read-only file.  Depending.  But you're right, this
is a wrongness which shouldn't be expected.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:1107: bool success =
file_util::Delete(path_, false);
Suggest doing db_.Raze() before closing it.  That will clear the database down
to empty.

IF you still want the filesystem-level Delete, then you need to make sure you
delete the -journal file, and you might also want to delete a -wal file (which
isn't currently present, but which might be someday).  I cannot offhand make any
recommendations on how to order the deletes - I recall having an argument about
this on the SQLite mailing lists years ago, and Dr Hipp asserted that you cannot
really do a safe delete, which was a bit mind-boggling.

Actually, it would be much better to refactor to put the Delete() as a static
function in sql::Connection, so that anyone doing refactors around this can see
what's going on.

[Scott Hess - ex-Googler, 2012-09-14 17:15:16]

OK, tiny bit more.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:126: // Backend will call
reset_backend() when it is going away.
Rather than doing this dance, would it make sense to have Backend simply
implement sql::ErrorDelegate directly?  Then you don't need to break the
backlink manually.

Alternately, I think if you reset the error delegate on the connection before
any call to db_.reset(), you'd never see inappropriate access.  Unless there are
thread-safety issues I'm missing.

[erikwright (departed), 2012-09-14 18:11:02]

PTAL. I manually tested the Raze and it worked fine in my case, at least.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:126: // Backend will call
reset_backend() when it is going away.
On 2012/09/14 17:15:16, shess wrote:
> Rather than doing this dance, would it make sense to have Backend simply
> implement sql::ErrorDelegate directly?  Then you don't need to break the
> backlink manually.

Then you would have the circular reference of Backend -> Connection -> Backend,
and no way of breaking it.

> Alternately, I think if you reset the error delegate on the connection before
> any call to db_.reset(), you'd never see inappropriate access.  Unless there
are
> thread-safety issues I'm missing.

I agree that there shouldn't be any inappropriate accesses if sql::Connection is
implemented the way one would expect.

Unforunately, we're passing around a reference counted object and there's no
explicit guarantee that it will be decremented instantly upon a
set_error_delegate(NULL).

reset_backend() exists purely for the sake of correctness, not to prevent any
actual crash.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:318: break;
On 2012/09/14 17:01:34, shess wrote:
> It is possible to write the code such that SQLITE_BUSY is impossible.
> 
> SQLITE_IOERR is tricky.  If the file contains broken blocks, deleting it may
> actually not be a bad idea.  But I'm not sure how often these kinds of things
> are transient, like a write error.

SQLITE_IOERR is a relatively commonly occurring error. I'll try to dig into the
data to see if it tends to be transient or permanent.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:329: break;
On 2012/09/14 17:01:34, shess wrote:
> We might be able to delete a read-only file.  Depending.  But you're right,
this
> is a wrongness which shouldn't be expected.

It is a commonly occuring error. Again, I'll dig into the data.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:1107: bool success =
file_util::Delete(path_, false);
On 2012/09/14 17:01:34, shess wrote:
> Suggest doing db_.Raze() before closing it.  That will clear the database down
> to empty.
> 
> IF you still want the filesystem-level Delete, then you need to make sure you
> delete the -journal file, and you might also want to delete a -wal file (which
> isn't currently present, but which might be someday).  I cannot offhand make
any
> recommendations on how to order the deletes - I recall having an argument
about
> this on the SQLite mailing lists years ago, and Dr Hipp asserted that you
cannot
> really do a safe delete, which was a bit mind-boggling.
> 
> Actually, it would be much better to refactor to put the Delete() as a static
> function in sql::Connection, so that anyone doing refactors around this can
see
> what's going on.

We can do the Raze for now and see what happens. We should see occurrences of
these errors drop dramatically, and we should presumably not see repeated
Cookie.KillDatabaseResult metrics from the same users.

[Scott Hess - ex-Googler, 2012-09-14 18:34:03]

I'm about 70% on wanting a better way to break the back-link.  I'd hate to end
up causing a bunch of edge-case crashes for users who don't have corrupted
databases.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:126: // Backend will call
reset_backend() when it is going away.
On 2012/09/14 18:11:02, erikwright wrote:
> On 2012/09/14 17:15:16, shess wrote:
> > Rather than doing this dance, would it make sense to have Backend simply
> > implement sql::ErrorDelegate directly?  Then you don't need to break the
> > backlink manually.
> 
> Then you would have the circular reference of Backend -> Connection ->
Backend,
> and no way of breaking it.

Doh!

> > Alternately, I think if you reset the error delegate on the connection
before
> > any call to db_.reset(), you'd never see inappropriate access.  Unless there
> are
> > thread-safety issues I'm missing.
> 
> I agree that there shouldn't be any inappropriate accesses if sql::Connection
is
> implemented the way one would expect.
> 
> Unforunately, we're passing around a reference counted object and there's no
> explicit guarantee that it will be decremented instantly upon a
> set_error_delegate(NULL).
> 
> reset_backend() exists purely for the sake of correctness, not to prevent any
> actual crash.

Hmm.  OK.  I honestly cannot see any reason for the error delegate to have been
ref-counted in the first place.

Part of why I'm nervous about manually breaking the link is because there are a
lot of places where db_ is reset().  Would it be reasonable to use a weak
pointer factory to generalize the solution, rather than adding manual
infrastructure?

http://codereview.chromium.org/10918220/diff/7001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/7001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:1110: }
Would it be worthwhile to document the no-db_ case?

Think about whether there will be additional related events to track, also.  I
usually just go directly to an enum histogram, because after the first version
rolls out and solves the 90% case, I find that I need additional data to figure
out what's up with the 10% case.

[erikwright (departed), 2012-09-14 18:43:26]

PTAL. I think that the reference breaking is provably safe and simpler than a
weak ref in this case.

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10918220/diff/1001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:126: // Backend will call
reset_backend() when it is going away.
On 2012/09/14 18:34:03, shess wrote:
> On 2012/09/14 18:11:02, erikwright wrote:
> > On 2012/09/14 17:15:16, shess wrote:
> > > Rather than doing this dance, would it make sense to have Backend simply
> > > implement sql::ErrorDelegate directly?  Then you don't need to break the
> > > backlink manually.
> > 
> > Then you would have the circular reference of Backend -> Connection ->
> Backend,
> > and no way of breaking it.
> 
> Doh!
> 
> > > Alternately, I think if you reset the error delegate on the connection
> before
> > > any call to db_.reset(), you'd never see inappropriate access.  Unless
there
> > are
> > > thread-safety issues I'm missing.
> > 
> > I agree that there shouldn't be any inappropriate accesses if
sql::Connection
> is
> > implemented the way one would expect.
> > 
> > Unforunately, we're passing around a reference counted object and there's no
> > explicit guarantee that it will be decremented instantly upon a
> > set_error_delegate(NULL).
> > 
> > reset_backend() exists purely for the sake of correctness, not to prevent
any
> > actual crash.
> 
> Hmm.  OK.  I honestly cannot see any reason for the error delegate to have
been
> ref-counted in the first place.
> 
> Part of why I'm nervous about manually breaking the link is because there are
a
> lot of places where db_ is reset().  Would it be reasonable to use a weak
> pointer factory to generalize the solution, rather than adding manual
> infrastructure?

We hold a pointer to the KillDatabaseErrorDelegate as a member of Backend. In
InternalBackgroundClose we break the back-ref before releasing our ref to the
delegate. In ~Backend() we DCHECK that Close has been called, though in the case
of db_ being reset you _could_ have a false positive there.

I have moved the reset_backend() to ~Backend, so it is now guaranteed that the
pointer to Backend is gone before Backend is destroyed.

[Scott Hess - ex-Googler, 2012-09-14 19:21:25]

LGTM.

[commit-bot: I haz the power, 2012-09-14 19:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10918220/10001

[commit-bot: I haz the power, 2012-09-14 20:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10918220/14002

[commit-bot: I haz the power, 2012-09-14 22:37:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-14 22:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10918220/14002

[commit-bot: I haz the power, 2012-09-15 17:39:10]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2012-09-15 17:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10918220/14002

[commit-bot: I haz the power, 2012-09-15 20:15:43]

Change committed as 157011