[search engines] Add sync integration test for deletion of default

This patch adds a sync integration test that exercises a problematic
scenario: the deletion of a search engine via sync while the engine is
the default one locally.

This is possible because the default search engine is sync-ed via
preferences, that is, syncer::PREFERENCES, whereas the actual list of
search engines is sync-ed via another sync datatype, namely
syncer::SEARCH_ENGINES.

Ordering across different sync datatypes is not guaranteed and
therefore, if a user changes the default search engine and deletes the
old one, it is possible that a remote sync-ing device observes this
changes in the opposite order.

There is logic in TemplateURLService to special-case this scenario to
append underscores, and this patch simply documents this behavior using
a two-client sync integration test. Because reproducing the precise
problematic ordering is not feasible, the test leverages a
newly-introduced functionality in FakeServer that allows throttling
sync datatypes (resembling a very slow sync for preferences).

Change-Id: Ia0929717d0ea99a38969298b94f68a1c1ab44364
Bug: 1022775
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2471398
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#820238}

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

@@ -19,7 +19,6 @@
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/search_engines/template_url.h"
-#include "components/search_engines/template_url_service.h"
 
 using sync_datatype_helper::test;
 
@@ -54,7 +53,7 @@ bool TURLsMatch(const TemplateURL& turl1, const TemplateURL& turl2) {
   return result;
 }
 
