Make SyncBackendRegistrar aware of loaded data

sync/internal_api/sync_manager_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/internal_api/sync_manager_impl.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 274 matching lines @@
 }
 
 void SyncManagerImpl::ThrowUnrecoverableError() {
   DCHECK(thread_checker_.CalledOnValidThread());
   ReadTransaction trans(FROM_HERE, GetUserShare());
   trans.GetWrappedTrans()->OnUnrecoverableError(
       FROM_HERE, "Simulating unrecoverable error for testing purposes.");
 }
 
 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;
     directory()->GetDownloadProgress(i.Get(), &marker);
 
     if (marker.token().empty())
       result.Put(i.Get());
 
   }
   return result;
@@ skipping 50 matching lines @@
 }
 
 bool SyncManagerImpl::Init(
     const FilePath& database_location,
     const WeakHandle<JsEventHandler>& event_handler,
     const std::string& sync_server_and_path,
     int port,
     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,
     const SyncCredentials& credentials,
     scoped_ptr<SyncNotifier> sync_notifier,
     const std::string& restored_key_for_bootstrapping,
     scoped_ptr<InternalComponentsFactory> internal_components_factory,
     Encryptor* encryptor,
     UnrecoverableErrorHandler* unrecoverable_error_handler,
     ReportUnrecoverableErrorFunction report_unrecoverable_error_function) {
   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());
 
   blocking_task_runner_ = blocking_task_runner;
 
   change_delegate_ = change_delegate;
 
   sync_notifier_ = sync_notifier.Pass();
 
   AddObserver(&js_sync_manager_observer_);
   SetJsEventHandler(event_handler);
 
   AddObserver(&debug_info_event_listener_);
 
   database_path_ = database_location.Append(
       syncable::Directory::kSyncDatabaseFilename);
   encryptor_ = encryptor;
   unrecoverable_error_handler_ = unrecoverable_error_handler;
   report_unrecoverable_error_function_ = report_unrecoverable_error_function;
 
   FilePath absolute_db_path(database_path_);
   file_util::AbsolutePath(&absolute_db_path);
   scoped_ptr<syncable::DirectoryBackingStore> backing_store =
       internal_components_factory->BuildDirectoryBackingStore(
           credentials.email, absolute_db_path).Pass();
 
   DCHECK(backing_store.get());
+  share_.name = credentials.email;
   share_.directory.reset(
       new syncable::Directory(encryptor_,
                               unrecoverable_error_handler_,
                               report_unrecoverable_error_function_,
                               backing_store.release()));
 
   connection_manager_.reset(new SyncAPIServerConnectionManager(
       sync_server_and_path, port, use_ssl, post_factory.release()));
 
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
   observing_ip_address_changes_ = true;
 
   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;
   listeners.push_back(&allstatus_);
   listeners.push_back(this);
   session_context_ = internal_components_factory->BuildContext(
       connection_manager_.get(),
       directory(),
-      model_safe_routing_info,
       workers,
       extensions_activity_monitor,
       &throttled_data_type_tracker_,
       listeners,
       &debug_info_event_listener_,
       &traffic_recorder_).Pass();
   session_context_->set_account_name(credentials.email);
   scheduler_ = internal_components_factory->BuildScheduler(
       name_, session_context_.get()).Pass();
 
-  bool success = SignIn(credentials);
+  scheduler_->Start(SyncScheduler::CONFIGURATION_MODE);
 
-  if (success) {
-    scheduler_->Start(SyncScheduler::CONFIGURATION_MODE);
+  initialized_ = true;
 
-    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);
 
-    // 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;
-
-    // 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,
                                     const base::Closure& done_callback) {
   DCHECK(initialized_);
   DCHECK(thread_checker_.CalledOnValidThread());
   GetSessionName(
       blocking_task_runner_,
       base::Bind(
           &SyncManagerImpl::UpdateCryptographerAndNigoriCallback,
@@ skipping 155 matching lines @@
       MakeWeakHandle(js_mutation_event_observer_.AsWeakPtr()));
 
   syncable::DirOpenResult open_result = syncable::NOT_INITIALIZED;
   open_result = directory()->Open(username_for_share(), this,
                                   transaction_observer);
   if (open_result != syncable::OPENED) {
     LOG(ERROR) << "Could not open share for:" << username_for_share();
     return false;
   }
 
+  // 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;
+
   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())
-    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);
-  return true;
-}
-
 bool SyncManagerImpl::PurgePartiallySyncedTypes() {
   ModelTypeSet partially_synced_types = ModelTypeSet::All();
   partially_synced_types.RemoveAll(InitialSyncEndedTypes());
   partially_synced_types.RemoveAll(GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet::All()));
 
   UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
                        partially_synced_types.Size());
   if (partially_synced_types.Empty())
     return true;
   return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
 }
 
 void SyncManagerImpl::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(credentials.email, share_.name);
   DCHECK(!credentials.email.empty());
   DCHECK(!credentials.sync_token.empty());
 
   observing_ip_address_changes_ = true;
   if (connection_manager_->set_auth_token(credentials.sync_token)) {
     sync_notifier_->UpdateCredentials(
         credentials.email, credentials.sync_token);
     if (initialized_) {

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

Do we actually need this if (initialized_) block anyway?  Seems like it should
be safe to always call OnCredentialsUpdated, and the scheduler should figure
what matters itself?  I think just removing the if() is safe today, but looking
at scheduler_impl I see an argument that it should not pay attention if it
hasn't even been started yet (it surely hasn't seen a SyncSession go by and had
UpdateServerConnectionManagerStatus called!). 

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

See comment above.

       scheduler_->OnCredentialsUpdated();
     }
   }
 }
 
 void SyncManagerImpl::UpdateEnabledTypes(
     const ModelTypeSet& enabled_types) {
   DCHECK(thread_checker_.CalledOnValidThread());
   sync_notifier_->UpdateEnabledTypes(enabled_types);
 }
@@ skipping 1156 matching lines @@
 int SyncManagerImpl::GetDefaultNudgeDelay() {
   return kDefaultNudgeDelayMilliseconds;
 }
 
 // static.
 int SyncManagerImpl::GetPreferencesNudgeDelay() {
   return kPreferencesNudgeDelayMilliseconds;
 }
 
 }  // namespace syncer