[Sync] Erase sync DB when corrupted

[Sync] Erase sync DB when corrupted

Directory now registers to be notified of catastrophic sync DB errors.
Upon notification, Directory triggers an unrecoverable sync error which
causes ProfileSyncService to delete the sync DB.

Add integration test that verifies the sync DB is deleted when DB
corruption is detected.

Remove unused member variable allow_failure_for_test_ from
DirectoryBackingStore.

Add MockUnrecoverableErrorHandler to assist in writing tests for
Directory.

BUG=470993
Committed: https://crrev.com/90a7f8d3e3b3a2aac43e71a93eac2731f084945c
Cr-Commit-Position: refs/heads/master@{#327120}

Committed: https://crrev.com/8382e4e5dce071e21f1297df234a17bdf7a5ed32
Cr-Commit-Position: refs/heads/master@{#327766}

[maniscalco, 2015-04-24 22:39:31]

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

[maniscalco, 2015-04-24 22:39:32]

Nicolas, would you please review this change?

Previously we talked about writing a sentinel file once we detected DB
corruption. The idea being that upon restart we'd see the sentinel and delete
the DB instead of loading it.

This CL does not implement the sentinel approach because I found that PSS
already supports deleting the DB when an unrecoverable error has occurred. I
decided to use the existing delete logic instead of introducing the sentinel
mechanism because it seems to work well for this case.

LMK if you think we should still do the write-sentinel/delete-if-exists thing. 
It would be easy to add in a subsequent patch.

Patrick, would you please review the integration test case,
c/b/sync/test/integration/single_client_directory_sync_test.cc ?

Thanks!

[pval...(no longer on Chromium), 2015-04-27 19:09:34]

test LGTM (one small non-blocking comment)

Thanks for taking the time to create an awesome integration test.

https://codereview.chromium.org/1082893003/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_directory_sync_test.cc
(right):

https://codereview.chromium.org/1082893003/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_directory_sync_test.cc:116:
// Write a bunch more bookmarks to ensure the sync DB notices the corruption.
Re: "sync DB notices the corruption"

Does this happen because adding the bookmarks triggers a sync cycle? If so,
consider mentioning it in this comment.

[maniscalco, 2015-04-27 19:16:41]

https://codereview.chromium.org/1082893003/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_directory_sync_test.cc
(right):

https://codereview.chromium.org/1082893003/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_directory_sync_test.cc:116:
// Write a bunch more bookmarks to ensure the sync DB notices the corruption.
On 2015/04/27 19:09:34, pvalenzuela wrote:
> Re: "sync DB notices the corruption"
> 
> Does this happen because adding the bookmarks triggers a sync cycle? If so,
> consider mentioning it in this comment.

Good point.  Flushing the directory is an important part of forcing sync to
notice the corruption.  I've updated the comment to clarify this.  Done.

[Nicolas Zea, 2015-04-27 19:17:33]

LGTM

https://codereview.chromium.org/1082893003/diff/20001/sync/syncable/directory...
File sync/syncable/directory_unittest.cc (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/syncable/directory...
sync/syncable/directory_unittest.cc:2045: dir.OnCatastrophicError();
out of curiosity, why twice?

https://codereview.chromium.org/1082893003/diff/20001/sync/test/directory_bac...
File sync/test/directory_backing_store_corruption_testing.h (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/test/directory_bac...
sync/test/directory_backing_store_corruption_testing.h:16: extern const int
kNumEntriesRequiredForCorruption;
any reason you don't just define the constant here?

https://codereview.chromium.org/1082893003/diff/20001/sync/util/mock_unrecove...
File sync/util/mock_unrecoverable_error_handler.h (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/util/mock_unrecove...
sync/util/mock_unrecoverable_error_handler.h:24: // Returns the number of times
this handelr has been invoked.
nit: handelr->handler

[maniscalco, 2015-04-27 19:51:17]

Thanks for the speedy reviews!

https://codereview.chromium.org/1082893003/diff/20001/sync/syncable/directory...
File sync/syncable/directory_unittest.cc (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/syncable/directory...
sync/syncable/directory_unittest.cc:2045: dir.OnCatastrophicError();
On 2015/04/27 19:17:32, Nicolas Zea wrote:
> out of curiosity, why twice?

Just to make sure Directory can tolerate since that can happen outside of tests.
 I've added a comment to clarify.

https://codereview.chromium.org/1082893003/diff/20001/sync/test/directory_bac...
File sync/test/directory_backing_store_corruption_testing.h (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/test/directory_bac...
sync/test/directory_backing_store_corruption_testing.h:16: extern const int
kNumEntriesRequiredForCorruption;
On 2015/04/27 19:17:32, Nicolas Zea wrote:
> any reason you don't just define the constant here?

If the value were defined in the header file, then each .o file could end up
with its own copy of the value (potentially causing binary bloat).  By declaring
extern in the .h and defining in the .cc, I can be sure there is only one copy
of the value.

https://codereview.chromium.org/1082893003/diff/20001/sync/util/mock_unrecove...
File sync/util/mock_unrecoverable_error_handler.h (right):

https://codereview.chromium.org/1082893003/diff/20001/sync/util/mock_unrecove...
sync/util/mock_unrecoverable_error_handler.h:24: // Returns the number of times
this handelr has been invoked.
On 2015/04/27 19:17:32, Nicolas Zea wrote:
> nit: handelr->handler

Done.

[maniscalco, 2015-04-27 20:43:28]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2015-04-27 20:43:29]

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

[commit-bot: I haz the power, 2015-04-27 20:45:01]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-27 21:16:34]

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

[vkuzkokov, 2015-04-28 11:26:37]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1104423005/ by vkuzkokov@chromium.org.

The reason for reverting is: Test fails on Win7 bots. see crbug.com/481895.

[maniscalco, 2015-04-28 23:39:21]

Nicolas, Patrick, PTAL at the diff between patch sets #4 and #5 (#4 is what you
reviewed previously).  #5 drops the test run time to about 7 seconds on my linux
box and 14 seconds on win_chromium_dbg_gn.

[maniscalco, 2015-04-29 15:18:14]

The CQ bit was checked by maniscalco@chromium.org to run a CQ dry run

[maniscalco, 2015-04-29 15:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
pvalenzuela@chromium.org
Link to the patchset: https://codereview.chromium.org/1082893003/#ps80001
(title: "Reduce test runtime by writing fewer bookmarks")

[commit-bot: I haz the power, 2015-04-29 15:18:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082893003/80001

[commit-bot: I haz the power, 2015-04-29 16:05:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-29 16:05:36]

Dry run: This issue passed the CQ dry run.

[pval...(no longer on Chromium), 2015-04-29 17:09:21]

lgtm

Great work reducing the time. This is a lot faster than patchset 4. By how much
exactly?

[maniscalco, 2015-04-29 19:48:53]

On 2015/04/29 17:09:21, pvalenzuela wrote:
> lgtm
> 
> Great work reducing the time. This is a lot faster than patchset 4. By how
much
> exactly?

Thanks!  In patch set #4 it was taking about 100 seconds on the
win_chromium_dbg_ng trybot (seems to be the slowest bot for this test).  With
patch set #5 it completed in under 14 seconds:

[27/125] SingleClientDirectorySyncTest.DeleteDirectoryWhenCorrupted (13523 ms)

And on my workstation it completes in under 8 seconds.

Hopefully the builders are at least as speedy as win_chromium_dbg_ng.

[Nicolas Zea, 2015-04-30 17:39:22]

lgtm

[maniscalco, 2015-04-30 17:42:55]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2015-04-30 17:43:31]

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

[commit-bot: I haz the power, 2015-04-30 19:39:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-30 19:41:23]

Patchset 5 (id:??) landed as
https://crrev.com/8382e4e5dce071e21f1297df234a17bdf7a5ed32
Cr-Commit-Position: refs/heads/master@{#327766}