From e57ff14e4760db555de7ffee347fc69ab613dd76 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Wed, 27 Nov 2019 16:58:46 +0200 Subject: [PATCH 1/2] Reset sync when offline: call ResetSyncInternal from OnRecordsSent --- .../brave_sync/brave_profile_sync_service_impl.cc | 13 +++++++++++-- .../brave_sync/brave_profile_sync_service_impl.h | 4 ++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 9cf46edeb690..ecce593d8b7b 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -289,6 +289,12 @@ void BraveProfileSyncServiceImpl::OnDeleteDevice(const std::string& device_id) { const std::string object_id = device->object_id_; SendDeviceSyncRecord(SyncRecord::Action::A_DELETE, device_name, device_id, object_id); + if (device_id == brave_sync_prefs_->GetThisDeviceId()) { + // Mark state we have sent DELETE for own device and we are going to + // call ResetSyncInternal() at OnRecordsSent after ensuring we had made + // a proper attemp to send the record + pending_self_reset_ = true; + } FetchDevices(); } } @@ -578,11 +584,14 @@ void BraveProfileSyncServiceImpl::OnCompactComplete( void BraveProfileSyncServiceImpl::OnRecordsSent( const std::string& category, std::unique_ptr records) { - for (auto& record : *records) { - if (category == kBookmarks) { + if (category == kBookmarks) { + for (auto& record : *records) { // Remove Acked sent records brave_sync_prefs_->RemoveFromRecordsToResend(record->objectId); } + } else if (category == kPreferences && pending_self_reset_) { + ResetSyncInternal(); + pending_self_reset_ = false; } } diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index 99e3b370b646..a1adc0882211 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -28,6 +28,7 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDevice); FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice); FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted); FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnResetSync); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnResetSyncWhenOffline); FORWARD_DECLARE_TEST(BraveSyncServiceTest, ClientOnGetInitData); FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetInitData); FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSaveBookmarksBaseOrder); @@ -140,6 +141,7 @@ class BraveProfileSyncServiceImpl FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnResetSync); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnResetSyncWhenOffline); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, ClientOnGetInitData); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSaveBookmarksBaseOrder); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnGetInitData); @@ -237,6 +239,8 @@ class BraveProfileSyncServiceImpl // Time when current device sent CREATE device record base::Time this_device_created_time_; + bool pending_self_reset_ = false; + // Used to ensure that certain operations are performed on the sequence that // this object was created on. SEQUENCE_CHECKER(sequence_checker_); From 1e0c3380bf4c893c90e7624f4c09d716488c6487 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Wed, 27 Nov 2019 16:59:23 +0200 Subject: [PATCH 2/2] Testcase for resetting sync when offline --- .../brave_sync/brave_sync_service_unittest.cc | 133 ++++++++++++------ 1 file changed, 93 insertions(+), 40 deletions(-) diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 4d6f7f7ee310..eae5d5de659b 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -224,6 +224,9 @@ class BraveSyncServiceTest : public testing::Test { } syncer::SyncPrefs* sync_prefs() { return sync_prefs_.get(); } + // Common part of OnResetSync and OnResetSyncWhenOffline tests + void VerifyResetDone(); + private: // Need this as a very first member to run tests in UI thread // When this is set, class should not install any other MessageLoops, like @@ -530,7 +533,54 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2")); } +void BraveSyncServiceTest::VerifyResetDone() { + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetString(brave_sync::prefs::kSyncDeviceId) + .empty()); + EXPECT_TRUE( + profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncSeed).empty()); + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetString(brave_sync::prefs::kSyncDeviceName) + .empty()); + EXPECT_FALSE( + profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncBookmarksEnabled)); + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetString(brave_sync::prefs::kSyncBookmarksBaseOrder) + .empty()); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncSiteSettingsEnabled)); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncHistoryEnabled)); + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetTime(brave_sync::prefs::kSyncLatestRecordTime) + .is_null()); + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetTime(brave_sync::prefs::kSyncLastFetchTime) + .is_null()); + EXPECT_TRUE(profile() + ->GetPrefs() + ->GetString(brave_sync::prefs::kSyncDeviceList) + .empty()); + EXPECT_EQ( + profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncApiVersion), + "0"); + + EXPECT_FALSE(sync_service()->brave_sync_ready_); + + EXPECT_FALSE(sync_prefs()->IsSyncRequested()); + EXPECT_FALSE( + profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks)); +} + TEST_F(BraveSyncServiceTest, OnResetSync) { + using brave_sync::jslib_const::kPreferences; EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())) .Times(AtLeast(3)); @@ -553,7 +603,7 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1")); EXPECT_CALL(*sync_client(), - SendSyncRecords("PREFERENCES", + SendSyncRecords(kPreferences, ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "this_device"))) .Times(1); @@ -571,49 +621,52 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { EXPECT_FALSE(DevicesContains(devices_final.get(), "0", "this_device")); EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1")); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetString(brave_sync::prefs::kSyncDeviceId) - .empty()); + VerifyResetDone(); +} + +TEST_F(BraveSyncServiceTest, OnResetSyncWhenOffline) { + using brave_sync::jslib_const::kPreferences; + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())) + .Times(AtLeast(3)); + sync_service()->OnSetupSyncNewToSync("this_device"); EXPECT_TRUE( - profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncSeed).empty()); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetString(brave_sync::prefs::kSyncDeviceName) - .empty()); - EXPECT_FALSE( profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled)); - EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( - brave_sync::prefs::kSyncBookmarksEnabled)); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetString(brave_sync::prefs::kSyncBookmarksBaseOrder) - .empty()); - EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( - brave_sync::prefs::kSyncSiteSettingsEnabled)); - EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( - brave_sync::prefs::kSyncHistoryEnabled)); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetTime(brave_sync::prefs::kSyncLatestRecordTime) - .is_null()); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetTime(brave_sync::prefs::kSyncLastFetchTime) - .is_null()); - EXPECT_TRUE(profile() - ->GetPrefs() - ->GetString(brave_sync::prefs::kSyncDeviceList) - .empty()); - EXPECT_EQ( - profile()->GetPrefs()->GetString(brave_sync::prefs::kSyncApiVersion), - "0"); + brave_sync_prefs()->SetThisDeviceId("0"); - EXPECT_FALSE(sync_service()->brave_sync_ready_); + RecordsList records; + records.push_back( + SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "0", "this_device")); + records.push_back( + SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "1", "device1")); - EXPECT_FALSE(sync_prefs()->IsSyncRequested()); - EXPECT_FALSE( - profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks)); + sync_service()->OnResolvedPreferences(records); + + auto devices = brave_sync_prefs()->GetSyncDevices(); + + EXPECT_TRUE(DevicesContains(devices.get(), "0", "this_device")); + EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1")); + + EXPECT_CALL(*sync_client(), + SendSyncRecords(kPreferences, + ContainsDeviceRecord(SyncRecord::Action::A_DELETE, + "this_device"))) + .Times(1); + + EXPECT_CALL(*sync_client(), SendFetchSyncRecords); + EXPECT_FALSE(sync_service()->pending_self_reset_); + sync_service()->OnResetSync(); + EXPECT_TRUE(sync_service()->pending_self_reset_); + + auto recordsSent = std::make_unique(); + sync_service()->OnRecordsSent(kPreferences, std::move(recordsSent)); + EXPECT_FALSE(sync_service()->pending_self_reset_); + + auto devices_final = brave_sync_prefs()->GetSyncDevices(); + EXPECT_FALSE(DevicesContains(devices_final.get(), "0", "this_device")); + EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1")); + + VerifyResetDone(); } TEST_F(BraveSyncServiceTest, OnSetSyncBookmarks) {