-bool ServicesMatch(int profile_a, int profile_b) {
+bool ServicesMatch(int profile_a, int profile_b, std::ostream* os) {
   TemplateURLService* service_a =
       search_engines_helper::GetServiceForBrowserContext(profile_a);
   TemplateURLService* service_b =
@@ -66,14 +65,14 @@ bool ServicesMatch(int profile_a, int profile_b) {
   GUIDToTURLMap b_turls = CreateGUIDToTURLMap(service_b);
 
   if (a_turls.size() != b_turls.size()) {
-    DVLOG(1) << "Service a and b do not match in size: " << a_turls.size()
-             << " vs " << b_turls.size() << " respectively.";
+    *os << "Service a and b do not match in size: " << a_turls.size() << " vs "
+        << b_turls.size() << " respectively.";
     return false;
   }
 
   for (auto it = a_turls.begin(); it != a_turls.end(); ++it) {
     if (b_turls.find(it->first) == b_turls.end()) {
-      DVLOG(1) << "TURL GUID from a not found in b's TURLs: " << it->first;
+      *os << "TURL GUID from a not found in b's TURLs: " << it->first;
       return false;
     }
     if (!TURLsMatch(*b_turls[it->first], *it->second))
@@ -83,12 +82,11 @@ bool ServicesMatch(int profile_a, int profile_b) {
   const TemplateURL* default_a = service_a->GetDefaultSearchProvider();
   const TemplateURL* default_b = service_b->GetDefaultSearchProvider();
   if (!TURLsMatch(*default_a, *default_b)) {
-    DVLOG(1) << "Default search providers do not match: A's default: "
-             << default_a->keyword()
-             << " B's default: " << default_b->keyword();
+    *os << "Default search providers do not match: A's default: "
+        << default_a->keyword() << " B's default: " << default_b->keyword();
     return false;
   } else {
-    DVLOG(1) << "A had default with URL: " << default_a->url()
+    *os << "A had default with URL: " << default_a->url()
         << " and keyword: " << default_a->keyword();
   }
 
@@ -145,15 +143,19 @@ bool ServiceMatchesVerifier(int profile_index) {
 }
 
 bool AllServicesMatch() {
+  return AllServicesMatch(&VLOG_STREAM(1));
+}
+
+bool AllServicesMatch(std::ostream* os) {
   // Use 0 as the baseline.
   if (test()->UseVerifier() && !ServiceMatchesVerifier(0)) {
-    DVLOG(1) << "TemplateURLService 0 does not match verifier.";
+    *os << "TemplateURLService 0 does not match verifier.";
     return false;
   }
 
   for (int it = 1; it < test()->num_clients(); ++it) {
-    if (!ServicesMatch(0, it)) {
-      DVLOG(1) << "TemplateURLService " << it << " does not match with "
+    if (!ServicesMatch(0, it, os)) {
+      *os << "TemplateURLService " << it << " does not match with "
           << "service 0.";
       return false;
     }
@@ -252,15 +254,58 @@ void ChangeDefaultSearchProvider(int profile_index, int seed) {
 }
 
 bool HasSearchEngine(int profile_index, int seed) {
+  return HasSearchEngineWithKeyword(profile_index, CreateKeyword(seed));
+}
+
+bool HasSearchEngineWithKeyword(int profile_index,
+                                const base::string16& keyword) {
   TemplateURLService* service = GetServiceForBrowserContext(profile_index);
-  base::string16 keyword(CreateKeyword(seed));
   TemplateURL* turl = service->GetTemplateURLForKeyword(keyword);
   return turl != nullptr;
 }
 
-}  // namespace search_engines_helper
+base::string16 GetDefaultSearchEngineKeyword(int profile_index) {
+  TemplateURLService* service = GetServiceForBrowserContext(profile_index);
+  return service->GetDefaultSearchProvider()->keyword();
+}
+
+SearchEnginesMatchChecker::SearchEnginesMatchChecker() {
+  if (test()->UseVerifier()) {
+    observer_.Add(GetVerifierService());
+  }
+
+  for (int i = 0; i < test()->num_clients(); ++i) {
+    observer_.Add(GetServiceForBrowserContext(i));
+  }
+}
 
-SearchEnginesMatchChecker::SearchEnginesMatchChecker()
-    : AwaitMatchStatusChangeChecker(
-          base::Bind(search_engines_helper::AllServicesMatch),
-          "All search engines match") {}
+SearchEnginesMatchChecker::~SearchEnginesMatchChecker() = default;
+
+bool SearchEnginesMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
+  return AllServicesMatch(os);
+}
+
+void SearchEnginesMatchChecker::OnTemplateURLServiceChanged() {
+  CheckExitCondition();
+}
+
+HasSearchEngineChecker::HasSearchEngineChecker(int profile_index, int seed)
+    : HasSearchEngineChecker(profile_index, CreateKeyword(seed)) {}
+
+HasSearchEngineChecker::HasSearchEngineChecker(int profile_index,
+                                               const base::string16& keyword)
+    : service_(GetServiceForBrowserContext(profile_index)), keyword_(keyword) {
+  observer_.Add(service_);
+}
+
+HasSearchEngineChecker::~HasSearchEngineChecker() = default;
+
+bool HasSearchEngineChecker::IsExitConditionSatisfied(std::ostream* os) {
+  return service_->GetTemplateURLForKeyword(keyword_) != nullptr;
+}
+
+void HasSearchEngineChecker::OnTemplateURLServiceChanged() {
+  CheckExitCondition();
+}
+
+}  // namespace search_engines_helper

chrome/browser/sync/test/integration/search_engines_helper.h

@@ -9,8 +9,11 @@
 #include <memory>
 #include <string>
 
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
-#include "chrome/browser/sync/test/integration/await_match_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/status_change_checker.h"
+#include "components/search_engines/template_url_service.h"
+#include "components/search_engines/template_url_service_observer.h"
 
 class Profile;
 class TemplateURL;
@@ -32,6 +35,7 @@ bool ServiceMatchesVerifier(int profile_index);
 
 // Returns true iff all TemplateURLServices match with the verifier.
 bool AllServicesMatch();
+bool AllServicesMatch(std::ostream* os);
 
 // Create a TemplateURL with some test values based on |seed|.
 std::unique_ptr<TemplateURL> CreateTestTemplateURL(
@@ -72,12 +76,54 @@ void ChangeDefaultSearchProvider(int profile_index, int seed);
 // the search engine generated with |seed|.
 bool HasSearchEngine(int profile_index, int seed);
 
-}  // namespace search_engines_helper
+// Returns true if the profile at |profile_index| has a search engine matching
+// |keyword|.
+bool HasSearchEngineWithKeyword(int profile_index,
+                                const base::string16& keyword);
+
+// Returns the keyword for the default search engine at |profile_index|.
+base::string16 GetDefaultSearchEngineKeyword(int profile_index);
 
 // Checker that blocks until all services have the same search engine data.
-class SearchEnginesMatchChecker : public AwaitMatchStatusChangeChecker {
+class SearchEnginesMatchChecker : public StatusChangeChecker,
+                                  public TemplateURLServiceObserver {
  public:
   SearchEnginesMatchChecker();
+  ~SearchEnginesMatchChecker() override;
+
+  // StatusChangeChecker overrides.
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+
+  // TemplateURLServiceObserver overrides.
+  void OnTemplateURLServiceChanged() override;
+
+ private:
+  ScopedObserver<TemplateURLService, TemplateURLServiceObserver> observer_{
+      this};
+};
+
+// Checker that blocks until |profile_index| has a search engine matching the
+// search engine generated with |seed| or |keyword|.
+class HasSearchEngineChecker : public StatusChangeChecker,
+                               public TemplateURLServiceObserver {
+ public:
+  HasSearchEngineChecker(int profile_index, int seed);
+  HasSearchEngineChecker(int profile_index, const base::string16& keyword);
+  ~HasSearchEngineChecker() override;
+
+  // StatusChangeChecker overrides.
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+
+  // TemplateURLServiceObserver overrides.
+  void OnTemplateURLServiceChanged() override;
+
+ private:
+  TemplateURLService* const service_;
+  const base::string16 keyword_;
+  ScopedObserver<TemplateURLService, TemplateURLServiceObserver> observer_{
+      this};
 };
 
+}  // namespace search_engines_helper
+
 #endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_SEARCH_ENGINES_HELPER_H_

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

@@ -16,6 +16,7 @@
 #include "content/public/test/browser_test.h"
 
 using base::ASCIIToUTF16;
+using search_engines_helper::SearchEnginesMatchChecker;
 
 class TwoClientSearchEnginesSyncTest : public SyncTest {
  public:
@@ -305,3 +306,71 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
   search_engines_helper::DeleteSearchEngineBySeed(0, 0);
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
+
+// Same as above that forces the deletion to propagate faster than the
+// preference, which is complex to deal with for the receiving client (deletion
+// of the default search engine), and currently leads to search engines with
+// underscores being created. This is achieved in the test by throttling
+// PREFERENCES in FakeServer, which prevents the sync-ing of the default search
+// engine change.
+IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+                       DeleteSyncedDefaultWithoutPrefSync) {
+  ASSERT_TRUE(SetupClients());
+
+  search_engines_helper::AddSearchEngine(/*profile_index=*/0, /*seed=*/0);
+  search_engines_helper::AddSearchEngine(/*profile_index=*/0, /*seed=*/1);
+  search_engines_helper::AddSearchEngine(/*profile_index=*/1, /*seed=*/0);
+  search_engines_helper::AddSearchEngine(/*profile_index=*/1, /*seed=*/1);
+  search_engines_helper::ChangeDefaultSearchProvider(/*profile_index=*/0,
+                                                     /*seed=*/0);
+
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
+
+  // Throttle PREFERENCES to block any commits to them, which in this case
+  // prevents the default search engine selection from sync-ing.
+  GetFakeServer()->SetThrottledTypes({syncer::PREFERENCES});
+
+  // Rule out search engines with underscores existing at this point.
+  // Note that seed==0 corresponds to keyword "test0".
+  ASSERT_TRUE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/0, ASCIIToUTF16("test0")));
+  ASSERT_FALSE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/0, ASCIIToUTF16("test0_")));
+  ASSERT_TRUE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/1, ASCIIToUTF16("test0")));
+  ASSERT_FALSE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/1, ASCIIToUTF16("test0_")));
+
+  // Change the default on the first client (profile index 0) and delete the old
+  // default.
+  search_engines_helper::ChangeDefaultSearchProvider(/*profile_index=*/0,
+                                                     /*seed=*/1);
+  search_engines_helper::DeleteSearchEngineBySeed(/*profile_index=*/0,
+                                                  /*seed=*/0);
+
+  // The test needs to wait until the second client (profile index 1) receives
+  // the deletion. In order to do so, use the first client (profile index 0) to
+  // create a third search engine (seed 2) and wait until it gets sync-ed to the
+  // second client (profile index 1).
+  search_engines_helper::AddSearchEngine(/*profile_index=*/0, /*seed=*/2);
+  ASSERT_TRUE(search_engines_helper::HasSearchEngineChecker(/*profile_index=*/1,
+                                                            /*seed=*/2)
+                  .Wait());
+
+  // In the receiving end (profile index 1), the deletion cannot be honored as
+  // is, so instead the keyword is changed for the default search engine by
+  // appending an underscore.
+  EXPECT_TRUE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/1, ASCIIToUTF16("test0_")));
+  EXPECT_FALSE(search_engines_helper::HasSearchEngineWithKeyword(
+      /*profile_index=*/1, ASCIIToUTF16("test0")));
+  EXPECT_EQ(
+      search_engines_helper::GetDefaultSearchEngineKeyword(/*profile_index=*/1),
+      ASCIIToUTF16("test0_"));
+
+  // The search engine with an underscore should sync back to profile index 0.
+  EXPECT_TRUE(search_engines_helper::HasSearchEngineChecker(
+                  /*profile_index=*/0, ASCIIToUTF16("test0_"))
+                  .Wait());
+}

