From f5429817b445b2c8bede1e5e03114dc808ed6320 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 10 Sep 2021 17:45:31 +0300 Subject: [PATCH] Leave sync chain after 5 seconds without ack response fixes brave/brave-browser#18054 --- .../device_info_sync_bridge.cc | 10 ++- .../device_info_sync_bridge.h | 10 +-- .../device_info_sync_bridge_unittest.cc | 71 +++++++++++++++++++ 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/chromium_src/components/sync_device_info/device_info_sync_bridge.cc b/chromium_src/components/sync_device_info/device_info_sync_bridge.cc index 4bc72b741b43..f91f7d7c54c6 100644 --- a/chromium_src/components/sync_device_info/device_info_sync_bridge.cc +++ b/chromium_src/components/sync_device_info/device_info_sync_bridge.cc @@ -25,6 +25,8 @@ namespace syncer { namespace { +const int kFailedAttemtpsToAckDeviceDelete = 5; + std::unique_ptr BraveSpecificsToModel( const DeviceInfoSpecifics& specifics) { ModelTypeSet data_types; @@ -67,19 +69,21 @@ void DeviceInfoSyncBridge::DeleteDeviceInfo(const std::string& client_id, base::SequencedTaskRunnerHandle::Get()->PostDelayedTask( FROM_HERE, base::BindOnce(&DeviceInfoSyncBridge::OnDeviceInfoDeleted, - weak_ptr_factory_.GetWeakPtr(), client_id, + weak_ptr_factory_.GetWeakPtr(), client_id, 1, std::move(callback)), base::TimeDelta::FromSeconds(1)); } void DeviceInfoSyncBridge::OnDeviceInfoDeleted(const std::string& client_id, + const int attempt, base::OnceClosure callback) { // Make sure the deleted device info is sent - if (change_processor()->IsEntityUnsynced(client_id)) { + if (change_processor()->IsEntityUnsynced(client_id) && + attempt < kFailedAttemtpsToAckDeviceDelete) { base::SequencedTaskRunnerHandle::Get()->PostDelayedTask( FROM_HERE, base::BindOnce(&DeviceInfoSyncBridge::OnDeviceInfoDeleted, - weak_ptr_factory_.GetWeakPtr(), client_id, + weak_ptr_factory_.GetWeakPtr(), client_id, attempt + 1, std::move(callback)), base::TimeDelta::FromSeconds(1)); } else { diff --git a/chromium_src/components/sync_device_info/device_info_sync_bridge.h b/chromium_src/components/sync_device_info/device_info_sync_bridge.h index 13f5042b9dbb..7567b048ddfa 100644 --- a/chromium_src/components/sync_device_info/device_info_sync_bridge.h +++ b/chromium_src/components/sync_device_info/device_info_sync_bridge.h @@ -12,8 +12,8 @@ #define ForcePulseForTest \ DeleteDeviceInfo(const std::string& client_id, base::OnceClosure callback) \ override; \ - std::vector> \ - GetAllBraveDeviceInfo() const override; \ + std::vector> GetAllBraveDeviceInfo() \ + const override; \ void ForcePulseForTest #define RefreshLocalDeviceInfoIfNeeded \ @@ -21,9 +21,9 @@ void RefreshLocalDeviceInfoIfNeeded // private: -#define StoreSpecifics \ - OnDeviceInfoDeleted(const std::string& client_id, \ - base::OnceClosure callback); \ +#define StoreSpecifics \ + OnDeviceInfoDeleted(const std::string& client_id, const int attempt, \ + base::OnceClosure callback); \ void StoreSpecifics #include "../../../../components/sync_device_info/device_info_sync_bridge.h" diff --git a/chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc b/chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc index 514dff0d96d2..b4244f297d37 100644 --- a/chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc +++ b/chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc @@ -4,9 +4,39 @@ // you can obtain one at http://mozilla.org/MPL/2.0/. #include "base/system/sys_info.h" +#include "base/test/task_environment.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace base { +namespace test { +namespace { + +// This trick substitutes TaskEnvironment to make the test +// LocalDeleteWhenOffline avoid wait 6 sec for real +class TaskEnvironmentOptionalMockTime : public TaskEnvironment { + public: + TaskEnvironmentOptionalMockTime() + : TaskEnvironment( + IsMockTimedTest( + testing::UnitTest::GetInstance()->current_test_info()->name()) + ? TimeSource::MOCK_TIME + : TimeSource::DEFAULT) {} + + static bool IsMockTimedTest(base::StringPiece test_name) { + return test_name == "LocalDeleteWhenOffline"; + } +}; + +} // namespace +} // namespace test +} // namespace base + +#define TaskEnvironment TaskEnvironmentOptionalMockTime #include "../../../../components/sync_device_info/device_info_sync_bridge_unittest.cc" +#undef TaskEnvironment + namespace syncer { namespace { @@ -84,5 +114,46 @@ TEST_F(DeviceInfoSyncBridgeTest, RemoteDelete) { EXPECT_THAT(ReadAllFromStore(), UnorderedElementsAre(Pair(kLocalGuid, _))); } +TEST_F(DeviceInfoSyncBridgeTest, LocalDeleteWhenOffline) { + InitializeAndMergeInitialData(SyncMode::kFull); + ASSERT_EQ(1u, bridge()->GetAllDeviceInfo().size()); + ASSERT_EQ(1, change_count()); + ASSERT_FALSE(ReadAllFromStore().empty()); + + const DeviceInfoSpecifics specifics = CreateSpecifics(1, base::Time::Now()); + auto error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(), + EntityAddList({specifics})); + + ASSERT_FALSE(error); + ASSERT_EQ(2u, bridge()->GetAllDeviceInfo().size()); + ASSERT_EQ(2, change_count()); + + const std::string kLocalGuid = CacheGuidForSuffix(kLocalSuffix); + ON_CALL(*processor(), IsEntityUnsynced(kLocalGuid)) + .WillByDefault(Return(true)); + // The statement below means that DeviceInfoSyncBridge::OnDeviceInfoDeleted + // 5 times did check of IsEntityUnsynced for the entity being deleted + EXPECT_CALL(*processor(), IsEntityUnsynced).Times(5); + EXPECT_CALL(*processor(), Delete(kLocalGuid, _)).Times(1); + + bool deleted_device_info_sent = false; + base::RunLoop loop; + bridge()->DeleteDeviceInfo( + kLocalGuid, base::BindOnce( + [](base::RunLoop* loop, bool* deleted_device_info_sent) { + *deleted_device_info_sent = true; + loop->Quit(); + }, + &loop, &deleted_device_info_sent)); + + loop.Run(); + + EXPECT_TRUE(deleted_device_info_sent); + EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size()); + ASSERT_EQ(3, change_count()); + EXPECT_THAT(ReadAllFromStore(), + UnorderedElementsAre(Pair(specifics.cache_guid(), _))); +} + } // namespace } // namespace syncer