AppCache: Run a quick integrity check on the sqlite database when opening.

AppCache: Run a quick integrity check on the sqlite database when opening. If the test fails, delete the appcache directory and start afresh.

NOTRY=true
TBR=sky
BUG=318544
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240937

[michaeln, 2013-12-05 23:21:54]

hi scott, ptal

[michaeln, 2013-12-06 00:14:08]

i'm working on a unittest test for this

[Scott Hess - ex-Googler, 2013-12-06 00:34:12]

On 2013/12/06 00:14:08, michaeln wrote:
> i'm working on a unittest test for this

sql/test/test_helpers.h has a couple helpers for making things corrupt.

[Scott Hess - ex-Googler, 2013-12-06 00:35:00]

On 2013/12/06 00:34:12, shess wrote:
> On 2013/12/06 00:14:08, michaeln wrote:
> > i'm working on a unittest test for this
> 
> sql/test/test_helpers.h has a couple helpers for making things corrupt.

Actually - look at thumbnail_database_unittest.cc for some examples of
unit-testing corruption.  There are probably some in sql/recovery_unittest.cc,
too.

[Scott Hess - ex-Googler, 2013-12-06 00:44:00]

I don't know enough about how AppCache works to answer this myself, so I'll ask
you.  Would it make sense to have a variant of sql::Connection::Open() which ran
one of the checks, and then did a Raze() in case of failure?  If the Raze()
works, the Open() would succeed with an empty database.

https://codereview.chromium.org/104593010/diff/1/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/104593010/diff/1/sql/connection.h#newcode163
sql/connection.h:163: bool IntegrityCheck(std::vector<std::string>* messages,
bool quick);
Move IntegrityCheck() to private, maybe call it "IntegrityCheckHelper(), and put
the in parameter before the out parameter.

I'm a little nervous about having these inline in the header.  There's no
performance reason for it (running the integrity check is like a billion times
more expensive than function-call overhead).  It maybe would make sense to move
them into the .cc and just have them pass in the pragma rather than passing a
bool which modifies the pragma.

It would be nice to have a unit test for both cases doing what they say.  I
_think_ that a quick check would probably succeed on the corruption helper which
messes up the index/table relationship, while a full check would not.

[michaeln, 2013-12-06 02:45:40]

Added a test. (A file containing "deadbeef" actually opens :)

> I don't know enough about how AppCache works to answer this myself, so I'll
ask
> you.  Would it make sense to have a variant of sql::Connection::Open() which
ran
> one of the checks, and then did a Raze() in case of failure?  If the Raze()
> works, the Open() would succeed with an empty database.

There is other data outside of the sqlite db file, so the razing cannot be
transparent.

> It would be nice to have a unit test for both cases doing what they say.  I
> _think_ that a quick check would probably succeed on the corruption helper
which
> messes up the index/table relationship, while a full check would not.

I'll take a look at refactoring the tests a little tomorrow. Agree that having
some unittests for the integrity checking method of Connection would be a good
thing.

> https://codereview.chromium.org/104593010/diff/1/sql/connection.h#newcode163
> sql/connection.h:163: bool IntegrityCheck(std::vector<std::string>* messages,
> bool quick);
> Move IntegrityCheck() to private, maybe call it "IntegrityCheckHelper(), and
put
> the in parameter before the out parameter.
> 
> I'm a little nervous about having these inline in the header.  There's no
> performance reason for it (running the integrity check is like a billion times
> more expensive than function-call overhead).  It maybe would make sense to
move
> them into the .cc and just have them pass in the pragma rather than passing a
> bool which modifies the pragma.

I'll poke at the packaging tomorrow too.

[Scott Hess - ex-Googler, 2013-12-06 19:59:58]

On 2013/12/06 02:45:40, michaeln wrote:
> Added a test. (A file containing "deadbeef" actually opens :)

sqlite3_open() does _very_ little.  I've been doing corruption-handling, and
have been layering in test helpers in sql/test/ to make it easier to accomplish.
 As mentioned out-of-band, the latest one isn't quite landed, but today :-).

