ProfileSyncService: Remove CanEngineStart

CanEngineStart specified the required conditions for creating the
SyncEngine object: CanSyncStart() plus a refresh token being available.
The refresh token condition was there because creating the engine will
trigger a request for an access token. Previously, that request would
fail in unhelpful ways if the refresh token wasn't loaded yet.
Now however, PrimaryAccountAccessTokenFetcher handles that situation
just fine, it'll simply wait for the refresh token.

So this CL removes the refresh token condition from ProfileSyncService.

Bug: 825190, 842697
Change-Id: Ica57945f93efcdf8597c1bdfdbda19a0b879e6d9
Reviewed-on: https://chromium-review.googlesource.com/997796
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562000}

chrome/browser/chromeos/profiles/profile_helper.cc

@@ -121,6 +121,9 @@ base::FilePath ProfileHelper::GetProfilePathByUserIdHash(
 // static
 base::FilePath ProfileHelper::GetSigninProfileDir() {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
+  // profile_manager can be null in unit tests.
+  if (!profile_manager)
+    return base::FilePath();
   base::FilePath user_data_dir = profile_manager->user_data_dir();
   return user_data_dir.AppendASCII(chrome::kInitialProfile);
 }

chrome/browser/ui/webui/sync_internals_browsertest.js

@@ -267,19 +267,31 @@ TEST_F('SyncInternalsWebUITest', 'LoadPastedAboutInfo', function() {
 });
 
 TEST_F('SyncInternalsWebUITest', 'NetworkEventsTest', function() {
-  networkEvent1 = new Event('onProtocolEvent');
+  let networkEvent1 = new Event('onProtocolEvent');
   networkEvent1.details = NETWORK_EVENT_DETAILS_1;
-  networkEvent2 = new Event('onProtocolEvent');
+  let networkEvent2 = new Event('onProtocolEvent');
   networkEvent2.details = NETWORK_EVENT_DETAILS_2;
 
   chrome.sync.events.dispatchEvent(networkEvent1);
   chrome.sync.events.dispatchEvent(networkEvent2);
 
-  expectEquals(2, $('traffic-event-container').children.length);
+  // Make sure that both events arrived.
+  let eventCount = $('traffic-event-container').children.length;
+  assertGE(eventCount, 2);
+
+  // Check that the event details are displayed.
+  let displayedEvent1 = $('traffic-event-container').children[eventCount - 2];
+  let displayedEvent2 = $('traffic-event-container').children[eventCount - 1];
+  expectTrue(
+      displayedEvent1.innerHTML.includes(NETWORK_EVENT_DETAILS_1.details));
+  expectTrue(displayedEvent1.innerHTML.includes(NETWORK_EVENT_DETAILS_1.type));
+  expectTrue(
+      displayedEvent2.innerHTML.includes(NETWORK_EVENT_DETAILS_2.details));
+  expectTrue(displayedEvent2.innerHTML.includes(NETWORK_EVENT_DETAILS_2.type));
 
   // Test that repeated events are not re-displayed.
   chrome.sync.events.dispatchEvent(networkEvent1);
-  expectEquals(2, $('traffic-event-container').children.length);
+  expectEquals(eventCount, $('traffic-event-container').children.length);
 });
 
 TEST_F('SyncInternalsWebUITest', 'SearchTabDoesntChangeOnItemSelect',
@@ -373,14 +385,15 @@ TEST_F('SyncInternalsWebUITest', 'EventLogTest', function() {
 
   // Verify that it is displayed in the events log.
   var syncEventsTable = $('sync-events');
-  var firstRow = syncEventsTable.children[0];
+  assertGE(syncEventsTable.children.length, 1);
+  var lastRow = syncEventsTable.children[syncEventsTable.children.length - 1];
 
   // Makes some assumptions about column ordering.  We'll need re-think this if
   // it turns out to be a maintenance burden.
-  assertEquals(4, firstRow.children.length);
-  var detailsText = firstRow.children[0].textContent;
-  var submoduleName = firstRow.children[1].textContent;
-  var eventName = firstRow.children[2].textContent;
+  assertEquals(4, lastRow.children.length);
+  var detailsText = lastRow.children[0].textContent;
+  var submoduleName = lastRow.children[1].textContent;
+  var eventName = lastRow.children[2].textContent;
 
   expectGE(submoduleName.indexOf('manager'), 0,
       'submoduleName=' + submoduleName);

components/browser_sync/profile_sync_service.cc

@@ -220,7 +220,7 @@ void ProfileSyncService::Initialize() {
   // against the controller impl changing to post tasks.
   startup_controller_ = std::make_unique<syncer::StartupController>(
       &sync_prefs_,
-      base::BindRepeating(&ProfileSyncService::CanEngineStart,
+      base::BindRepeating(&ProfileSyncService::CanSyncStart,
                           base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::StartUpSlowEngineComponents,
                           weak_factory_.GetWeakPtr()));
@@ -518,12 +518,11 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(syncer::ModelType type) {
 }
 
 void ProfileSyncService::StartUpSlowEngineComponents() {
-  invalidation::InvalidationService* invalidator =
-      sync_client_->GetInvalidationService();
+  DCHECK(CanSyncStart());
 
   engine_ = sync_client_->GetSyncApiComponentFactory()->CreateSyncEngine(
-      debug_identifier_, invalidator, sync_prefs_.AsWeakPtr(),
-      FormatSyncDataPath(base_directory_));
+      debug_identifier_, sync_client_->GetInvalidationService(),
+      sync_prefs_.AsWeakPtr(), FormatSyncDataPath(base_directory_));
 
   // Clear any old errors the first time sync starts.
   if (!IsFirstSetupComplete())
@@ -619,6 +618,10 @@ void ProfileSyncService::OnRefreshTokenAvailable() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (HasSyncingEngine()) {
+    // TODO(treib): This call shouldn't be necessary: Either this is the initial
+    // refresh token, in which case a request is already pending, or the refresh
+    // token changed, which the SyncAuthManager should detect and handle
+    // internally (but it doesn't yet).
     auth_manager_->RequestAccessToken();
   } else {
     startup_controller_->TryStart();
@@ -1289,12 +1292,6 @@ bool ProfileSyncService::IsSignedIn() const {
   return !GetAuthenticatedAccountInfo().account_id.empty();
 }
 
-bool ProfileSyncService::CanEngineStart() const {
-  if (IsLocalSyncEnabled())
-    return true;
-  return CanSyncStart() && auth_manager_->RefreshTokenIsAvailable();
-}
-
 bool ProfileSyncService::IsEngineInitialized() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return engine_initialized_;
@@ -2087,7 +2084,29 @@ syncer::SyncTokenStatus ProfileSyncService::GetSyncTokenStatus() const {
 void ProfileSyncService::OverrideNetworkResourcesForTest(
     std::unique_ptr<syncer::NetworkResources> network_resources) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // If the engine has already been created, then it holds a pointer to the
+  // previous |network_resources_| which will become invalid. In that case, shut
+  // down and recreate the engine, so that it gets the correct (overridden)
+  // NetworkResources.
+  // This is a horrible hack; the proper fix would be to inject the
+  // NetworkResources in the ctor instead of adding them retroactively.
+  bool restart = false;
+  if (engine_) {
+    RequestStop(KEEP_DATA);
+    restart = true;
+  }
+  DCHECK(!engine_);
+
+  // If a previous request (with the wrong network resources) already failed,
+  // the next one would be backed off, which breaks tests. So reset the backoff.
+  auth_manager_->ResetRequestAccessTokenBackoffForTest();
+
   network_resources_ = std::move(network_resources);
+
+  if (restart) {
+    RequestStart();
+    DCHECK(engine_);
+  }
 }
 
 bool ProfileSyncService::HasSyncingEngine() const {

components/browser_sync/profile_sync_service.h

@@ -647,13 +647,6 @@ class ProfileSyncService : public syncer::SyncService,
   // Whether sync has been authenticated with an account ID.
   bool IsSignedIn() const;
 
-  // The engine can only start if sync can start and has an auth token. This is
-  // different fron CanSyncStart because it represents whether the engine can
-  // be started at this moment, whereas CanSyncStart represents whether sync can
-  // conceptually start without further user action (acquiring a token is an
-  // automatic process).
-  bool CanEngineStart() const;
-
   // True if a syncing engine exists.
   bool HasSyncingEngine() const;
 

components/browser_sync/profile_sync_service_mock.h

@@ -88,6 +88,8 @@ class ProfileSyncServiceMock : public ProfileSyncService {
   MOCK_METHOD0(GetOpenTabsUIDelegateMock, sync_sessions::OpenTabsUIDelegate*());
   sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate() override;
 
+  MOCK_METHOD0(StartUpSlowEngineComponents, void());
+
   // DataTypeManagerObserver mocks.
   MOCK_METHOD1(OnConfigureDone,
                void(const syncer::DataTypeManager::ConfigureResult&));

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -279,12 +279,12 @@ TEST_F(ProfileSyncServiceStartupTest, DISABLED_StartInvalidCredentials) {
 }
 
 TEST_F(ProfileSyncServiceStartupCrosTest, StartCrosNoCredentials) {
-  EXPECT_CALL(*component_factory_, CreateDataTypeManager(_, _, _, _, _, _))
-      .Times(0);
-  EXPECT_CALL(*component_factory_, CreateSyncEngine(_, _, _, _)).Times(0);
   pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
   EXPECT_CALL(observer_, OnStateChanged(_)).Times(AnyNumber());
 
+  SetUpSyncEngine();
+  SetUpDataTypeManager();
+
   sync_service_->Initialize();
   // Sync should not start because there are no tokens yet.
   EXPECT_FALSE(sync_service_->IsSyncActive());

components/browser_sync/sync_auth_manager.cc

@@ -88,12 +88,6 @@ AccountInfo SyncAuthManager::GetAuthenticatedAccountInfo() const {
                            : AccountInfo();
 }
 
-bool SyncAuthManager::RefreshTokenIsAvailable() const {
-  std::string account_id = GetAuthenticatedAccountInfo().account_id;
-  return !account_id.empty() &&
-         token_service_->RefreshTokenIsAvailable(account_id);
-}
-
 const syncer::SyncTokenStatus& SyncAuthManager::GetSyncTokenStatus() const {
   return token_status_;
 }
@@ -283,6 +277,10 @@ bool SyncAuthManager::IsRetryingAccessTokenFetchForTest() const {
   return request_access_token_retry_timer_.IsRunning();
 }
 
+void SyncAuthManager::ResetRequestAccessTokenBackoffForTest() {
+  request_access_token_backoff_.Reset();
+}
+
 void SyncAuthManager::RequestAccessToken() {
   // Only one active request at a time.
   if (ongoing_access_token_fetch_) {
@@ -311,7 +309,8 @@ void SyncAuthManager::RequestAccessToken() {
           kSyncOAuthConsumerName, oauth2_scopes,
           base::BindOnce(&SyncAuthManager::AccessTokenFetched,
                          base::Unretained(this)),
-          identity::PrimaryAccountAccessTokenFetcher::Mode::kImmediate);
+          identity::PrimaryAccountAccessTokenFetcher::Mode::
+              kWaitUntilAvailable);
 }
 
 void SyncAuthManager::AccessTokenFetched(const GoogleServiceAuthError& error,

components/browser_sync/sync_auth_manager.h

@@ -56,11 +56,6 @@ class SyncAuthManager : public identity::IdentityManager::Observer,
   // an empty AccountInfo if there isn't one.
   AccountInfo GetAuthenticatedAccountInfo() const;
 
-  // Returns whether a refresh token is available for the primary account.
-  // TODO(crbug.com/842697, crbug.com/825190): ProfileSyncService shouldn't have
-  // to care about the refresh token state.
-  bool RefreshTokenIsAvailable() const;
-
   const GoogleServiceAuthError& GetLastAuthError() const {
     return last_auth_error_;
   }
@@ -97,8 +92,9 @@ class SyncAuthManager : public identity::IdentityManager::Observer,
   void OnRefreshTokenRevoked(const std::string& account_id) override;
   void OnRefreshTokensLoaded() override;
 
-  // Test-only method for inspecting internal state.
+  // Test-only methods for inspecting/modifying internal state.
   bool IsRetryingAccessTokenFetchForTest() const;
+  void ResetRequestAccessTokenBackoffForTest();
 
  private:
   void UpdateAuthErrorState(const GoogleServiceAuthError& error);