[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/sync/profile_sync_service.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 "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 713 matching lines @@
   is_auth_in_progress_ = false;
   backend_initialized_ = false;
   // NULL if we're called from Shutdown().
   if (invalidator_registrar_)
     UpdateInvalidatorRegistrarState();
   cached_passphrase_.clear();
   encryption_pending_ = false;
   encrypt_everything_ = false;
   encrypted_types_ = syncer::SyncEncryptionHandler::SensitiveTypes();
   passphrase_required_reason_ = syncer::REASON_PASSPHRASE_NOT_REQUIRED;
+  start_up_time_ = base::Time();
   // Revert to "no auth error".
   if (last_auth_error_.state() != GoogleServiceAuthError::NONE)
     UpdateAuthErrorState(GoogleServiceAuthError::AuthErrorNone());
 
   if (sync_global_error_) {
     GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(
         sync_global_error_.get());
     RemoveObserver(sync_global_error_.get());
     sync_global_error_.reset(NULL);
   }
@@ skipping 331 matching lines @@
 
 void ProfileSyncService::OnConnectionStatusChange(
     syncer::ConnectionStatus status) {
   const GoogleServiceAuthError auth_error =
       ConnectionStatusToAuthError(status);
   DVLOG(1) << "Connection status change: " << auth_error.ToString();
   UpdateAuthErrorState(auth_error);
 }
 
 void ProfileSyncService::OnStopSyncingPermanently() {
-  UpdateAuthErrorState(AuthError(AuthError::SERVICE_UNAVAILABLE));
   sync_prefs_.SetStartSuppressed(true);
   DisableForUser();
-
-  // Signout doesn't exist as a concept on Chrome OS. It currently does
-  // on other auto-start platforms (like Android, though we should probably
-  // use SigninManagerBase there as well), but we don't want to sign the
-  // user out on auto-start platforms if sync was disabled.
-  // TODO(tim): Platform specific refactoring here is bug 237866.
-#if !defined(OS_CHROMEOS)
-  SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
-
-  if (!auto_start_enabled_)  // Skip signout on ChromeOS/Android.
-    signin->SignOut();
-#endif
 }
 
 void ProfileSyncService::OnPassphraseRequired(
     syncer::PassphraseRequiredReason reason,
     const sync_pb::EncryptedData& pending_keys) {
   DCHECK(backend_.get());
   DCHECK(backend_->IsNigoriEnabled());
 
   // TODO(lipalani) : add this check to other locations as well.
   if (HasUnrecoverableError()) {
@@ skipping 104 matching lines @@
         expect_sync_configuration_aborted_ = true;
       }
       // Trigger an unrecoverable error to stop syncing.
       OnInternalUnrecoverableError(FROM_HERE,
                                    last_actionable_error_.error_description,
                                    true,
                                    ERROR_REASON_ACTIONABLE_ERROR);
       break;
     case syncer::DISABLE_SYNC_ON_CLIENT:
       OnStopSyncingPermanently();
+      // TODO(rsimha): Re-evaluate whether to also sign out the user here after
+      // a dashboard clear. See http://crbug.com/240436.
       break;
     default:
       NOTREACHED();
   }
   NotifyObservers();
 }
 
 void ProfileSyncService::OnConfigureBlocked() {
   NotifyObservers();
 }
@@ skipping 686 matching lines @@
           !successful->password.empty()) {
         cached_passphrase_ = successful->password;
         // Try to consume the passphrase we just cached. If the sync backend
         // is not running yet, the passphrase will remain cached until the
         // backend starts up.
         ConsumeCachedPassphraseIfPossible();
       }
 #if defined(OS_CHROMEOS)
       RefreshSpareBootstrapToken(successful->password);
 #endif
-      if (!sync_initialized() ||
-          GetAuthError().state() != AuthError::NONE) {
+      if (!sync_initialized() && GetAuthError().state() != AuthError::NONE) {

[Andrew T Wilson (Slow), 2013/05/15 15:01:24]

This doesn't seem like the right change - we want is_auth_in_progress to be true
during initial signin, but with this change it won't be (because GetAuthError()
starts out as NONE).

Also, the comment below seems wrong. If the user signs in and re-enables sync
while last_auth_error != NONE, auth is *not* complete until
UpdateAuthErrorState() is called (we want to wait to see if the backend is able
to contact the sync server).

[Raghu Simha, 2013/05/16 02:13:20]

I had orignally changed this logic to handle the case where the user hits an
auth error while re-setting up sync, and the backend was already initialized by
the time the user finished entering their newest gaia passphrase.
SigninTracker::GetSigninState was silently dropping the OnStateChanged callback
resulting from the GOOGLE_SIGNIN_SUCCESSFUL notification in that case, and was
never getting to the SIGNIN_COMPLETE state.

Now that I've switched to the flow where the user sees a link on the settings
page during an auth error, this change is no longer required. I've reverted this
section to what it was originally. I've run this by Rachel, and she's fine with
the string change.

         // Track the fact that we're still waiting for auth to complete.
+        // Note: We mustn't set |is_auth_in_progress_| to true when the backend
+        // is already initialized because it's possible to end up here when a
+        // signed in user with sync disabled changes their gaia password, and
+        // then re-auths while trying to re-enable sync. In such a case, auth
+        // is complete when we receive NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL.
         is_auth_in_progress_ = true;
       }
       break;
     }
     case chrome::NOTIFICATION_TOKEN_REQUEST_FAILED: {
       const TokenService::TokenRequestFailedDetails& token_details =
           *(content::Details<const TokenService::TokenRequestFailedDetails>(
               details).ptr());
       if (IsTokenServiceRelevant(token_details.service()) &&
           !IsSyncTokenAvailable()) {
@@ skipping 170 matching lines @@
   // See http://stackoverflow.com/questions/6224121/is-new-this-myclass-undefined-behaviour-after-directly-calling-the-destru.
   ProfileSyncService* old_this = this;
   this->~ProfileSyncService();
   new(old_this) ProfileSyncService(
       new ProfileSyncComponentsFactoryImpl(profile,
                                            CommandLine::ForCurrentProcess()),
       profile,
       signin,
       behavior);
 }