> > I don't know enough about how AppCache works to answer this myself, so I'll
> ask
> > you.  Would it make sense to have a variant of sql::Connection::Open() which
> ran
> > one of the checks, and then did a Raze() in case of failure?  If the Raze()
> > works, the Open() would succeed with an empty database.
> 
> There is other data outside of the sqlite db file, so the razing cannot be
> transparent.

OK, this is good to know.  AppCache is one of the databases with lots of
SQLITE_CORRUPT in it, so I had been considering whether to make it one of the
next targets for recovery code.  The recovery code makes a best-effort attempt
to recover the data from the database, and in many cases it will get everything
(say if an index and table mismatch).  But it is possible that something will be
missed, and it sounds like AppCache would prefer to just go directly to "blow
everything away" if any problems are detected.

That probably implies that an error handler should be instantiated which blows
everything away in that case.

[michaeln, 2013-12-09 19:12:55]

> I'll poke at the packaging tomorrow too.

Done

> That probably implies that an error handler should be instantiated which blows
> everything away in that case.

Yes, or arranges for that to happen "soon". I'm not shooting for this behavior
in this CL. In this one, i just want to get the low-hanging low-risk fruit.

> some unittests for the integrity checking method of Connection

I have not added those yet, but the AppCacheDatabase test does provide some
coverage.

[michaeln, 2013-12-09 19:15:50]

https://codereview.chromium.org/104593010/diff/30001/chrome/browser/history/t...
File chrome/browser/history/thumbnail_database.cc (right):

https://codereview.chromium.org/104593010/diff/30001/chrome/browser/history/t...
chrome/browser/history/thumbnail_database.cc:143:
db->FullIntegrityCheck(&messages);
ooops, have to check the return value here

[Scott Hess - ex-Googler, 2013-12-09 20:27:27]

https://codereview.chromium.org/104593010/diff/30001/chrome/browser/history/t...
File chrome/browser/history/thumbnail_database.cc (right):

https://codereview.chromium.org/104593010/diff/30001/chrome/browser/history/t...
chrome/browser/history/thumbnail_database.cc:143:
db->FullIntegrityCheck(&messages);
On 2013/12/09 19:15:51, michaeln wrote:
> ooops, have to check the return value here

FYI, IntegrityCheck() was only added for this code to build fake-crash-dump
diagnostics.  So the API may not be anything like what a real client might want.

IMHO, nobody but diagnostics tools are going to want the actual messages.  I
think it would be perfectly fine to shift your new changes towards an API with a
bool return and no messages, and WARN_UNUSED_RESULT as appropriate.

[michaeln, 2013-12-09 21:58:53]

> IMHO, nobody but diagnostics tools are going to want the actual messages.  I
> think it would be perfectly fine to shift your new changes towards an API with
a
> bool return and no messages, and WARN_UNUSED_RESULT as appropriate.

sgtm, then i could get rid of the helper in class appcachedb

[michaeln, 2013-12-13 19:48:16]

On 2013/12/09 21:58:53, michaeln wrote:
> > IMHO, nobody but diagnostics tools are going to want the actual messages.  I
> > think it would be perfectly fine to shift your new changes towards an API
with
> a
> > bool return and no messages, and WARN_UNUSED_RESULT as appropriate.

Done.

FullIntegrityCheck has returns the statement execution result and the messages
(just like the original IntegrityCheck method), the new QuickIntegrityCheck
method just returns the simple true/false.

[Scott Hess - ex-Googler, 2013-12-13 20:03:45]

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:81: // to the consumer so 
we can test the error handling of the consumer.
Does sql::ScopedErrorIgnorer not handle this?  There are edge cases out there,
but mostly it's not a problem.

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:104:
EXPECT_TRUE(base::PathExists(kDbFile));
If I understand things correctly, the AppCacheDatabase must be deleting the
entire directory that kDbFile is in?  If so, prefer to create a subdir under
temp_dir, rather than delete the dir and re-create.  I don't think anything will
go wrong as-is, but messing with the temp_dir itself potentially breaks
assumptions.

[michaeln, 2013-12-13 22:46:04]

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:81: // to the consumer so 
we can test the error handling of the consumer.
On 2013/12/13 20:03:45, shess wrote:
> Does sql::ScopedErrorIgnorer not handle this?

