sync: Fix Nigori download sequencing.

Only drop SetPassphrase requests if we hadn't requested NIGORI nodes
in routing info.  The reasoning is that NIGORI will be on-by-default always, meaning you cannot disable it, so once it is present it will remain, and it only ever gets added during init.  Hence, if sync setup has completed, and NIGORI is in the routing info, we are good to go.

Also make sure we remove passphrase notification observers when shutting down to avoid re-registration problems.

BUG=58098, 48702
TEST=ProfileSyncPasswordsTest, point chrome at passwords-enabled server, run with --enable-sync-passwords, and successfully sync passwords.

Review URL: http://codereview.chromium.org/3569022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61811 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 85 insertions, 43 deletions

diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index f91d0f8..dabb926 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -1863,7 +1863,7 @@ void SyncManager::SyncInternal::HandleChannelEvent(const SyncerEvent& event) {
         sync_api::ReadTransaction trans(GetUserShare());
         sync_api::ReadNode node(&trans);
         if (!node.InitByTagLookup(kNigoriTag)) {
-          NOTREACHED();
+          DCHECK(!event.snapshot->is_share_usable);
           return;
         }
         const sync_pb::NigoriSpecifics& nigori = node.GetNigoriSpecifics();
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 74a8c47..05cbd7b 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -22,6 +22,8 @@
 #include "chrome/browser/sync/glue/http_bridge.h"
 #include "chrome/browser/sync/glue/password_model_worker.h"
 #include "chrome/browser/sync/sessions/session_state.h"
+// TODO(tim): Remove this! We should have a syncapi pass-thru instead.
+#include "chrome/browser/sync/syncable/directory_manager.h"  // Cryptographer.
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/net/gaia/gaia_constants.h"
@@ -115,12 +117,13 @@ void SyncBackendHost::Initialize(
     registrar_.routing_info[(*it)] = GROUP_PASSIVE;
   }
 
-  // TODO(tim): This should be encryption-specific instead of passwords
-  // specific.  For now we have to do this to avoid NIGORI node lookups when
-  // we haven't downloaded that node.
-  if (profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords)) {
+  // TODO(tim): Remove this special case once NIGORI is populated by
+  // default.  We piggy back off of the passwords flag for now to not
+  // require both encryption and passwords flags.
+  bool enable_encryption = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableSyncPasswords) || types.count(syncable::PASSWORDS);
+  if (enable_encryption)
     registrar_.routing_info[syncable::NIGORI] = GROUP_PASSIVE;
