From d7fe9d4b135defca21bf41e2df5e30a787c09ee4 Mon Sep 17 00:00:00 2001 From: Rushan Suleymanov Date: Wed, 20 Jul 2022 13:13:49 +0000 Subject: [PATCH] [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 Reviewed-by: Marc Treib Cr-Commit-Position: refs/heads/main@{#1026209} --- .../single_client_device_info_sync_test.cc | 75 +++++++++++++++++-- components/sync_device_info/DEPS | 1 + .../device_info_sync_bridge.cc | 22 +++++- .../device_info_sync_bridge.h | 2 + 4 files changed, 94 insertions(+), 6 deletions(-) diff --git a/chrome/browser/sync/test/integration/single_client_device_info_sync_test.cc b/chrome/browser/sync/test/integration/single_client_device_info_sync_test.cc index c11155deddea68..1ff310875e7e96 100644 --- a/chrome/browser/sync/test/integration/single_client_device_info_sync_test.cc +++ b/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_; +}; + 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, diff --git a/components/sync_device_info/DEPS b/components/sync_device_info/DEPS index d016c1f9f986b7..766edb278831c3 100644 --- a/components/sync_device_info/DEPS +++ b/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", diff --git a/components/sync_device_info/device_info_sync_bridge.cc b/components/sync_device_info/device_info_sync_bridge.cc index 3922345285ee47..93b280268977f7 100644 --- a/components/sync_device_info/device_info_sync_bridge.cc +++ b/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() diff --git a/components/sync_device_info/device_info_sync_bridge.h b/components/sync_device_info/device_info_sync_bridge.h index 97754618646525..d3422bcc6c7dc9 100644 --- a/components/sync_device_info/device_info_sync_bridge.h +++ b/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 delete_metadata_change_list) override; + ModelTypeSyncBridge::CommitAttemptFailedBehavior OnCommitAttemptFailed( + syncer::SyncCommitError commit_error) override; // DeviceInfoTracker implementation. bool IsSyncing() const override;