Nuke from orbit corrupt websql databases.

Nuke from orbit corrupt websql databases.

BUG=98939
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=122651

[michaeln, 2012-02-10 00:30:12]

ptal

[Scott Hess - ex-Googler, 2012-02-10 23:34:07]

http://codereview.chromium.org/9371008/diff/2004/content/common/web_database_...
File content/common/web_database_observer_impl.cc (right):

http://codereview.chromium.org/9371008/diff/2004/content/common/web_database_...
content/common/web_database_observer_impl.cc:155: const WebDatabase& database,
int error) {
Do you have a histogram out there to tell the relative occurrence of errors?  If
not, this would be a great place to land one...

http://codereview.chromium.org/9371008/diff/2004/webkit/database/database_tra...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/9371008/diff/2004/webkit/database/database_tra...
webkit/database/database_tracker.cc:192: // database will fail.
I guess that DeleteDatabase() implements all that message-and-delete-when-done
magic?

http://codereview.chromium.org/9371008/diff/2004/webkit/database/database_tra...
webkit/database/database_tracker.cc:193: if (error == SQLITE_CORRUPT || error ==
SQLITE_NOTADB) {
The sender also has this guard.  It is definitely needed here, but maybe it
would make sense to not have the check in the sender (forward all errors, and
let the policy decision be made in one place).

http://codereview.chromium.org/9371008/diff/2004/webkit/database/database_tra...
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/9371008/diff/2004/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:904:
tracker->RemoveObserver(&observer);
Any possibility of an end-to-end test, where you create an actual database and
allow a query against it to cause everything to work?

SQLITE_NOTADB is easy to cause by making a change to the header (along the lines
of "SQLite format 3" -> "SQLite poison 3"!).  Making a SQLITE_CORRUPT is more
challenging, but I can probably think up a straight-forward option if you can
make the framework to test it.

[michaeln, 2012-02-11 00:34:20]

https://chromiumcodereview.appspot.com/9371008/diff/2004/content/common/web_d...
File content/common/web_database_observer_impl.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/content/common/web_d...
content/common/web_database_observer_impl.cc:155: const WebDatabase& database,
int error) {
On 2012/02/10 23:34:07, shess wrote:
> Do you have a histogram out there to tell the relative occurrence of errors? 
If
> not, this would be a great place to land one...

Yes, see histograms scattered in this file.

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
File webkit/database/database_tracker.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
webkit/database/database_tracker.cc:192: // database will fail.
On 2012/02/10 23:34:07, shess wrote:
> I guess that DeleteDatabase() implements all that message-and-delete-when-done
> magic?

Yes

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
webkit/database/database_tracker.cc:193: if (error == SQLITE_CORRUPT || error ==
SQLITE_NOTADB) {
On 2012/02/10 23:34:07, shess wrote:
> The sender also has this guard.  It is definitely needed here, but maybe it
> would make sense to not have the check in the sender (forward all errors, and
> let the policy decision be made in one place).

That could be, but i'm avoiding necessary messaging with the test in the
renderer. This stuff is called per statement in the renderer.

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
File webkit/database/database_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
webkit/database/database_tracker_unittest.cc:904:
tracker->RemoveObserver(&observer);
On 2012/02/10 23:34:07, shess wrote:
> Any possibility of an end-to-end test, where you create an actual database and
> allow a query against it to cause everything to work?

An end-to-end test is probably doable as a ui test or a browser test, but i
haven't put that together such a framework.  I'm doing local testing for now.

[Scott Hess - ex-Googler, 2012-02-11 19:48:23]

LGTM, but as noted, I'm not completely convinced about using a generic-sounding
callback which only receives a fraction of the errors.

https://chromiumcodereview.appspot.com/9371008/diff/2004/content/common/web_d...
File content/common/web_database_observer_impl.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/content/common/web_d...
content/common/web_database_observer_impl.cc:155: const WebDatabase& database,
int error) {
On 2012/02/11 00:34:20, michaeln wrote:
> On 2012/02/10 23:34:07, shess wrote:
> > Do you have a histogram out there to tell the relative occurrence of errors?

> If
> > not, this would be a great place to land one...
> 
> Yes, see histograms scattered in this file.

<decoding> surprisingly few SQLITE_CORRUPT cases!  I wonder how SQLITE_CANTOPEN
is coming into play, here...

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
File webkit/database/database_tracker.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
webkit/database/database_tracker.cc:193: if (error == SQLITE_CORRUPT || error ==
SQLITE_NOTADB) {
On 2012/02/11 00:34:20, michaeln wrote:
> On 2012/02/10 23:34:07, shess wrote:
> > The sender also has this guard.  It is definitely needed here, but maybe it
> > would make sense to not have the check in the sender (forward all errors,
and
> > let the policy decision be made in one place).
> 
> That could be, but i'm avoiding necessary messaging with the test in the
> renderer. This stuff is called per statement in the renderer.

OK.  In that case, perhaps document that this is security filtering but
otherwise redundant, so that someone doesn't accidentally expect to see other
errors, here.  I'm mixed as to whether that would be better as else
{NOTREACHED();} though.

Another option might be to forward only for error != SQLITE_OK.

Can the client schedule deletes already?  It might make sense to just have the
client do it, then.

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
File webkit/database/database_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9371008/diff/2004/webkit/database/data...
webkit/database/database_tracker_unittest.cc:904:
tracker->RemoveObserver(&observer);
On 2012/02/11 00:34:20, michaeln wrote:
> On 2012/02/10 23:34:07, shess wrote:
> > Any possibility of an end-to-end test, where you create an actual database
and
> > allow a query against it to cause everything to work?
> 
> An end-to-end test is probably doable as a ui test or a browser test, but i
> haven't put that together such a framework.  I'm doing local testing for now.

Makes sense.