Fix: Properly disable Autofill Sync when Autofill itself is disabled

This is a fix to the SyncService::ReadyForStartChanged code path:
When stopping the data type, set an UNREADY_ERROR (instead of leaving
the error unset). This causes the Sync engine to reconfigure, which
properly marks the data type as no longer active. Before, the data
type did stop syncing, but was still returned as part of
SyncService::GetActiveDataTypes.

It also adds integration tests that AUTOFILL_PROFILE gets disabled on
autofill::prefs::SetProfileAutofillEnabled(.., false).

Bug: 895824, 890737
Change-Id: I3736ef010ad09ecd79a101aeff400d597956569e
Reviewed-on: https://chromium-review.googlesource.com/c/1319609
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605990}

chrome/browser/sync/test/integration/single_client_autofill_profile_sync_test.cc

+// Copyright 2018 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 "base/macros.h"
+#include "base/run_loop.h"
+#include "chrome/browser/sync/test/integration/autofill_helper.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "components/autofill/core/browser/autofill_profile.h"
+#include "components/autofill/core/browser/personal_data_manager.h"
+#include "components/autofill/core/common/autofill_prefs.h"
+#include "components/browser_sync/profile_sync_service.h"
+#include "components/prefs/pref_service.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/driver/sync_driver_switches.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class AutofillProfileDisabledChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit AutofillProfileDisabledChecker(
+      browser_sync::ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service) {}
+  ~AutofillProfileDisabledChecker() override = default;
+
+  // SingleClientStatusChangeChecker implementation.
+  bool IsExitConditionSatisfied() override {
+    return service()->GetTransportState() ==
+               syncer::SyncService::TransportState::ACTIVE &&
+           !service()->GetActiveDataTypes().Has(syncer::AUTOFILL_PROFILE);
+  }
+  std::string GetDebugMessage() const override {
+    return "Waiting for AUTOFILL_PROFILE to get disabled";
+  }
+};
+
+class SingleClientAutofillProfileSyncTest : public FeatureToggler,
+                                            public SyncTest {
+ public:
+  SingleClientAutofillProfileSyncTest()
+      : FeatureToggler(switches::kSyncUSSAutofillProfile),
+        SyncTest(SINGLE_CLIENT) {}
+  ~SingleClientAutofillProfileSyncTest() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SingleClientAutofillProfileSyncTest);
+};
+
+IN_PROC_BROWSER_TEST_P(SingleClientAutofillProfileSyncTest,
+                       DisablingAutofillAlsoDisablesSyncing) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_PROFILE));
+
+  // Add an autofill profile.
+  autofill_helper::AddProfile(0, autofill_helper::CreateAutofillProfile(
+                                     autofill_helper::PROFILE_HOMER));
+  autofill::PersonalDataManager* pdm =
+      autofill_helper::GetPersonalDataManager(0);
+  ASSERT_EQ(1uL, pdm->GetProfiles().size());
+
+  // Disable autofill (e.g. via chrome://settings).
+  autofill::prefs::SetProfileAutofillEnabled(GetProfile(0)->GetPrefs(), false);
+
+  // Wait for Sync to get reconfigured.
+  AutofillProfileDisabledChecker(GetClient(0)->service()).Wait();
+
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetClient(0)->service()->GetTransportState());
+
+  // This should also disable syncing of autofill profiles.
+  EXPECT_FALSE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_PROFILE));
+  // The autofill profile itself should still be there though.
+  EXPECT_EQ(1uL, pdm->GetProfiles().size());
+}
+
+INSTANTIATE_TEST_CASE_P(USS,
+                        SingleClientAutofillProfileSyncTest,
+                        ::testing::Values(false, true));
+
+}  // namespace

chrome/test/BUILD.gn

@@ -5330,6 +5330,7 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/migration_test.cc",
       "../browser/sync/test/integration/single_client_app_list_sync_test.cc",
       "../browser/sync/test/integration/single_client_apps_sync_test.cc",
+      "../browser/sync/test/integration/single_client_autofill_profile_sync_test.cc",
       "../browser/sync/test/integration/single_client_bookmarks_sync_test.cc",
       "../browser/sync/test/integration/single_client_custom_passphrase_sync_test.cc",
       "../browser/sync/test/integration/single_client_dictionary_sync_test.cc",

components/sync/driver/data_type_manager_impl.cc

@@ -123,8 +123,10 @@ void DataTypeManagerImpl::ReadyForStartChanged(ModelType type) {
   if (dtc_iter->second->ReadyForStart()) {
     ForceReconfiguration();
   } else {
-    // Stop the datatype
-    model_association_manager_.StopDatatype(type, DISABLE_SYNC, SyncError());
+    model_association_manager_.StopDatatype(
+        type, DISABLE_SYNC,
+        SyncError(FROM_HERE, syncer::SyncError::UNREADY_ERROR,
+                  "Data type is unready.", type));
   }
 }
 

components/sync/driver/model_association_manager.cc

@@ -178,6 +178,7 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
 void ModelAssociationManager::StopDatatype(ModelType type,
                                            ShutdownReason shutdown_reason,
                                            SyncError error) {
+  DCHECK(error.IsSet());
   DataTypeController* dtc = controllers_->find(type)->second.get();
   if (dtc->state() != DataTypeController::NOT_RUNNING &&
       dtc->state() != DataTypeController::STOPPING) {

components/sync/driver/model_association_manager.h

@@ -101,7 +101,8 @@ class ModelAssociationManager {
   // When this is completed, |OnModelAssociationDone| will be invoked.
   void StartAssociationAsync(const ModelTypeSet& types_to_associate);
 
-  // Stops an individual datatype |type| for |shutdown_reason|.
+  // Stops an individual datatype |type| for |shutdown_reason|. |error| must be
+  // an actual error (i.e. not UNSET).
   void StopDatatype(ModelType type,
                     ShutdownReason shutdown_reason,
                     SyncError error);

components/sync/driver/model_association_manager_unittest.cc

@@ -596,7 +596,10 @@ TEST_F(SyncModelAssociationManagerTest, StopDataType) {
   ASSERT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
 
-  model_association_manager.StopDatatype(BOOKMARKS, DISABLE_SYNC, SyncError());
+  model_association_manager.StopDatatype(
+      BOOKMARKS, DISABLE_SYNC,
+      SyncError(FROM_HERE, syncer::SyncError::UNREADY_ERROR,
+                "Data type is unready.", BOOKMARKS));
 
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::NOT_RUNNING);
@@ -612,7 +615,10 @@ TEST_F(SyncModelAssociationManagerTest, StopDataType_NotRunning) {
   ASSERT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::NOT_RUNNING);
 
-  model_association_manager.StopDatatype(BOOKMARKS, DISABLE_SYNC, SyncError());
+  model_association_manager.StopDatatype(
+      BOOKMARKS, DISABLE_SYNC,
+      SyncError(FROM_HERE, syncer::SyncError::UNREADY_ERROR,
+                "Data type is unready.", BOOKMARKS));
 
   // The state should still be not running.
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),