components/sync/engine_impl/loopback_server/loopback_server.cc

@@ -326,6 +326,7 @@ net::HttpStatusCode LoopbackServer::HandleCommand(
   } else {
     bool success = false;
     std::vector<ModelType> datatypes_to_migrate;
+    ModelTypeSet throttled_datatypes_in_request;
     switch (message.message_contents()) {
       case sync_pb::ClientToServerMessage::GET_UPDATES:
         success = HandleGetUpdatesRequest(
@@ -334,9 +335,9 @@ net::HttpStatusCode LoopbackServer::HandleCommand(
             &datatypes_to_migrate);
         break;
       case sync_pb::ClientToServerMessage::COMMIT:
-        success = HandleCommitRequest(message.commit(),
-                                      message.invalidator_client_id(),
-                                      response->mutable_commit());
+        success = HandleCommitRequest(
+            message.commit(), message.invalidator_client_id(),
+            response->mutable_commit(), &throttled_datatypes_in_request);
         break;
       case sync_pb::ClientToServerMessage::CLEAR_SERVER_DATA:
         ClearServerData();
@@ -350,19 +351,29 @@ net::HttpStatusCode LoopbackServer::HandleCommand(
 
     if (success) {
       response->set_error_code(sync_pb::SyncEnums::SUCCESS);
-    } else if (datatypes_to_migrate.empty()) {
-      UMA_HISTOGRAM_ENUMERATION(
-          "Sync.Local.RequestTypeOnError", message.message_contents(),
-          sync_pb::ClientToServerMessage_Contents_Contents_MAX);
-      return net::HTTP_INTERNAL_SERVER_ERROR;
-    } else {
+    } else if (!datatypes_to_migrate.empty()) {
       DLOG(WARNING) << "Migration required for " << datatypes_to_migrate.size()
                     << " datatypes";
+      response->set_error_code(sync_pb::SyncEnums::MIGRATION_DONE);
       for (ModelType type : datatypes_to_migrate) {
         response->add_migrated_data_type_id(
             GetSpecificsFieldNumberFromModelType(type));
       }
-      response->set_error_code(sync_pb::SyncEnums::MIGRATION_DONE);
+    } else if (!throttled_datatypes_in_request.Empty()) {
+      DLOG(WARNING) << "Throttled datatypes: "
+                    << ModelTypeSetToString(throttled_datatypes_in_request);
+      response->set_error_code(sync_pb::SyncEnums::PARTIAL_FAILURE);
+      response->mutable_error()->set_error_type(
+          sync_pb::SyncEnums::PARTIAL_FAILURE);
+      for (ModelType type : throttled_datatypes_in_request) {
+        response->mutable_error()->add_error_data_type_ids(
+            syncer::GetSpecificsFieldNumberFromModelType(type));
+      }
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Sync.Local.RequestTypeOnError", message.message_contents(),
+          sync_pb::ClientToServerMessage_Contents_Contents_MAX);
+      return net::HTTP_INTERNAL_SERVER_ERROR;
     }
   }
 
@@ -610,7 +621,8 @@ void LoopbackServer::DeleteChildren(const string& parent_id) {
 bool LoopbackServer::HandleCommitRequest(
     const sync_pb::CommitMessage& commit,
     const std::string& invalidator_client_id,
-    sync_pb::CommitResponse* response) {
+    sync_pb::CommitResponse* response,
+    ModelTypeSet* throttled_datatypes_in_request) {
   std::map<string, string> client_to_server_ids;
   string guid = commit.cache_guid();
   ModelTypeSet committed_model_types;
@@ -630,6 +642,13 @@ bool LoopbackServer::HandleCommitRequest(
       parent_id = client_to_server_ids[parent_id];
     }
 
+    const ModelType entity_model_type = GetModelType(client_entity);
+    if (throttled_types_.Has(entity_model_type)) {
+      entry_response->set_response_type(sync_pb::CommitResponse::OVER_QUOTA);
+      throttled_datatypes_in_request->Put(entity_model_type);
+      continue;
+    }
+
     const string entity_id =
         CommitEntity(client_entity, entry_response, guid, parent_id);
     if (entity_id.empty()) {
@@ -663,7 +682,7 @@ bool LoopbackServer::HandleCommitRequest(
   if (observer_for_tests_)
     observer_for_tests_->OnCommit(invalidator_client_id, committed_model_types);
 
-  return true;
+  return throttled_datatypes_in_request->Empty();
 }
 
 void LoopbackServer::ClearServerData() {

components/sync/engine_impl/loopback_server/loopback_server.h

@@ -81,6 +81,10 @@ class LoopbackServer : public base::ImportantFileWriter::DataSerializer {
 
   void AddNewKeystoreKeyForTesting();
 
+  void SetThrottledTypesForTesting(ModelTypeSet model_types) {
+    throttled_types_ = model_types;
+  }
+
  private:
   // Allow the FakeServer decorator to inspect the internals of this class.
   friend class fake_server::FakeServer;
@@ -108,7 +112,8 @@ class LoopbackServer : public base::ImportantFileWriter::DataSerializer {
   // Processes a Commit call.
   bool HandleCommitRequest(const sync_pb::CommitMessage& message,
                            const std::string& invalidator_client_id,
-                           sync_pb::CommitResponse* response);
+                           sync_pb::CommitResponse* response,
+                           ModelTypeSet* throttled_datatypes_in_request);
 
   void ClearServerData();
 
@@ -229,6 +234,8 @@ class LoopbackServer : public base::ImportantFileWriter::DataSerializer {
 
   int64_t store_birthday_;
 
+  ModelTypeSet throttled_types_;
+
   base::Optional<sync_pb::ChipBag> bag_of_chips_;
 
   std::map<ModelType, int> migration_versions_;

components/sync/test/fake_server/fake_server.cc

@@ -577,6 +577,10 @@ void FakeServer::DisallowSendingEncryptionKeys() {
   disallow_sending_encryption_keys_ = true;
 }
 
+void FakeServer::SetThrottledTypes(syncer::ModelTypeSet types) {
+  loopback_server_->SetThrottledTypesForTesting(types);
+}
+
 bool FakeServer::ShouldSendTriggeredError() const {
   if (!alternate_triggered_errors_)
     return true;

components/sync/test/fake_server/fake_server.h

@@ -194,6 +194,9 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // encryption_keys.
   void DisallowSendingEncryptionKeys();
 
+  // Mimics throttling of datatypes.
+  void SetThrottledTypes(syncer::ModelTypeSet types);
+
   // Adds |observer| to FakeServer's observer list. This should be called
   // before the Profile associated with |observer| is connected to the server.
   void AddObserver(Observer* observer);