It does not, other DLOG(FATAL)'s get hit given these inputs, one example is in
Connnection::GetUniqueStatement() but there are others too.
  int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
  if (rc != SQLITE_OK) {
    // This is evidence of a syntax error in the incoming SQL.
    DLOG(FATAL) << "SQL compile error " << GetErrorMessage();

I could modify this to use CorruptSizeInHeader instead to make the inputs?

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:104:
EXPECT_TRUE(base::PathExists(kDbFile));
On 2013/12/13 20:03:45, shess wrote:
> If I understand things correctly, the AppCacheDatabase must be deleting the
> entire directory that kDbFile is in?  If so, prefer to create a subdir under
> temp_dir, rather than delete the dir and re-create.  I don't think anything
will
> go wrong as-is, but messing with the temp_dir itself potentially breaks
> assumptions.

Done.

[michaeln, 2013-12-13 22:47:04]

https://codereview.chromium.org/104593010/diff/90001/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/90001/sql/connection_unittest....
sql/connection_unittest.cc:825: TEST_F(SQLConnectionTest,
Basic_QuickIntegrityCheck) {
Also, added some basic tests for the integrity checkers.

[Scott Hess - ex-Googler, 2013-12-13 23:15:45]

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:81: // to the consumer so 
we can test the error handling of the consumer.
On 2013/12/13 22:46:05, michaeln wrote:
> On 2013/12/13 20:03:45, shess wrote:
> > Does sql::ScopedErrorIgnorer not handle this?
> 
> It does not, other DLOG(FATAL)'s get hit given these inputs, one example is in
> Connnection::GetUniqueStatement() but there are others too.
>   int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
>   if (rc != SQLITE_OK) {
>     // This is evidence of a syntax error in the incoming SQL.
>     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
> 
> I could modify this to use CorruptSizeInHeader instead to make the inputs?

I think CorruptSizeInHeader() will let it get through things, though let me know
if it doesn't.

I think I've seen that same problem with using garbage data.  That's why a
couple tests in connection_unittest.cc use IsSQLValid() instead of executing it.
 I'm not sure how feasible that it in this case, though.  Long-term, I should
maybe augment the error ignorer.

My main concern is doing things like NDEBUG.  It makes it harder to track down
failures, because you'll get "Failing BlahTest.SpecificSomething" and your first
build won't show any failures, eventually you'll realize it's not even running
the test, swear a bit, etc.

https://codereview.chromium.org/104593010/diff/90001/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/90001/sql/connection_unittest....
sql/connection_unittest.cc:840: }
Per offline discussion, given a table with an index, CorruptTableOrIndex() can
be used to make a result which will pass quick-check but fail full-check.  It's
a little more complicated to setup (needs table, index, and data), so I'm fine
with a TODO(shess): or something.

The problem with getting overly concerned about testing these negatives is that
the REAL concern is making sure it never generates false positives.  So I don't
think we need to get too worked up about it.

https://codereview.chromium.org/104593010/diff/90001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/90001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:87:
ASSERT_TRUE(base::CreateDirectory(mock_dir));
Yeah, that's what I meant.

[michaeln, 2013-12-13 23:57:00]

starting some tryjobs...

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/104593010/diff/70001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_database_unittest.cc:81: // to the consumer so 
we can test the error handling of the consumer.
> I think CorruptSizeInHeader() will let it get through things, though let me
know
> if it doesn't.

The test is modifed to use that helper, but... it gets tripped up on a
DLOG(FATAL) in the Preload() method. I've opened that bug like we talked about
and restored the NDEBUG only'ness.

[michaeln, 2013-12-14 01:27:29]

Fixed the clang error and started some new tryjobs... ptal

[Scott Hess - ex-Googler, 2013-12-14 16:40:29]

lgtm

[commit-bot: I haz the power, 2013-12-16 00:40:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/104593010/130001

[commit-bot: I haz the power, 2013-12-16 00:55:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-16 08:24:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/104593010/130001

[commit-bot: I haz the power, 2013-12-16 09:55:17]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 15:57:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/104593010/130001

[commit-bot: I haz the power, 2013-12-16 18:23:49]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 18:28:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/104593010/130001

[commit-bot: I haz the power, 2013-12-16 18:37:00]

Change committed as 240937