Remove initial_sync_ended bits

Remove initial_sync_ended bits

As of crbug.com/164288, we can no longer trust initial_sync_ended.  If
my diagnosis is correct, then there are some clients out there who have
their initial_sync_ended bits set for some data types which have not
actually been synced.  Those clients now crash at every startup.

This change attempts to solve the problem by removing the separate
initial_sync_ended array of bools and checking the database directly to
learn of a type's status.  The new definition of initial_sync_ended is
that a type has finished initial sync if its top level folder has been
downloaded and applied.

To prevent crashing if the server decides to not send us those root
nodes during the control types configure step, we now check that all
control types are initial_sync_ended following that configure and fail
gracefully if their initial sync is not completed.

The new definition of initial_sync_ended makes it much easier to hit
crbug.com/164914.  That bug would prevent us from saving our changes
following a call to PurgeEntriesWithTypeIn() in some situations.  This
commit also fixes that bug.

Since the initial sync ended status is no longer independent from the
database state, it is no longer necessary to collect debug info as to
its status.  It has been removed from the about:sync UI and other debug
info reporting mechanisms.  Its role in unit tests has also been greatly
reduced.

BUG=164288, 164914
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172707

[rlarocque, 2012-12-07 22:39:04]

Please review.

[Nicolas Zea, 2012-12-10 22:50:38]

