[Sync] Update DirectoryBackingStore to detect Sync DB corruption

[Sync] Update DirectoryBackingStore to detect Sync DB corruption

Leverage sql::Connection's error handling mechanism to detect
"catastrophic" errors like SQLITE_CORRUPT.

Add unit tests that verify behavior of SetCatastrophicErrorHandler and
trigger DB corruption.

Because handler is not installed by default, this change will have no
impact on Sync's behavior. A future change will connect Directory to
DirectoryBackingStore's to ensure Sync is shutdown when DB corruption
occurs.

This change depends on https://codereview.chromium.org/1069313004/.

BUG=470993
Committed: https://crrev.com/4c4a52e3948bbf601248983afedb4e65d5a7ac2f
Cr-Commit-Position: refs/heads/master@{#325135}

[maniscalco, 2015-04-10 16:31:34]

maniscalco@chromium.org changed reviewers:
+ shess@chromium.org

[maniscalco, 2015-04-10 16:31:35]

Hi Scott, would you please take a look at directory_backing_store_unittest.cc. 
I want to make sure my use of ScopedErrorIgnorer and sql:Connection's
error_callback make sense together.

DirectoryBackingStore::SetCatastrophicErrorHandler installs an error_callback. 
The test uses a ScopedErrorIgnorer to ensure that in Debug builds the test runs
to completion.  This all seems to work fine (after applying issue 1075993003). 
I understand that in the future we may run into other places in sql:Connection
(or related classes) where ScopedErrorIgnorer won't prevent a DLOG(FATAL).  I'm
assuming we would deal with those on a case by case basis when/if that happens. 
WDYT?

[maniscalco, 2015-04-13 19:54:40]

maniscalco@chromium.org changed reviewers:
+ zea@chromium.org

[maniscalco, 2015-04-13 19:54:41]

Nicolas, would you please review this change?

[Nicolas Zea, 2015-04-13 20:16:33]

LGTM

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi#n...
sync/sync_tests.gypi:252: '../sql/sql.gyp:test_support_sql',
strange, does sql have the target named two different ways in gyp vs gn
(test_support vs test_support_sql)?

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store.cc:51: task_runner->PostTask(FROM_HERE,
catastrophic_error_handler);
nit: do you need the task runner? Can you just post to
base::MessageLoop::current()?

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store_unittest.cc:4031: const base::Closure
handler =
nit: const vars should be of form kVariableName.

[Scott Hess - ex-Googler, 2015-04-13 20:26:46]

LGTM, I think this seems reasonable.

I'm slightly nervous about whether the "post a task to clean up later" might
someday interact poorly with something else such that cleanup never happens.

[maniscalco, 2015-04-14 16:11:59]

On 2015/04/13 20:26:46, Scott Hess wrote:
> LGTM, I think this seems reasonable.
> 
> I'm slightly nervous about whether the "post a task to clean up later" might
> someday interact poorly with something else such that cleanup never happens.

Thanks for the feedback.  In a future patch I'm working on, this error handler
task will be connected to SyncManagerImpl, who is (transitively) responsible for
the lifetime of DirectoryBackingStore.  That patch will have unit tests that
verify the DB gets cleaned up.

Another approach I considered, but decided to call out of scope for now is to
synchronously write a db_is_corrupt sentinel file and then change the Open/Load
code to look for the sentinel and destroy the DB if found.  I'm expecting that
with the existing UMA counters we should see a reduction in number of
SQLITE_CORRUPT errors for the sync DB once I've wired things up to
SyncManagerImpl.  If we don't see a reduction, it may be wise to pursue the
sentinel approach (or something similar).

[maniscalco, 2015-04-14 17:44:15]

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi#n...
sync/sync_tests.gypi:252: '../sql/sql.gyp:test_support_sql',
On 2015/04/13 20:16:33, Nicolas Zea wrote:
> strange, does sql have the target named two different ways in gyp vs gn
> (test_support vs test_support_sql)?

Yes, two different names.  I believe the name used in gn is the more standard
naming scheme.

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store.cc:51: task_runner->PostTask(FROM_HERE,
catastrophic_error_handler);
On 2015/04/13 20:16:33, Nicolas Zea wrote:
> nit: do you need the task runner? Can you just post to
> base::MessageLoop::current()?

Good idea.  Done.

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store_unittest.cc:4031: const base::Closure
handler =
On 2015/04/13 20:16:33, Nicolas Zea wrote:
> nit: const vars should be of form kVariableName.

Definitely when they are global or class scoped.  For local variables I believe
it can be done either way.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names

So far it seems like underscore_naming_scheme is more common for const locals so
I figured I'd keep doing that.

[maniscalco, 2015-04-14 19:27:20]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2015-04-14 19:27:20]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1072093002/#ps140001
(title: "Apply CR feedback.")

