[Sync] NonBlocking type registration clear instead of crashes when also registered as pssive.

components/sync/driver/glue/sync_backend_registrar.cc

Index: components/sync/driver/glue/sync_backend_registrar.cc
diff --git a/components/sync/driver/glue/sync_backend_registrar.cc b/components/sync/driver/glue/sync_backend_registrar.cc
index 02fe1c42f537c332ca0616506fc29549baaf2917..0b8a38961d7f3347935fc8be6e062cd882b2980e 100644
--- a/components/sync/driver/glue/sync_backend_registrar.cc
+++ b/components/sync/driver/glue/sync_backend_registrar.cc
@@ -4,6 +4,7 @@
 
 #include "components/sync/driver/glue/sync_backend_registrar.h"
 
+#include <algorithm>
 #include <cstddef>
 #include <utility>
 
@@ -14,6 +15,11 @@
 #include "components/sync/driver/change_processor.h"
 #include "components/sync/driver/sync_client.h"
 
+using syncer::ModelSafeGroup;
+using syncer::ModelSafeRoutingInfo;
+using syncer::ModelType;
+using syncer::ModelTypeSet;
+
 namespace browser_sync {
 
 SyncBackendRegistrar::SyncBackendRegistrar(
@@ -51,15 +57,22 @@ SyncBackendRegistrar::SyncBackendRegistrar(
   DCHECK_GT(workers_.size(), 0u);
 }
 
-void SyncBackendRegistrar::RegisterNonBlockingType(syncer::ModelType type) {
+void SyncBackendRegistrar::RegisterNonBlockingType(ModelType type) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   base::AutoLock lock(lock_);
-  DCHECK(routing_info_.find(type) == routing_info_.end() ||
-         routing_info_[type] == syncer::GROUP_NON_BLOCKING);
+  // There may have been a previously successful sync of a type when passive,
+  // which is now NonBlocking. We're not sure what order these two sets of types
+  // are being registered in, so guard against SetInitialTypes(...) having been
+  // already called by undoing everything to these types.
+  if (routing_info_.find(type) != routing_info_.end() &&
+      routing_info_[type] != syncer::GROUP_NON_BLOCKING) {
+    routing_info_.erase(type);
+    // last_configured_types_.Remove(type);

[skym, 2016/09/16 22:51:18]

Whhhhooops. Was making sure that commenting this line out broke unit tests, and
somehow it made it into uploaded copy. Fixing.

+  }
   non_blocking_types_.Put(type);
 }
 
-void SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
+void SyncBackendRegistrar::SetInitialTypes(ModelTypeSet initial_types) {
   base::AutoLock lock(lock_);
 
   // This function should be called only once, shortly after construction. The
@@ -69,9 +82,13 @@ void SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
   // Set our initial state to reflect the current status of the sync directory.
   // This will ensure that our calculations in ConfigureDataTypes() will always
   // return correct results.
-  for (syncer::ModelTypeSet::Iterator it = initial_types.First(); it.Good();
-       it.Inc()) {
-    routing_info_[it.Get()] = GetInitialGroupForType(it.Get());
+  for (ModelTypeSet::Iterator it = initial_types.First(); it.Good(); it.Inc()) {
+    // If this type is also registered as NonBlocking, assume that it shouldn't
+    // be registered as passive. The NonBlocking path will eventually take care
+    // of adding to routing_info_ later on.
+    if (!non_blocking_types_.Has(it.Get())) {
+      routing_info_[it.Get()] = syncer::GROUP_PASSIVE;
+    }
   }
 
   if (!workers_.count(syncer::GROUP_HISTORY)) {
@@ -86,10 +103,11 @@ void SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
     routing_info_.erase(syncer::PASSWORDS);
   }
 
+  // Although this can re-set NonBlocking types, this should be idempotent.
   last_configured_types_ = syncer::GetRoutingInfoTypes(routing_info_);
 }
 
-void SyncBackendRegistrar::AddRestoredNonBlockingType(syncer::ModelType type) {
+void SyncBackendRegistrar::AddRestoredNonBlockingType(ModelType type) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   base::AutoLock lock(lock_);
   DCHECK(non_blocking_types_.Has(type));
@@ -104,11 +122,11 @@ bool SyncBackendRegistrar::IsNigoriEnabled() const {
   return routing_info_.find(syncer::NIGORI) != routing_info_.end();
 }
 
-syncer::ModelTypeSet SyncBackendRegistrar::ConfigureDataTypes(
-    syncer::ModelTypeSet types_to_add,
-    syncer::ModelTypeSet types_to_remove) {
+ModelTypeSet SyncBackendRegistrar::ConfigureDataTypes(
+    ModelTypeSet types_to_add,
+    ModelTypeSet types_to_remove) {
   DCHECK(Intersection(types_to_add, types_to_remove).Empty());
-  syncer::ModelTypeSet filtered_types_to_add = types_to_add;
+  ModelTypeSet filtered_types_to_add = types_to_add;
   if (workers_.count(syncer::GROUP_HISTORY) == 0) {
     LOG(WARNING) << "No history worker -- removing TYPED_URLS";
     filtered_types_to_add.Remove(syncer::TYPED_URLS);
@@ -119,36 +137,34 @@ syncer::ModelTypeSet SyncBackendRegistrar::ConfigureDataTypes(
   }
 
   base::AutoLock lock(lock_);
-  syncer::ModelTypeSet newly_added_types;
-  for (syncer::ModelTypeSet::Iterator it = filtered_types_to_add.First();
-       it.Good(); it.Inc()) {
-    // Add a newly specified data type as syncer::GROUP_PASSIVE into the
+  ModelTypeSet newly_added_types;
+  for (ModelTypeSet::Iterator it = filtered_types_to_add.First(); it.Good();
+       it.Inc()) {
+    // Add a newly specified data type corresponding initial group into the
     // routing_info, if it does not already exist.
     if (routing_info_.count(it.Get()) == 0) {
       routing_info_[it.Get()] = GetInitialGroupForType(it.Get());
       newly_added_types.Put(it.Get());
     }
   }
-  for (syncer::ModelTypeSet::Iterator it = types_to_remove.First(); it.Good();
+  for (ModelTypeSet::Iterator it = types_to_remove.First(); it.Good();
        it.Inc()) {
     routing_info_.erase(it.Get());
   }
 
   // TODO(akalin): Use SVLOG/SLOG if we add any more logging.
-  DVLOG(1) << name_ << ": Adding types "
-           << syncer::ModelTypeSetToString(types_to_add)
+  DVLOG(1) << name_ << ": Adding types " << ModelTypeSetToString(types_to_add)
            << " (with newly-added types "
-           << syncer::ModelTypeSetToString(newly_added_types)
-           << ") and removing types "
-           << syncer::ModelTypeSetToString(types_to_remove)
+           << ModelTypeSetToString(newly_added_types) << ") and removing types "
+           << ModelTypeSetToString(types_to_remove)
            << " to get new routing info "
-           << syncer::ModelSafeRoutingInfoToString(routing_info_);
+           << ModelSafeRoutingInfoToString(routing_info_);
   last_configured_types_ = syncer::GetRoutingInfoTypes(routing_info_);
 
   return newly_added_types;
 }
 
-syncer::ModelTypeSet SyncBackendRegistrar::GetLastConfiguredTypes() const {
+ModelTypeSet SyncBackendRegistrar::GetLastConfiguredTypes() const {
   return last_configured_types_;
 }
 
@@ -162,15 +178,15 @@ void SyncBackendRegistrar::RequestWorkerStopOnUIThread() {
 }
 
 void SyncBackendRegistrar::ActivateDataType(
-    syncer::ModelType type,
-    syncer::ModelSafeGroup group,
+    ModelType type,
+    ModelSafeGroup group,
     sync_driver::ChangeProcessor* change_processor,
     syncer::UserShare* user_share) {
-  DVLOG(1) << "Activate: " << syncer::ModelTypeToString(type);
+  DVLOG(1) << "Activate: " << ModelTypeToString(type);
 
   base::AutoLock lock(lock_);
   // Ensure that the given data type is in the PASSIVE group.
-  syncer::ModelSafeRoutingInfo::iterator i = routing_info_.find(type);
+  ModelSafeRoutingInfo::iterator i = routing_info_.find(type);
   DCHECK(i != routing_info_.end());
   DCHECK_EQ(i->second, syncer::GROUP_PASSIVE);
   routing_info_[type] = group;
@@ -185,8 +201,8 @@ void SyncBackendRegistrar::ActivateDataType(
   DCHECK(GetProcessorUnsafe(type));
 }
 
-void SyncBackendRegistrar::DeactivateDataType(syncer::ModelType type) {
-  DVLOG(1) << "Deactivate: " << syncer::ModelTypeToString(type);
+void SyncBackendRegistrar::DeactivateDataType(ModelType type) {
+  DVLOG(1) << "Deactivate: " << ModelTypeToString(type);
 
   DCHECK(ui_thread_->BelongsToCurrentThread() || IsControlType(type));
   base::AutoLock lock(lock_);
@@ -196,13 +212,12 @@ void SyncBackendRegistrar::DeactivateDataType(syncer::ModelType type) {
   DCHECK(!GetProcessorUnsafe(type));
 }
 
-bool SyncBackendRegistrar::IsTypeActivatedForTest(
-    syncer::ModelType type) const {
+bool SyncBackendRegistrar::IsTypeActivatedForTest(ModelType type) const {
   return GetProcessor(type) != NULL;
 }
 
 void SyncBackendRegistrar::OnChangesApplied(
-    syncer::ModelType model_type,
+    ModelType model_type,
     int64_t model_version,
     const syncer::BaseTransaction* trans,
     const syncer::ImmutableChangeRecordList& changes) {
@@ -213,7 +228,7 @@ void SyncBackendRegistrar::OnChangesApplied(
   processor->ApplyChangesFromSyncModel(trans, model_version, changes);
 }
 
-void SyncBackendRegistrar::OnChangesComplete(syncer::ModelType model_type) {
+void SyncBackendRegistrar::OnChangesComplete(ModelType model_type) {
   sync_driver::ChangeProcessor* processor = GetProcessor(model_type);
   if (!processor)
     return;
@@ -234,15 +249,14 @@ void SyncBackendRegistrar::GetWorkers(
   }
 }
 
-void SyncBackendRegistrar::GetModelSafeRoutingInfo(
-    syncer::ModelSafeRoutingInfo* out) {
+void SyncBackendRegistrar::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {
   base::AutoLock lock(lock_);
-  syncer::ModelSafeRoutingInfo copy(routing_info_);
+  ModelSafeRoutingInfo copy(routing_info_);
   out->swap(copy);
 }
 
 sync_driver::ChangeProcessor* SyncBackendRegistrar::GetProcessor(
-    syncer::ModelType type) const {
+    ModelType type) const {
   base::AutoLock lock(lock_);
   sync_driver::ChangeProcessor* processor = GetProcessorUnsafe(type);
   if (!processor)
@@ -255,10 +269,10 @@ sync_driver::ChangeProcessor* SyncBackendRegistrar::GetProcessor(
 }
 
 sync_driver::ChangeProcessor* SyncBackendRegistrar::GetProcessorUnsafe(
-    syncer::ModelType type) const {
+    ModelType type) const {
   lock_.AssertAcquired();
-  std::map<syncer::ModelType, sync_driver::ChangeProcessor*>::const_iterator
-      it = processors_.find(type);
+  std::map<ModelType, sync_driver::ChangeProcessor*>::const_iterator it =
+      processors_.find(type);
 
   // Until model association happens for a datatype, it will not
   // appear in the processors list.  During this time, it is OK to
@@ -273,15 +287,14 @@ sync_driver::ChangeProcessor* SyncBackendRegistrar::GetProcessorUnsafe(
 }
 
 bool SyncBackendRegistrar::IsCurrentThreadSafeForModel(
-    syncer::ModelType model_type) const {
+    ModelType model_type) const {
   lock_.AssertAcquired();
   return IsOnThreadForGroup(model_type,
                             GetGroupForModelType(model_type, routing_info_));
 }
 
-bool SyncBackendRegistrar::IsOnThreadForGroup(
-    syncer::ModelType type,
-    syncer::ModelSafeGroup group) const {
+bool SyncBackendRegistrar::IsOnThreadForGroup(ModelType type,
+                                              ModelSafeGroup group) const {
   switch (group) {
     case syncer::GROUP_PASSIVE:
       return IsControlType(type);
@@ -309,11 +322,11 @@ SyncBackendRegistrar::~SyncBackendRegistrar() {
   DCHECK(workers_.empty());
 }
 
-void SyncBackendRegistrar::OnWorkerLoopDestroyed(syncer::ModelSafeGroup group) {
+void SyncBackendRegistrar::OnWorkerLoopDestroyed(ModelSafeGroup group) {
   RemoveWorker(group);
 }
 
-void SyncBackendRegistrar::MaybeAddWorker(syncer::ModelSafeGroup group) {
+void SyncBackendRegistrar::MaybeAddWorker(ModelSafeGroup group) {
   const scoped_refptr<syncer::ModelSafeWorker> worker =
       sync_client_->CreateModelWorkerForGroup(group, this);
   if (worker) {
@@ -323,12 +336,11 @@ void SyncBackendRegistrar::MaybeAddWorker(syncer::ModelSafeGroup group) {
   }
 }
 
-void SyncBackendRegistrar::OnWorkerUnregistrationDone(
-    syncer::ModelSafeGroup group) {
+void SyncBackendRegistrar::OnWorkerUnregistrationDone(ModelSafeGroup group) {
   RemoveWorker(group);
 }
 
-void SyncBackendRegistrar::RemoveWorker(syncer::ModelSafeGroup group) {
+void SyncBackendRegistrar::RemoveWorker(ModelSafeGroup group) {
   DVLOG(1) << "Remove " << ModelSafeGroupToString(group) << " worker.";
 
   bool last_worker = false;
@@ -370,12 +382,10 @@ base::Thread* SyncBackendRegistrar::sync_thread() {
   return sync_thread_.get();
 }
 
-syncer::ModelSafeGroup SyncBackendRegistrar::GetInitialGroupForType(
-    syncer::ModelType type) const {
-  if (non_blocking_types_.Has(type))
-    return syncer::GROUP_NON_BLOCKING;
-  else
-    return syncer::GROUP_PASSIVE;
+ModelSafeGroup SyncBackendRegistrar::GetInitialGroupForType(
+    ModelType type) const {
+  return non_blocking_types_.Has(type) ? syncer::GROUP_NON_BLOCKING
+                                       : syncer::GROUP_PASSIVE;
 }
 
 }  // namespace browser_sync