Describe crbug.com/164914 in commit message too?

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:682: // until they succeed or the
browser is closed.
nit: s/browser is closed/backend is shut down

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1243: DVLOG(ERROR) << "Failed to
download control types";
I think it makes sense for this to be a LOG(ERROR), as it's effectively fatal
for sync.

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/public/s...
File sync/internal_api/public/sessions/sync_session_snapshot.h (left):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/public/s...
sync/internal_api/public/sessions/sync_session_snapshot.h:34: bool
is_share_usable,
why is is_share_usable being removed as well? (I'll be honest, I don't even know
what it's for)

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (left):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2877: // Verify all the disabled
types were purged.
Doesn't this still apply?

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2936:
EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
I guess we don't actually create roots for these tests, so can't verify they're
removed?

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2997: // Also add a pending
autofill update from the server.
Can you use the test entry factory for this?

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:3056: // by the previous and
current enabled params. Enabled partial types should not
nit: Last sentence no longer applies.

[tim (not reviewing), 2012-12-10 23:05:36]

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/public/s...
File sync/internal_api/public/sessions/sync_session_snapshot.h (left):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/public/s...
sync/internal_api/public/sessions/sync_session_snapshot.h:34: bool
is_share_usable,
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> why is is_share_usable being removed as well? (I'll be honest, I don't even
know
> what it's for)

It used to denote "initial sync ended for all datatypes", and we wouldn't let
anything interact with the sync engine until that condition was met.  The
concept doesn't really make sense anymore though and AFAICT it's not used in any
important way. I think AllStatus uses it, but at the time is_share_usable was
relevant AllStatus wasn't, so I doubt it's doing anything important.

[rlarocque, 2012-12-11 01:44:48]

Patch and commit message updated.  PTAL.

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:682: // until they succeed or the
browser is closed.
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> nit: s/browser is closed/backend is shut down

Done.

https://codereview.chromium.org/11474036/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1243: DVLOG(ERROR) << "Failed to
download control types";
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> I think it makes sense for this to be a LOG(ERROR), as it's effectively fatal
> for sync.

Done.

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (left):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2877: // Verify all the disabled
types were purged.
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> Doesn't this still apply?

Yes it does.  Fixed.

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2936:
EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> I guess we don't actually create roots for these tests, so can't verify
they're
> removed?

More or less, yes.

The test harness creates root nodes for some types.  See lines 850 and 870 for
more info.  Others, like AUTOFILL, are not touched at all by this test.  That
breaks the assertion on line 2936, since AUTOFILL was never InitialSyncEnded(),
yet it is not in the 'disabled_types' set.

It would be possible to try to keep this assertion, but I don't think it's worth
the effort.  I think the point of it was to ensure that the initial_sync_ended
bits remained in sync with our database status.  Following this change, such
bugs are much less likely to occur, so the assertion is less valuable.

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:2997: // Also add a pending
autofill update from the server.
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> Can you use the test entry factory for this?

That would be the first use of TestEntryFactory from this file.  I agree it's a
good idea, though.

Done.

https://codereview.chromium.org/11474036/diff/2001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:3056: // by the previous and
current enabled params. Enabled partial types should not
On 2012/12/10 22:50:38, Nicolas Zea wrote:
> nit: Last sentence no longer applies.

Done.

[Nicolas Zea, 2012-12-11 02:12:40]

https://codereview.chromium.org/11474036/diff/9001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11474036/diff/9001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1242: if
(!sync_manager_->InitialSyncEndedTypes().HasAll(syncer::ControlTypes())) {
Could you add a unit test for this case? (i.e. where configuration does not
actually download/apply the control types)

[rlarocque, 2012-12-11 19:23:03]

Patch updated with a new unit test.  PTAL.

https://codereview.chromium.org/11474036/diff/9001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11474036/diff/9001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1242: if
(!sync_manager_->InitialSyncEndedTypes().HasAll(syncer::ControlTypes())) {
On 2012/12/11 02:12:40, Nicolas Zea wrote:
> Could you add a unit test for this case? (i.e. where configuration does not
> actually download/apply the control types)

Good idea.  Done.

[Nicolas Zea, 2012-12-11 19:35:00]

https://codereview.chromium.org/11474036/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/11474036/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_unittest.cc:429:
harness_.StartSyncServiceAndSetInitialSyncEnded(false, true, true, true,
I'd prefer having this test in SBH's unit tests, and actually verifying that the
OnBackendInitialized is called with a failure result. This test relies on a
somewhat heavily modified SBH implementation, and doesn't actually pay attention
to the SBH results.

[rlarocque, 2012-12-11 22:59:33]

Added another test.  PTAL.

[Nicolas Zea, 2012-12-11 23:12:36]

Thanks, LGTM

[rlarocque, 2012-12-12 01:19:16]

The rebase had a lot of conflict, particularly in directory_backing_store.cc and
directory_backing_store_unittest.cc.  

I think the changes are significant enough that they deserve another look.  Much
of it is cleaning up previously written migration code.  PTAL.

https://codereview.chromium.org/11474036/diff/21002/sync/syncable/directory_b...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/11474036/diff/21002/sync/syncable/directory_b...
sync/syncable/directory_backing_store.cc:384: // TODO(rlarocque): It's wrong to
recreate the database here unless the higher
I've been meaning to delete this comment for a while.  It's been obsolete for ~6
months.

https://codereview.chromium.org/11474036/diff/21002/sync/syncable/directory_b...
sync/syncable/directory_backing_store.cc:1067: // Version 82 added
transaction_version to kernel info. But if user is
This is the wrong way to handle a change in the CreateModelsTable() function. 
The right way is to modify the Version74->75 migration to use a different
CreateModelsTable() function, one that's frozen in time and won't be affected by
future changes.  That's what I've done in this change.

https://codereview.chromium.org/11474036/diff/28001/sync/syncable/directory_b...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/11474036/diff/28001/sync/syncable/directory_b...
sync/syncable/directory_backing_store_unittest.cc:1973: "INSERT INTO
'share_version' VALUES('nick@chromium.org',82);"
I'm surprised this code ever worked.  The Version83 set up code has the same
bug.

[Nicolas Zea, 2012-12-12 22:03:55]

lgtm

[commit-bot: I haz the power, 2012-12-12 22:11:10]

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

[commit-bot: I haz the power, 2012-12-12 23:53:21]

Change committed as 172707