Update next_id when loading sync DB

Update next_id when loading sync DB

The directory code is smart enough to maintain a bit of extra slack in
the next_id so we don't accidentally reuse IDs if we crash.  However,
it only works after we've saved changes to the directory at least once.

This change ensures that we save changes immediately after we load the
directory, so that we can recover from crashes that occur before we
would otherwise save changes due to natural causes.

This won't do anything to help users who've gotten into a bad state
because next_id was corrupted by an early crash, but it will prevent us
from corrupting databases in the future.

BUG=142987
TEST=See below:
Examine 'next_id' in sync DB.  Open chrome and close it within two to
ten seconds.  Verify 'next_id' has been incremented.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152706

[rlarocque, 2012-08-16 00:36:14]

Please review.

[Nicolas Zea, 2012-08-16 20:40:10]

http://codereview.chromium.org/10832329/diff/1/sync/syncable/dir_open_result.h
File sync/syncable/dir_open_result.h (right):

http://codereview.chromium.org/10832329/diff/1/sync/syncable/dir_open_result....
sync/syncable/dir_open_result.h:19: FAILED_INITIAL_WRITE,
comment about why we care?

http://codereview.chromium.org/10832329/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

http://codereview.chromium.org/10832329/diff/1/sync/syncable/directory.cc#new...
sync/syncable/directory.cc:213: result = FAILED_INITIAL_WRITE;
return FAILED_INITIAL_WRITE?

[Nicolas Zea, 2012-08-16 20:40:34]

Also, how tough would it be to add unit tests for this class?

[rlarocque, 2012-08-17 17:59:41]

On 2012/08/16 20:40:34, nzea wrote:
> Also, how tough would it be to add unit tests for this class?

Easier than I thought.  I've added two tests, one of which would have caught the
bug.

[rlarocque, 2012-08-17 18:02:07]

http://codereview.chromium.org/10832329/diff/1/sync/syncable/dir_open_result.h
File sync/syncable/dir_open_result.h (right):

http://codereview.chromium.org/10832329/diff/1/sync/syncable/dir_open_result....
sync/syncable/dir_open_result.h:19: FAILED_INITIAL_WRITE,
On 2012/08/16 20:40:10, nzea wrote:
> comment about why we care?

Done.

http://codereview.chromium.org/10832329/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

http://codereview.chromium.org/10832329/diff/1/sync/syncable/directory.cc#new...
sync/syncable/directory.cc:213: result = FAILED_INITIAL_WRITE;
On 2012/08/16 20:40:10, nzea wrote:
> return FAILED_INITIAL_WRITE?

Wow, that was a dumb mistake.  Fixed.

Unfortunately, I don't know how I could test this code path.  In fact, I'm not
even sure how this could happen.  If it does happen, though, it would be prudent
to not try using the database.

[Nicolas Zea, 2012-08-17 18:09:27]

LGTM

http://codereview.chromium.org/10832329/diff/7001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

http://codereview.chromium.org/10832329/diff/7001/sync/syncable/directory.cc#...
sync/syncable/directory.cc:212: if (!SaveChanges()) {
nit: remove curly braces

[rlarocque, 2012-08-17 22:03:03]

It was supposed to be simple.  This patch has now grown by a factor of 15.

Fixing the return value from OpenImpl() broke a few unit tests, which forced me
to refactor how we test SaveChanges() failures.  This allowed me to test the
FAILED_INITIAL_WRITE code path, but it also required an extra ~150 lines of
diffs.

The diff on the latest upload is big enough that it deserves further review. 
Please review again.

[Nicolas Zea, 2012-08-21 00:26:35]

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1264: TEST_F(SyncableDirectoryTest,
LocalIdReuseTestWithSave) {
is this really testing anything different than the above?

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1347: TestBackingStore(const std::string&
dir_name,
virtual destructor

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1357: bool fail_save_changes_;
make private

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1377: public:
virtual destructor

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1440: test_directory_ =
reuse the ReloadDir method? dir_->good should probably be in ReloadDir anyways

[rlarocque, 2012-08-21 00:54:35]

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1264: TEST_F(SyncableDirectoryTest,
LocalIdReuseTestWithSave) {
On 2012/08/21 00:26:35, nzea wrote:
> is this really testing anything different than the above?

They're slightly different.  This bug only affected clients that crashed before
the first call to SaveChanges().  Before this fix, LocalIdReuseTest would have
failed but LocalIdReuseTestWithSave would not.

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1347: TestBackingStore(const std::string&
dir_name,
On 2012/08/21 00:26:35, nzea wrote:
> virtual destructor

Done.

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1357: bool fail_save_changes_;
On 2012/08/21 00:26:35, nzea wrote:
> make private

Done.

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1377: public:
On 2012/08/21 00:26:35, nzea wrote:
> virtual destructor

Done.

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1440: test_directory_ =
On 2012/08/21 00:26:35, nzea wrote:
> reuse the ReloadDir method? dir_->good should probably be in ReloadDir anyways

Done.

[Nicolas Zea, 2012-08-21 01:08:56]

LGTM

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10832329/diff/9001/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1264: TEST_F(SyncableDirectoryTest,
LocalIdReuseTestWithSave) {
On 2012/08/21 00:54:35, rlarocque wrote:
> On 2012/08/21 00:26:35, nzea wrote:
> > is this really testing anything different than the above?
> 
> They're slightly different.  This bug only affected clients that crashed
before
> the first call to SaveChanges().  Before this fix, LocalIdReuseTest would have
> failed but LocalIdReuseTestWithSave would not.

I was thinking that if LocalIdReuseTest succeeds, then this one basically has to
succeed. But I guess that's not true the other way around, so it still has some
value.

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1451: ASSERT_EQ(OPENED, dir_->Open(kName,
&delegate_,
This can be removed.

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1453: ASSERT_TRUE(dir_->good());
Move into CreateDirectory()?

[commit-bot: I haz the power, 2012-08-21 18:19:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10832329/5003

[rlarocque, 2012-08-21 18:27:29]

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1451: ASSERT_EQ(OPENED, dir_->Open(kName,
&delegate_,
On 2012/08/21 01:08:56, nzea wrote:
> This can be removed.

Done.

http://codereview.chromium.org/10832329/diff/8002/sync/syncable/syncable_unit...
sync/syncable/syncable_unittest.cc:1453: ASSERT_TRUE(dir_->good());
On 2012/08/21 01:08:56, nzea wrote:
> Move into CreateDirectory()?

Done.

[commit-bot: I haz the power, 2012-08-21 18:58:28]

Try job failure for 10832329-5003 (retry) on linux_chromeos for steps
"interactive_ui_tests, browser_tests, unit_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests,
unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-21 22:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10832329/12006

[commit-bot: I haz the power, 2012-08-22 01:04:21]

Change committed as 152706