Make SyncBackendRegistrar aware of loaded data

sync/internal_api/sync_manager_impl.cc

Index: sync/internal_api/sync_manager_impl.cc
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 6a164d8f0ff2aac4ff56893cfc13914c323e1737..d456f7c3773cb2a33993987ff8815f0bfc3c44e1 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -292,13 +292,11 @@ void SyncManagerImpl::ThrowUnrecoverableError() {
 }
 
 ModelTypeSet SyncManagerImpl::InitialSyncEndedTypes() {
-  DCHECK(initialized_);

[tim (not reviewing), 2012/07/26 22:33:48]

It still seems wrong from a client of SyncManager (i.e. SyncBackendHost) point
of view to call anything on us before we're initialized :/  Can we keep this
DCHECK somehow?

[rlarocque, 2012/07/26 22:52:20]

My first instinct was to downgrade this to a DCHECK(directory()).  directory()
and initialized_ correlate very well, from the SBH's point of view.

Then I realized that there's no point in asserting on a pointer we're about to
de-reference anyway, so I removed it.

I know that doesn't quite answer your question, but it was enough to convince me
that removing the DCHECK made sense.

[tim (not reviewing), 2012/07/27 01:54:32]

True re: asserting on a pointer we're about to deref isn't useful. But I think
the DCHECK is more about obeying the "Don't use SyncManager before it's
initialized" contract than it is about directory() being null or not.  There may
be other cases where we won't necessarily crash if a method is invoked
pre-initialization, but the result is unexpected / undefined, and DCHECK(init)
is needed. It could change here in the future, too.  So I would argue the best
practice is to have the dchecks in any public function exposed here.

[rlarocque, 2012/07/27 17:47:19]

Resolved offline.

   return directory()->initial_sync_ended_types();
 }
 
 ModelTypeSet SyncManagerImpl::GetTypesWithEmptyProgressMarkerToken(
     ModelTypeSet types) {
-  DCHECK(initialized_);
   ModelTypeSet result;
   for (ModelTypeSet::Iterator i = types.First(); i.Good(); i.Inc()) {
     sync_pb::DataTypeProgressMarker marker;
@@ -369,7 +367,6 @@ bool SyncManagerImpl::Init(
     bool use_ssl,
     const scoped_refptr<base::TaskRunner>& blocking_task_runner,
     scoped_ptr<HttpPostProviderFactory> post_factory,
-    const ModelSafeRoutingInfo& model_safe_routing_info,
     const std::vector<ModelSafeWorker*>& workers,
     ExtensionsActivityMonitor* extensions_activity_monitor,
     SyncManager::ChangeDelegate* change_delegate,
@@ -383,6 +380,8 @@ bool SyncManagerImpl::Init(
   CHECK(!initialized_);
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(post_factory.get());
+  DCHECK(!credentials.email.empty());
+  DCHECK(!credentials.sync_token.empty());
   DVLOG(1) << "SyncManager starting Init...";
 
   weak_handle_this_ = MakeWeakHandle(weak_ptr_factory_.GetWeakPtr());
@@ -411,6 +410,7 @@ bool SyncManagerImpl::Init(
           credentials.email, absolute_db_path).Pass();
 
   DCHECK(backing_store.get());
+  share_.name = credentials.email;
   share_.directory.reset(
       new syncable::Directory(encryptor_,
                               unrecoverable_error_handler_,
@@ -425,6 +425,36 @@ bool SyncManagerImpl::Init(
 
   connection_manager_->AddListener(this);
 
+  DVLOG(1) << "Username: " << username_for_share();
+  if (!OpenDirectory()) {
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
+                      OnInitializationComplete(
+                          MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
+                          false, syncer::ModelTypeSet()));
+    return false;
+  }

[tim (not reviewing), 2012/07/26 22:33:48]

Yay, this looks way nicer.

+
+  // Retrieve and set the sync notifier state.
+  std::string unique_id = directory()->cache_guid();
+  DVLOG(1) << "Read notification unique ID: " << unique_id;
+  allstatus_.SetUniqueId(unique_id);
+  sync_notifier_->SetUniqueId(unique_id);
+
+  std::string state = directory()->GetNotificationState();
+  if (VLOG_IS_ON(1)) {
+    std::string encoded_state;
+    base::Base64Encode(state, &encoded_state);
+    DVLOG(1) << "Read notification state: " << encoded_state;
+  }
+
+  // TODO(tim): Remove once invalidation state has been migrated to new
+  // InvalidationStateTracker store. Bug 124140.
+  sync_notifier_->SetStateDeprecated(state);
+
+  connection_manager_->set_auth_token(credentials.sync_token);
+  sync_notifier_->UpdateCredentials(credentials.email,

[tim (not reviewing), 2012/07/26 22:33:48]

If we can get rid of the initialized_ special logic in UpdateCredentials (see
comment below) and still use it safely from here, then I'd say we should call
it.  If we can't, and the special casing is required, then I agree with you it's
just as good or better to make the if (initialized_) block in UpdateCredentials
a DCHECK(initialized_) and manually do the work here as you have it.

[rlarocque, 2012/07/26 22:52:20]

The scheduler doesn't exist at this point, so the initialized_ check would is
necessary if we were to call UpdateCredentials() on this line.

If we were to save the UpdateCredentials call until later, as was the case when
SignIn() was invoked immediately after the scheduler was initialized, then it
might be safe to remove it.

[tim (not reviewing), 2012/07/27 01:54:32]

Oh, right, you mentioned that above too.  Can we try that (move it later, remove
check)?  It's nice to reuse if it's not special, and doing the work once the
same components exist in all cases eliminates a bunch questions that may come up
when debugging (e.g.) a crash in a posted task, or bug that gets introduced
later.

[rlarocque, 2012/07/27 17:47:19]

Done.

I also moved the IPAddressChangeObserver registration because its callback
references the scheduler.  It would be very unlikely to cause problems in
practice, but it feels like it's one safe-looking CL away from developing into a
bug.

+                                    credentials.sync_token);
+
   // Build a SyncSessionContext and store the worker in it.
   DVLOG(1) << "Sync is bringing up SyncSessionContext.";
   std::vector<SyncEngineEventListener*> listeners;
@@ -433,7 +463,6 @@ bool SyncManagerImpl::Init(
   session_context_ = internal_components_factory->BuildContext(
       connection_manager_.get(),
       directory(),
-      model_safe_routing_info,
       workers,
       extensions_activity_monitor,
       &throttled_data_type_tracker_,
@@ -444,51 +473,25 @@ bool SyncManagerImpl::Init(
   scheduler_ = internal_components_factory->BuildScheduler(
       name_, session_context_.get()).Pass();
 
-  bool success = SignIn(credentials);
-
-  if (success) {
-    scheduler_->Start(SyncScheduler::CONFIGURATION_MODE);
-
-    initialized_ = true;
-
-    // Unapplied datatypes (those that do not have initial sync ended set) get
-    // re-downloaded during any configuration. But, it's possible for a datatype
-    // to have a progress marker but not have initial sync ended yet, making
-    // it a candidate for migration. This is a problem, as the DataTypeManager
-    // does not support a migration while it's already in the middle of a
-    // configuration. As a result, any partially synced datatype can stall the
-    // DTM, waiting for the configuration to complete, which it never will due
-    // to the migration error. In addition, a partially synced nigori will
-    // trigger the migration logic before the backend is initialized, resulting
-    // in crashes. We therefore detect and purge any partially synced types as
-    // part of initialization.
-    if (!PurgePartiallySyncedTypes())
-      success = false;
+  scheduler_->Start(SyncScheduler::CONFIGURATION_MODE);
 
-    // Cryptographer should only be accessed while holding a
-    // transaction.  Grabbing the user share for the transaction
-    // checks the initialization state, so this must come after
-    // |initialized_| is set to true.
-    ReadTransaction trans(FROM_HERE, GetUserShare());
-    trans.GetCryptographer()->Bootstrap(restored_key_for_bootstrapping);
-    trans.GetCryptographer()->AddObserver(this);
-  }
+  initialized_ = true;
+
+  // Cryptographer should only be accessed while holding a
+  // transaction.  Grabbing the user share for the transaction
+  // checks the initialization state, so this must come after
+  // |initialized_| is set to true.
+  ReadTransaction trans(FROM_HERE, GetUserShare());
+  trans.GetCryptographer()->Bootstrap(restored_key_for_bootstrapping);
+  trans.GetCryptographer()->AddObserver(this);
 
-  // Notify that initialization is complete. Note: This should be the last to
-  // execute if |signed_in| is false. Reason being in that case we would
-  // post a task to shutdown sync. But if this function posts any other tasks
-  // on the UI thread and if shutdown wins then that tasks would execute on
-  // a freed pointer. This is because UI thread is not shut down.
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        success));
-  if (!success)
-    return false;
+                        true, InitialSyncEndedTypes()));
 
   sync_notifier_->AddObserver(this);
-
-  return success;
+  return true;
 }
 
 void SyncManagerImpl::RefreshNigori(const std::string& chrome_version,
@@ -664,36 +667,21 @@ bool SyncManagerImpl::OpenDirectory() {
     return false;
   }
 
-  connection_manager_->set_client_id(directory()->cache_guid());
-  return true;
-}
-
-bool SyncManagerImpl::SignIn(const SyncCredentials& credentials) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(share_.name.empty());
-  share_.name = credentials.email;
-
-  DVLOG(1) << "Signing in user: " << username_for_share();
-  if (!OpenDirectory())
+  // Unapplied datatypes (those that do not have initial sync ended set) get
+  // re-downloaded during any configuration. But, it's possible for a datatype
+  // to have a progress marker but not have initial sync ended yet, making
+  // it a candidate for migration. This is a problem, as the DataTypeManager
+  // does not support a migration while it's already in the middle of a
+  // configuration. As a result, any partially synced datatype can stall the
+  // DTM, waiting for the configuration to complete, which it never will due
+  // to the migration error. In addition, a partially synced nigori will
+  // trigger the migration logic before the backend is initialized, resulting
+  // in crashes. We therefore detect and purge any partially synced types as
+  // part of initialization.
+  if (!PurgePartiallySyncedTypes())
     return false;
 
-  // Retrieve and set the sync notifier state. This should be done
-  // only after OpenDirectory is called.
-  std::string unique_id = directory()->cache_guid();
-  std::string state = directory()->GetNotificationState();
-  DVLOG(1) << "Read notification unique ID: " << unique_id;
-  if (VLOG_IS_ON(1)) {
-    std::string encoded_state;
-    base::Base64Encode(state, &encoded_state);
-    DVLOG(1) << "Read notification state: " << encoded_state;
-  }
-  allstatus_.SetUniqueId(unique_id);
-  sync_notifier_->SetUniqueId(unique_id);
-  // TODO(tim): Remove once invalidation state has been migrated to new
-  // InvalidationStateTracker store. Bug 124140.
-  sync_notifier_->SetStateDeprecated(state);
-
-  UpdateCredentials(credentials);
+  connection_manager_->set_client_id(directory()->cache_guid());
   return true;
 }