[Sync] Retry DeviceInfo commit on the next cycle after failure

DeviceInfo is normally updated once a day, therefore it's better to
retry its commit in case of failure. Prior to this CL DeviceInfo would
be committed again after browser restart only.

Fixed: 1345584
Change-Id: I7e30712b71b6c1649292ee56571805e236347af5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3773815
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1026209}

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

@@ -18,12 +18,8 @@
 #include "components/sync/base/features.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
-#include "components/sync/driver/glue/sync_transport_data_prefs.h"
-#include "components/sync/engine/loopback_server/persistent_tombstone_entity.h"
-#include "components/sync/nigori/nigori_test_utils.h"
 #include "components/sync/protocol/device_info_specifics.pb.h"
 #include "components/sync/protocol/entity_specifics.pb.h"
-#include "components/sync/protocol/proto_value_conversions.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/protocol/sync_entity.pb.h"
 #include "components/sync/protocol/sync_enums.pb.h"
@@ -33,7 +29,6 @@
 #include "components/sync_device_info/device_info_tracker.h"
 #include "components/sync_device_info/device_info_util.h"
 #include "content/public/test/browser_test.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -47,6 +42,7 @@ using testing::AllOf;
 using testing::Contains;
 using testing::ElementsAre;
 using testing::IsSupersetOf;
