Fix SyncManager initialization failure crash.

Fix SyncManager initialization failure crash.

This crash was introduced in r148926, which changes SyncManager init
behaviour.  Previously, the SyncManager would end up with a valid
SyncScheduler regardless of whether or not the initialization failed.
This SyncScheduler would later play an important role in shutting down
the syncer.  Without a scheduler, the shutdown process crashes.

The ProfileSyncServiceTest.CorruptDatabase test simulates the scenario
we fail to load a sync directory.  This will result in a SyncManager
initialization failure.  It is intended to test that this failure is
handled by the ProfileSyncService, SyncBackendHost and SyncManager.

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

[tim (not reviewing), 2012-07-31 19:40:26]

http://codereview.chromium.org/10830100/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10830100/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:852: sync_manager_.reset();
The main danger with this is that SyncManager needs to make sure it tears down
what it needs to if it detects failure. We actually carefully tear things down
in StopSyncingForShutdown and ShutdownOnSyncThread, but this will mean we just
rely on declaration orders / destructors. 

I'd recommend we move the call to set up connection_manager_ to *after* we
return false for good measure -- if we created that and (for example) set up the
NetworkChangeNotifier, we'd be in danger of hanging on shutdown since we'd
return false but never call TerminateAllIO (because we'd never call
StopSyncingForShutdown).

I'd also recommend, if we can, that we call
sync_manager_->ShutdownOnSyncThread() before reset here.

[rlarocque, 2012-07-31 20:14:07]

Updated in response to Tim's comments.

+Fred: Do you know when the notifier starts posting tasks to the sync loop?  Is
the destruction order being proposed here likely to leave behind any notifier
tasks?

http://codereview.chromium.org/10830100/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10830100/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:852: sync_manager_.reset();
On 2012/07/31 19:40:26, timsteele wrote:
> The main danger with this is that SyncManager needs to make sure it tears down
> what it needs to if it detects failure. We actually carefully tear things down
> in StopSyncingForShutdown and ShutdownOnSyncThread, but this will mean we just
> rely on declaration orders / destructors. 
> 
> I'd recommend we move the call to set up connection_manager_ to *after* we
> return false for good measure -- if we created that and (for example) set up
the
> NetworkChangeNotifier, we'd be in danger of hanging on shutdown since we'd
> return false but never call TerminateAllIO (because we'd never call
> StopSyncingForShutdown).

Good idea.  Done.

> I'd also recommend, if we can, that we call
> sync_manager_->ShutdownOnSyncThread() before reset here.

Done.

[tim (not reviewing), 2012-07-31 21:14:36]

I'm not 100% sure when the notifier starts posting to the sync loop, so yeah
Fred should know.  I just know the posted tasks won't reference the SyncManager
(via observer) until the SBH calls UpdateEnabledTypes.

Pending data from Fred, this LGTM

http://codereview.chromium.org/10830100/diff/1012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

http://codereview.chromium.org/10830100/diff/1012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:85: true, true, false,
true, true, syncer::STORAGE_IN_MEMORY);
+100

http://codereview.chromium.org/10830100/diff/1012/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (left):

http://codereview.chromium.org/10830100/diff/1012/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:688:
connection_manager_->set_client_id(directory()->cache_guid());
ah, yeah, good to move this up

http://codereview.chromium.org/10830100/diff/1012/sync/syncable/invalid_direc...
File sync/syncable/invalid_directory_backing_store.h (right):

http://codereview.chromium.org/10830100/diff/1012/sync/syncable/invalid_direc...
sync/syncable/invalid_directory_backing_store.h:16:
InvalidDirectoryBackingStore();
virtual dtor

[akalin, 2012-07-31 21:34:18]

On 2012/07/31 20:14:07, rlarocque wrote:
> Updated in response to Tim's comments.
> 
> +Fred: Do you know when the notifier starts posting tasks to the sync loop? 
Is
> the destruction order being proposed here likely to leave behind any notifier
> tasks?

It's after UpdateEnabledTypes, etc. is called on the notifier, but you shouldn't
need to worry about that.  NonBlockingInvalidationNotifier handles the sync/IO
thread communication, and behaves like a single-threaded SyncNotifier (i.e.,
UpdateRegisteredIds(handler, ObjectIdSet()) should immediately stop
notifications for handler [formerly RemoveObserver(handler)]).

But yeah, this destruction order LGTM.

[rlarocque, 2012-07-31 21:36:00]

http://codereview.chromium.org/10830100/diff/1012/sync/syncable/invalid_direc...
File sync/syncable/invalid_directory_backing_store.h (right):

http://codereview.chromium.org/10830100/diff/1012/sync/syncable/invalid_direc...
sync/syncable/invalid_directory_backing_store.h:16:
InvalidDirectoryBackingStore();
On 2012/07/31 21:14:36, timsteele wrote:
> virtual dtor

Good catch.  Done.

[commit-bot: I haz the power, 2012-07-31 21:36:47]

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

[commit-bot: I haz the power, 2012-07-31 23:01:00]

Change committed as 149306