[commit-bot: I haz the power, 2015-04-14 19:27:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072093002/140001

[Scott Hess - ex-Googler, 2015-04-14 19:51:01]

On 2015/04/14 16:11:59, maniscalco wrote:
> On 2015/04/13 20:26:46, Scott Hess wrote:
> > LGTM, I think this seems reasonable.
> > 
> > I'm slightly nervous about whether the "post a task to clean up later" might
> > someday interact poorly with something else such that cleanup never happens.
> 
> Thanks for the feedback.  In a future patch I'm working on, this error handler
> task will be connected to SyncManagerImpl, who is (transitively) responsible
for
> the lifetime of DirectoryBackingStore.  That patch will have unit tests that
> verify the DB gets cleaned up.
> 
> Another approach I considered, but decided to call out of scope for now is to
> synchronously write a db_is_corrupt sentinel file and then change the
Open/Load
> code to look for the sentinel and destroy the DB if found.  I'm expecting that
> with the existing UMA counters we should see a reduction in number of
> SQLITE_CORRUPT errors for the sync DB once I've wired things up to
> SyncManagerImpl.  If we don't see a reduction, it may be wise to pursue the
> sentinel approach (or something similar).

RazeAndClose() should clear the database to empty anywhere where you could
otherwise update it, mod transactions.  It could probably be extended to fake
things out somehow on that front, too.

On Google Gears multiple processes could hit the database, and there wasn't a
strong way to coordinate (which meant that under Windows you couldn't delete the
file), so we overwrote the header to change from "SQLite version 3\0" to "SQLite
poison 3\0".  Then whoever next reached it would know to destroy things.  I
think RazeAndClose() is a stronger approach, but it could be stymied by various
non-Chromium actors, like some other process messing with locking or whatever.

If you have ideas which need to get outside of these boundaries, please think on
ways to "upstream" them.  The main reason I haven't let my imagination run wild
is because there's no point to coding up subtle solutions to problems if they
are more dangerous than no solution :-).  Best case is instrumenting things to
figure out where the problem cases are so we can be certain we're addressing
them, and not just obfuscating things.

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/1072093002/diff/120001/sync/sync_tests.gypi#n...
sync/sync_tests.gypi:252: '../sql/sql.gyp:test_support_sql',
On 2015/04/14 17:44:14, maniscalco wrote:
> On 2015/04/13 20:16:33, Nicolas Zea wrote:
> > strange, does sql have the target named two different ways in gyp vs gn
> > (test_support vs test_support_sql)?
> 
> Yes, two different names.  I believe the name used in gn is the more standard
> naming scheme.

gyp seems to always say "namespace_test_supprt" or "test_support_namespace" and
I'm sure there's a "test_namespace_support".  Dunno why, I just copied the model
I found which seemed most similar to whatever I was attempting at the time.  I
suspect all of this is why GN doesn't namespace the namespace every other
namespace.

[commit-bot: I haz the power, 2015-04-14 21:50:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-14 21:51:45]

Patchset 8 (id:??) landed as
https://crrev.com/4c4a52e3948bbf601248983afedb4e65d5a7ac2f
Cr-Commit-Position: refs/heads/master@{#325135}