+using testing::Not;
 using testing::UnorderedElementsAre;
 
 MATCHER(HasFullHardwareClass, "") {
@@ -61,6 +57,19 @@ MATCHER_P(ModelEntryHasCacheGuid, expected_cache_guid, "") {
   return arg->guid() == expected_cache_guid;
 }
 
+MATCHER_P(HasInterestedDataType, expected_data_type, "") {
+  for (int32_t interested_data_type_id : arg.specifics()
+                                             .device_info()
+                                             .invalidation_fields()
+                                             .interested_data_type_ids()) {
+    if (interested_data_type_id ==
+        syncer::GetSpecificsFieldNumberFromModelType(expected_data_type)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 std::string CacheGuidForSuffix(int suffix) {
   return base::StringPrintf("cache guid %d", suffix);
 }
@@ -118,6 +127,37 @@ sync_pb::DeviceInfoSpecifics CreateSpecifics(int suffix) {
                          DefaultInterestedDataTypes());
 }
 
+// Waits for a DeviceInfo entity to be committed to the fake server (regardless
+// whether the commit succeeds or not). Note that it doesn't handle disabled
+// network case.
+class DeviceInfoCommitChecker : public SingleClientStatusChangeChecker {
+  // SingleClientStatusChangeChecker is used instead of
+  // FakeServerMatchStatusChecker because current checker is used when there is
+  // an HTTP error on the fake server.
+ public:
+  DeviceInfoCommitChecker(syncer::SyncServiceImpl* service,
+                          fake_server::FakeServer* fake_server)
+      : SingleClientStatusChangeChecker(service), fake_server_(fake_server) {}
+
+  // StatusChangeChecker overrides.
+  bool IsExitConditionSatisfied(std::ostream* os) override {
+    *os << "Waiting for DeviceInfo to be committed.";
+
+    sync_pb::ClientToServerMessage message;
+    fake_server_->GetLastCommitMessage(&message);
+    for (const sync_pb::SyncEntity& entity : message.commit().entries()) {
+      if (entity.specifics().has_device_info()) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+ private:
+  const raw_ptr<fake_server::FakeServer> fake_server_;
+};
+
 class SingleClientDeviceInfoSyncTest : public SyncTest {
  public:
   SingleClientDeviceInfoSyncTest() : SyncTest(SINGLE_CLIENT) {
@@ -161,6 +201,9 @@ class SingleClientDeviceInfoSyncTest : public SyncTest {
             /*creation_time=*/0, /*last_modified_time=*/0));
   }
 
+  // SyncTest overrides.
+  bool UseConfigurationRefresher() override { return false; }
+
  private:
   base::test::ScopedFeatureList override_features_;
 };
@@ -519,6 +562,28 @@ IN_PROC_BROWSER_TEST_F(SingleClientDeviceInfoSyncTest,
                   .Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientDeviceInfoSyncTest,
+                       ShouldRetryDeviceInfoCommitOnAuthError) {
+  ASSERT_TRUE(SetupSync());
+
+  GetFakeServer()->SetHttpError(net::HTTP_UNAUTHORIZED);
+
+  // Disable another data type to trigger a commit of a new DeviceInfo entity.
+  // Create a checker to catch a commit request before disabling the data type.
+  DeviceInfoCommitChecker device_info_committer_checker(GetSyncService(0),
+                                                        GetFakeServer());
+  ASSERT_TRUE(
+      GetClient(0)->DisableSyncForType(syncer::UserSelectableType::kBookmarks));
+  ASSERT_TRUE(device_info_committer_checker.Wait());
+
+  GetFakeServer()->ClearHttpError();
+
+  // Wait for the DeviceInfo to be committed to the fake server again.
+  EXPECT_TRUE(ServerDeviceInfoMatchChecker(
+                  ElementsAre(Not(HasInterestedDataType(syncer::BOOKMARKS))))
+                  .Wait());
+}
+
 // PRE_* tests aren't supported on Android browser tests.
 #if !BUILDFLAG(IS_ANDROID)
 IN_PROC_BROWSER_TEST_F(SingleClientDeviceInfoSyncTest,

components/sync_device_info/DEPS

@@ -5,6 +5,7 @@ include_rules = [
   "+components/metrics",
   "+components/prefs",
   "+components/sync/base",
+  "+components/sync/engine",
   "+components/sync/invalidations",
   "+components/sync/model",
   "+components/sync/protocol",

components/sync_device_info/device_info_sync_bridge.cc

@@ -18,10 +18,10 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/observer_list.h"
-#include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
+#include "components/sync/engine/commit_and_get_updates_types.h"
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/model/metadata_batch.h"
@@ -530,6 +530,26 @@ void DeviceInfoSyncBridge::ApplyStopSyncChanges(
   }
 }
 
+ModelTypeSyncBridge::CommitAttemptFailedBehavior
+DeviceInfoSyncBridge::OnCommitAttemptFailed(
+    syncer::SyncCommitError commit_error) {
+  // DeviceInfo is normally committed once a day and hence it's important to
+  // retry on the next sync cycle in case of auth or network errors. For other
+  // errors, do not retry to prevent blocking sync for other data types if
+  // DeviceInfo entity causes the error. OnCommitAttemptErrors would show that
+  // something is wrong with the DeviceInfo entity from the last commit request
+  // but those errors are not retried at the moment since it's a very tiny
+  // fraction.
+  switch (commit_error) {
+    case syncer::SyncCommitError::kAuthError:
+    case syncer::SyncCommitError::kNetworkError:
+      return CommitAttemptFailedBehavior::kShouldRetryOnNextCycle;
+    case syncer::SyncCommitError::kBadServerResponse:
+    case syncer::SyncCommitError::kServerError:
+      return CommitAttemptFailedBehavior::kDontRetryOnNextCycle;
+  }
+}
+
 bool DeviceInfoSyncBridge::IsSyncing() const {
   // Both conditions are neecessary due to the following possible cases:
   // 1. This method is called from MergeSyncData() when IsTrackingMetadata()

components/sync_device_info/device_info_sync_bridge.h

@@ -83,6 +83,8 @@ class DeviceInfoSyncBridge : public ModelTypeSyncBridge,
   std::string GetStorageKey(const EntityData& entity_data) override;
   void ApplyStopSyncChanges(
       std::unique_ptr<MetadataChangeList> delete_metadata_change_list) override;
+  ModelTypeSyncBridge::CommitAttemptFailedBehavior OnCommitAttemptFailed(
+      syncer::SyncCommitError commit_error) override;
 
   // DeviceInfoTracker implementation.
   bool IsSyncing() const override;