[Dice] Avoid showing the sync confirmation dialog before sync engine starts.

In pre-DICE, the sync confirmation dialog was always shown after the
accounts were added to the Gaia cookies. This took several seconds
allowing the sync engine to initialize and check whether sync was
allowed for the given account.

When DICE enabled, the sync confirmation dialog should only be shown
after sync has been initialized or failed to be initialized. This allows
us to make sure that the policy to enable or disable sync for the given
account is checked before displaying the sync confirmation dialog.

BUG = 812546

Change-Id: Ic414cd8eb5a810a1ff0827526361b10f8e76210b
Reviewed-on: https://chromium-review.googlesource.com/921741Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537348}

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

@@ -306,15 +306,42 @@ void DiceTurnSyncOnHelper::SigninAndShowSyncConfirmationUI() {
   signin_metrics::LogSigninReason(signin_reason_);
   base::RecordAction(base::UserMetricsAction("Signin_Signin_Succeed"));
 
-  // Take a SyncSetupInProgressHandle, so that the UI code can use
-  // IsFirstSyncSetupInProgress() as a way to know if there is a signin in
-  // progress.
-  // TODO(https://crbug.com/811211): Remove this handle.
   browser_sync::ProfileSyncService* sync_service = GetProfileSyncService();
-  if (sync_service)
+  if (sync_service) {
+    // Take a SyncSetupInProgressHandle, so that the UI code can use
+    // IsFirstSyncSetupInProgress() as a way to know if there is a signin in
+    // progress.
+    // TODO(https://crbug.com/811211): Remove this handle.
     sync_blocker_ = sync_service->GetSetupInProgressHandle();
 
-  // Show Sync confirmation.
+    if (SyncStartupTracker::GetSyncServiceState(profile_) ==
+        SyncStartupTracker::SYNC_STARTUP_PENDING) {
+      // Wait until sync is initialized so that the confirmation UI can be
+      // aware of startup errors. This is needed to make sure that the sync
+      // confirmation dialog is shown only after the sync service had a chance
+      // to check whether sync was disabled by admin.
+      // See http://crbug.com/812546
+      sync_startup_tracker_.reset(new SyncStartupTracker(profile_, this));
+      return;
+    }
+  }
+
+  ShowSyncConfirmationUI();
+}
+
+void DiceTurnSyncOnHelper::SyncStartupCompleted() {
+  DCHECK(sync_startup_tracker_);
+  sync_startup_tracker_.reset();
+  ShowSyncConfirmationUI();
+}
+
+void DiceTurnSyncOnHelper::SyncStartupFailed() {
+  DCHECK(sync_startup_tracker_);
+  sync_startup_tracker_.reset();
+  ShowSyncConfirmationUI();
+}
+
+void DiceTurnSyncOnHelper::ShowSyncConfirmationUI() {
   delegate_->ShowSyncConfirmation(
       base::BindOnce(&DiceTurnSyncOnHelper::FinishSyncSetupAndDelete,
                      weak_pointer_factory_.GetWeakPtr()));

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/sync_startup_tracker.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/signin_metrics.h"
@@ -30,7 +31,7 @@ class SyncSetupInProgressHandle;
 
 // Handles details of signing the user in with SigninManager and turning on
 // sync for an account that is already present in the token service.
-class DiceTurnSyncOnHelper {
+class DiceTurnSyncOnHelper : public SyncStartupTracker::Observer {
  public:
   // Behavior when the signin is aborted (by an error or cancelled by the user).
   enum class SigninAbortedMode {
@@ -107,6 +108,10 @@ class DiceTurnSyncOnHelper {
                        const std::string& account_id,
                        SigninAbortedMode signin_aborted_mode);
 
+  // SyncStartupTracker::Observer:
+  void SyncStartupCompleted() override;
+  void SyncStartupFailed() override;
+
  private:
   enum class ProfileMode {
     // Attempts to sign the user in |profile_|. Note that if the account to be
@@ -120,7 +125,7 @@ class DiceTurnSyncOnHelper {
   };
 
   // DiceTurnSyncOnHelper deletes itself.
-  ~DiceTurnSyncOnHelper();
+  ~DiceTurnSyncOnHelper() override;
 
   // Handles can offer sign-in errors.  It returns true if there is an error,
   // and false otherwise.
@@ -164,6 +169,11 @@ class DiceTurnSyncOnHelper {
   // UI.
   void SigninAndShowSyncConfirmationUI();
 
+  // Displays the Sync confirmation UI.
+  // Note: If sync fails to start (e.g. sync is disabled by admin), the sync
+  // confirmation dialog will be updated accordingly.
+  void ShowSyncConfirmationUI();
+
   // Handles the user input from the sync confirmation UI and deletes this
   // object.
   void FinishSyncSetupAndDelete(
@@ -193,6 +203,8 @@ class DiceTurnSyncOnHelper {
   std::string dm_token_;
   std::string client_id_;
 
+  std::unique_ptr<SyncStartupTracker> sync_startup_tracker_;
+
   base::WeakPtrFactory<DiceTurnSyncOnHelper> weak_pointer_factory_;
   DISALLOW_COPY_AND_ASSIGN(DiceTurnSyncOnHelper);
 };

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc

@@ -38,6 +38,9 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::AtLeast;
+using ::testing::Return;
+
 class DiceTurnSyncOnHelperTest;
 
 namespace {
@@ -221,6 +224,33 @@ class DiceTurnSyncOnHelperTest : public testing::Test {
         std::make_unique<TestDiceTurnSyncOnHelperDelegate>(this));
   }
 
+  void SetExpectationsForSyncStartupCompleted() {
+    browser_sync::ProfileSyncServiceMock* sync_service_mock =
+        GetProfileSyncServiceMock();
+    EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
+    EXPECT_CALL(*sync_service_mock, CanSyncStart())
+        .Times(AtLeast(0))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*sync_service_mock, IsEngineInitialized())
+        .Times(AtLeast(0))
+        .WillRepeatedly(Return(true));
+  }
+
+  void SetExpectationsForSyncStartupPending() {
+    browser_sync::ProfileSyncServiceMock* sync_service_mock =
+        GetProfileSyncServiceMock();
+    EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
+    EXPECT_CALL(*sync_service_mock, CanSyncStart())
+        .Times(AtLeast(0))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*sync_service_mock, IsEngineInitialized())
+        .Times(AtLeast(0))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*sync_service_mock, waiting_for_auth())
+        .Times(AtLeast(0))
+        .WillRepeatedly(Return(true));
+  }
+
   void CheckDelegateCalls() {
     EXPECT_EQ(expected_login_error_email_, login_error_email_);
     EXPECT_EQ(expected_login_error_message_, login_error_message_);
@@ -532,10 +562,9 @@ TEST_F(DiceTurnSyncOnHelperTest, EnterpriseConfirmationNewProfile) {
 TEST_F(DiceTurnSyncOnHelperTest, UndoSync) {
   // Set expectations.
   expected_sync_confirmation_shown_ = true;
-  browser_sync::ProfileSyncServiceMock* sync_service_mock =
-      GetProfileSyncServiceMock();
-  EXPECT_CALL(*sync_service_mock, SetFirstSetupComplete()).Times(0);
-  EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
+  SetExpectationsForSyncStartupCompleted();
+  EXPECT_CALL(*GetProfileSyncServiceMock(), SetFirstSetupComplete()).Times(0);
+
   // Signin flow.
   EXPECT_FALSE(signin_manager()->IsAuthenticated());
   CreateDiceTurnOnSyncHelper(
@@ -551,10 +580,9 @@ TEST_F(DiceTurnSyncOnHelperTest, ConfigureSync) {
   // Set expectations.
   expected_sync_confirmation_shown_ = true;
   expected_sync_settings_shown_ = true;
-  browser_sync::ProfileSyncServiceMock* sync_service_mock =
-      GetProfileSyncServiceMock();
-  EXPECT_CALL(*sync_service_mock, SetFirstSetupComplete()).Times(0);
-  EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
+  SetExpectationsForSyncStartupCompleted();
+  EXPECT_CALL(*GetProfileSyncServiceMock(), SetFirstSetupComplete()).Times(0);
+
   // Configure the test.
   sync_confirmation_result_ =
       LoginUIService::SyncConfirmationUIClosedResult::CONFIGURE_SYNC_FIRST;
@@ -572,10 +600,8 @@ TEST_F(DiceTurnSyncOnHelperTest, ConfigureSync) {
 TEST_F(DiceTurnSyncOnHelperTest, StartSync) {
   // Set expectations.
   expected_sync_confirmation_shown_ = true;
-  browser_sync::ProfileSyncServiceMock* sync_service_mock =
-      GetProfileSyncServiceMock();
-  EXPECT_CALL(*sync_service_mock, SetFirstSetupComplete()).Times(1);
-  EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
+  SetExpectationsForSyncStartupCompleted();
+  EXPECT_CALL(*GetProfileSyncServiceMock(), SetFirstSetupComplete()).Times(1);
   // Configure the test.
   sync_confirmation_result_ = LoginUIService::SyncConfirmationUIClosedResult::
       SYNC_WITH_DEFAULT_SETTINGS;
@@ -588,3 +614,58 @@ TEST_F(DiceTurnSyncOnHelperTest, StartSync) {
   EXPECT_EQ(account_id(), signin_manager()->GetAuthenticatedAccountId());
   CheckDelegateCalls();
 }
+
+// Tests that the user is signed in and Sync configuration is complete.
+// Regression test for http://crbug.com/812546
+TEST_F(DiceTurnSyncOnHelperTest,
+       ShowSyncDialogBlockedUntilSyncStartupCompleted) {
+  // Set expectations.
+  expected_sync_confirmation_shown_ = false;
+  SetExpectationsForSyncStartupPending();
+
+  // Signin flow.
+  EXPECT_FALSE(signin_manager()->IsAuthenticated());
+  DiceTurnSyncOnHelper* dice_sync_starter = CreateDiceTurnOnSyncHelper(
+      DiceTurnSyncOnHelper::SigninAbortedMode::REMOVE_ACCOUNT);
+
+  // Check that the account was set in the sign-in manager, but the sync
+  // confirmation dialog was not yet shown.
+  EXPECT_TRUE(token_service()->RefreshTokenIsAvailable(account_id()));
+  EXPECT_EQ(account_id(), signin_manager()->GetAuthenticatedAccountId());
+  CheckDelegateCalls();
+
+  // Simulate that sync startup has completed.
+  expected_sync_confirmation_shown_ = true;
+  EXPECT_CALL(*GetProfileSyncServiceMock(), SetFirstSetupComplete()).Times(1);
+  sync_confirmation_result_ = LoginUIService::SyncConfirmationUIClosedResult::
+      SYNC_WITH_DEFAULT_SETTINGS;
+  dice_sync_starter->SyncStartupCompleted();
+  CheckDelegateCalls();
+}
+
+// Tests that the user is signed in and Sync configuration is complete.
+// Regression test for http://crbug.com/812546
+TEST_F(DiceTurnSyncOnHelperTest, ShowSyncDialogBlockedUntilSyncStartupFailed) {
+  // Set expectations.
+  expected_sync_confirmation_shown_ = false;
+  SetExpectationsForSyncStartupPending();
+
+  // Signin flow.
+  EXPECT_FALSE(signin_manager()->IsAuthenticated());
+  DiceTurnSyncOnHelper* dice_sync_starter = CreateDiceTurnOnSyncHelper(
+      DiceTurnSyncOnHelper::SigninAbortedMode::REMOVE_ACCOUNT);
+
+  // Check that the primary account was added to the token service and in the
+  // sign-in manager.
+  EXPECT_TRUE(token_service()->RefreshTokenIsAvailable(account_id()));
+  EXPECT_EQ(account_id(), signin_manager()->GetAuthenticatedAccountId());
+  CheckDelegateCalls();
+
+  // Simulate that sync startup has failed.
+  expected_sync_confirmation_shown_ = true;
+  EXPECT_CALL(*GetProfileSyncServiceMock(), SetFirstSetupComplete()).Times(1);
+  sync_confirmation_result_ = LoginUIService::SyncConfirmationUIClosedResult::
+      SYNC_WITH_DEFAULT_SETTINGS;
+  dice_sync_starter->SyncStartupFailed();
+  CheckDelegateCalls();
+}