-  }
 
   InitCore(Core::DoInitializeOptions(
       sync_service_url,
@@ -146,6 +149,20 @@ std::string SyncBackendHost::RestoreEncryptionBootstrapToken() {
   return token;
 }
 
+bool SyncBackendHost::IsNigoriEnabled() const {
+  AutoLock lock(registrar_lock_);
+  // Note that NIGORI is only ever added/removed from routing_info once,
+  // during initialization / first configuration, so there is no real 'race'
+  // possible here or possibility of stale return value.
+  return registrar_.routing_info.find(syncable::NIGORI) !=
+      registrar_.routing_info.end();
+}
+
+bool SyncBackendHost::IsCryptographerReady() const {
+  return syncapi_initialized_ &&
+      GetUserShareHandle()->dir_manager->cryptographer()->is_ready();
+}
+
 sync_api::HttpPostProviderFactory* SyncBackendHost::MakeHttpBridgeFactory(
     URLRequestContextGetter* getter) {
   return new HttpBridgeFactory(getter);
@@ -170,6 +187,12 @@ void SyncBackendHost::StartSyncingWithServer() {
 }
 
 void SyncBackendHost::SetPassphrase(const std::string& passphrase) {
+  if (!IsNigoriEnabled()) {
+    LOG(WARNING) << "Silently dropping SetPassphrase request.";
+    return;
+  }
+
+  // If encryption is enabled and we've got a SetPassphrase
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoSetPassphrase,
                         passphrase));
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index d18bf26d..26d060a 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -191,6 +191,14 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
   // ONLY CALL THIS IF OnInitializationComplete was called!
   bool HasUnsyncedItems() const;
 
+  // Whether or not we are syncing encryption keys.
+  bool IsNigoriEnabled() const;
+
+  // True if the cryptographer has any keys available to attempt decryption.
+  // Could mean we've downloaded and loaded Nigori objects, or we bootstrapped
+  // using a token previously received.
+  bool IsCryptographerReady() const;
+
  protected:
   // The real guts of SyncBackendHost, to keep the public client API clean.
   class Core : public base::RefCountedThreadSafe<SyncBackendHost::Core>,
@@ -463,7 +471,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
   // pointer value", and then invoke methods), because lifetimes are managed on
   // the UI thread.  Of course, this comment only applies to ModelSafeWorker
   // impls that are themselves thread-safe, such as UIModelWorker.
-  Lock registrar_lock_;
+  mutable Lock registrar_lock_;
 
   // The frontend which we serve (and are owned by).
   SyncFrontend* frontend_;
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 9788f2c..01f03bb 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -23,16 +23,13 @@
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/net/gaia/token_service.h"
-#include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
 #include "chrome/browser/sync/glue/data_type_manager.h"
 #include "chrome/browser/sync/glue/session_data_type_controller.h"
 #include "chrome/browser/sync/profile_sync_factory.h"
 #include "chrome/browser/sync/signin_manager.h"
-#include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/token_migrator.h"
-#include "chrome/browser/sync/util/user_settings.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/net/gaia/gaia_constants.h"
 #include "chrome/common/notification_details.h"
@@ -73,6 +70,7 @@ ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
       unrecoverable_error_detected_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(scoped_runnable_method_factory_(this)),
       token_migrator_(NULL),
+      observed_passphrase_required_(false),
       clear_server_data_state_(CLEAR_NOT_STARTED) {
   DCHECK(factory);
   DCHECK(profile);
@@ -307,7 +305,7 @@ void ProfileSyncService::RegisterPreferences() {
 #endif
 
   pref_service->RegisterBooleanPref(prefs::kSyncBookmarks, true);
-  pref_service->RegisterBooleanPref(prefs::kSyncPasswords, false);
+  pref_service->RegisterBooleanPref(prefs::kSyncPasswords, enable_by_default);
   pref_service->RegisterBooleanPref(prefs::kSyncPreferences, enable_by_default);
   pref_service->RegisterBooleanPref(prefs::kSyncAutofill, enable_by_default);
   pref_service->RegisterBooleanPref(prefs::kSyncThemes, enable_by_default);
@@ -408,26 +406,40 @@ void ProfileSyncService::StartUp() {
 
 void ProfileSyncService::Shutdown(bool sync_disabled) {
   // Stop all data type controllers, if needed.
-  if (data_type_manager_.get() &&
-      data_type_manager_->state() != DataTypeManager::STOPPED) {
-    data_type_manager_->Stop();
-  }
+  if (data_type_manager_.get()) {
+    if (data_type_manager_->state() != DataTypeManager::STOPPED) {
+      data_type_manager_->Stop();
+    }
 
-  data_type_manager_.reset();
+    registrar_.Remove(this,
+                      NotificationType::SYNC_CONFIGURE_START,
+                      Source<DataTypeManager>(data_type_manager_.get()));
+    registrar_.Remove(this,
+                      NotificationType::SYNC_CONFIGURE_DONE,
+                      Source<DataTypeManager>(data_type_manager_.get()));
+    data_type_manager_.reset();
+  }
 
   // Move aside the backend so nobody else tries to use it while we are
   // shutting it down.
   scoped_ptr<SyncBackendHost> doomed_backend(backend_.release());
-
-  if (doomed_backend.get())
+  if (doomed_backend.get()) {
     doomed_backend->Shutdown(sync_disabled);
 
-  doomed_backend.reset();
+    registrar_.Remove(this,
+                   NotificationType::SYNC_PASSPHRASE_REQUIRED,
+                   Source<SyncBackendHost>(doomed_backend.get()));
+    registrar_.Remove(this,
+                   NotificationType::SYNC_PASSPHRASE_ACCEPTED,
+                   Source<SyncBackendHost>(doomed_backend.get()));
 
+    doomed_backend.reset();
+  }
 
   // Clear various flags.
   is_auth_in_progress_ = false;
   backend_initialized_ = false;
+  observed_passphrase_required_ = false;
   last_attempted_user_email_.clear();
 }
 
@@ -819,24 +831,7 @@ void ProfileSyncService::GetRegisteredDataTypes(
 }
 
 bool ProfileSyncService::IsCryptographerReady() const {
-  return backend_.get() && backend_initialized_ &&
-      backend_->GetUserShareHandle()->dir_manager->cryptographer()->is_ready();
-}
-
-void ProfileSyncService::SetPassphrase(const std::string& passphrase) {
-  // TODO(tim): This should be encryption-specific instead of passwords
-  // specific.  For now we have to do this to avoid NIGORI node lookups when
-  // we haven't downloaded that node.
-  if (!profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords)) {
-    LOG(WARNING) << "Silently dropping SetPassphrase request.";
-    return;
-  }
-
-  if (!sync_initialized()) {
-    cached_passphrase_ = passphrase;
-  } else {
-    backend_->SetPassphrase(passphrase);
-  }
+  return backend_.get() && backend_->IsCryptographerReady();
 }
 
 void ProfileSyncService::ConfigureDataTypeManager() {
@@ -877,6 +872,14 @@ void ProfileSyncService::DeactivateDataType(
     backend_->DeactivateDataType(data_type_controller, change_processor);
 }
 
+void ProfileSyncService::SetPassphrase(const std::string& passphrase) {
+  if (ShouldPushChanges() || observed_passphrase_required_) {
+    backend_->SetPassphrase(passphrase);
+  } else {
+    cached_passphrase_ = passphrase;
+  }
+}
+
 void ProfileSyncService::Observe(NotificationType type,
                                  const NotificationSource& source,
                                  const NotificationDetails& details) {
@@ -913,6 +916,9 @@ void ProfileSyncService::Observe(NotificationType type,
     }
     case NotificationType::SYNC_PASSPHRASE_REQUIRED: {
       DCHECK(backend_.get());
+      DCHECK(backend_->IsNigoriEnabled());
+      observed_passphrase_required_ = true;
+
       if (!cached_passphrase_.empty()) {
         SetPassphrase(cached_passphrase_);
         cached_passphrase_.clear();
@@ -955,8 +961,8 @@ void ProfileSyncService::Observe(NotificationType type,
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_SUCCESSFUL: {
-      if (!profile_->GetPrefs()->GetBoolean(
-          prefs::kSyncUsingSecondaryPassphrase)) {
+      PrefService* prefs = profile_->GetPrefs();
+      if (!prefs->GetBoolean(prefs::kSyncUsingSecondaryPassphrase)) {
         const GoogleServiceSigninSuccessDetails* successful =
             (Details<const GoogleServiceSigninSuccessDetails>(details).ptr());
         SetPassphrase(successful->password);
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index dc3e67d..f0a3db2 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -328,9 +328,10 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // for sensitive data types.
   virtual bool IsCryptographerReady() const;
 
-  // Sets the Cryptographer's passphrase. This will check asynchronously whether
-  // the passphrase is valid and notify ProfileSyncServiceObservers via the
-  // NotificationService when the outcome is known.
+  // Sets the Cryptographer's passphrase, or caches it until that is possible.

+  // This will check asynchronously whether the passphrase is valid and notify

+  // ProfileSyncServiceObservers via the NotificationService when the outcome
+  // is known.
   virtual void SetPassphrase(const std::string& passphrase);
 
   // Returns whether processing changes is allowed.  Check this before doing
@@ -484,6 +485,10 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // credentials were provided.
   std::string cached_passphrase_;
 
+  // Whether we have seen a SYNC_PASSPHRASE_REQUIRED since initializing the
+  // backend, telling us that it is safe to send a passphrase down ASAP.
+  bool observed_passphrase_required_;
+
   // Keep track of where we are in a server clear operation
   ClearServerDataState clear_server_data_state_;
 
diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc
index d0598c5..a355c60 100644
--- a/chrome/browser/sync/profile_sync_service_password_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc
@@ -144,7 +144,7 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest {
       service_.reset(new TestProfileSyncService(&factory_, &profile_,
                                                 "test_user", false, root_task));
       service_->RegisterPreferences();
-      profile_.GetPrefs()->SetBoolean(prefs::kSyncPasswords, true); 
+      profile_.GetPrefs()->SetBoolean(prefs::kSyncPasswords, true);
       service_->set_num_expected_resumes(num_resume_expectations);
       service_->set_num_expected_pauses(num_pause_expectations);
       PasswordDataTypeController* data_type_controller =