Merge pull request #1188 from brave/sync_frequent_initial_update

Update devices each 1 second until chain is not created

components/brave_sync/brave_sync_service_impl.cc

@@ -290,7 +290,9 @@ void BraveSyncServiceImpl::BackgroundSyncStarted(bool startup) {
   if (startup)
     bookmark_change_processor_->Start();
 
-  StartLoop();
+  const bool waiting_for_second_device =
+                            !sync_prefs_->GetSyncDevices()->has_second_device();
+  StartLoop(waiting_for_second_device);
 }
 
 void BraveSyncServiceImpl::BackgroundSyncStopped(bool shutdown) {
@@ -466,9 +468,10 @@ BraveSyncServiceImpl::PrepareResolvedPreferences(const RecordsList& records) {
 void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
   const std::string this_device_id = sync_prefs_->GetThisDeviceId();
   bool this_device_deleted = false;
-  bool contains_only_one_device = false;
+  bool at_least_one_deleted = false;
 
   auto sync_devices = sync_prefs_->GetSyncDevices();
+  const bool waiting_for_second_device = !sync_devices->has_second_device();
   for (const auto &record : records) {
     DCHECK(record->has_device() || record->has_sitesetting());
     if (record->has_device()) {
@@ -484,18 +487,20 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
         (record->deviceId == this_device_id &&
           record->action == jslib::SyncRecord::Action::A_DELETE &&
           actually_merged);
-      contains_only_one_device = sync_devices->size() < 2 &&
-        record->action == jslib::SyncRecord::Action::A_DELETE &&
-          actually_merged;
+      at_least_one_deleted = at_least_one_deleted ||
+          (record->action == jslib::SyncRecord::Action::A_DELETE &&
+          actually_merged);
     }
   } // for each device
 
   sync_prefs_->SetSyncDevices(*sync_devices);
-
-  if (this_device_deleted)
+  if (this_device_deleted ||
+                 (at_least_one_deleted && !sync_devices->has_second_device())) {
     ResetSyncInternal();
-  if (contains_only_one_device)
-    OnResetSync();
+  } else if (waiting_for_second_device && sync_devices->has_second_device()) {
+    // Restart loop with 30 sec interval
+    StartLoop(false);
+  }
 }
 
 void BraveSyncServiceImpl::OnSyncPrefsChanged(const std::string& pref) {
@@ -610,10 +615,14 @@ void BraveSyncServiceImpl::SendDeviceSyncRecord(
 }
 
 static const int64_t kCheckUpdatesIntervalSec = 60;
+static const int64_t kCheckInitialUpdatesIntervalSec = 1;
 
-void BraveSyncServiceImpl::StartLoop() {
+void BraveSyncServiceImpl::StartLoop(const bool use_initial_update_interval) {
   timer_->Start(FROM_HERE,
-                  base::TimeDelta::FromSeconds(kCheckUpdatesIntervalSec),
+                  base::TimeDelta::FromSeconds(
+                    use_initial_update_interval ?
+                        kCheckInitialUpdatesIntervalSec :
+                        kCheckUpdatesIntervalSec),
                   this,
                   &BraveSyncServiceImpl::LoopProc);
 }
@@ -640,6 +649,10 @@ void BraveSyncServiceImpl::LoopProcThreadAligned() {
   RequestSyncData();
 }
 
+base::TimeDelta BraveSyncServiceImpl::GetLoopDelay() const {
+  return timer_->GetCurrentDelay();
+}
+
 void BraveSyncServiceImpl::NotifyLogMessage(const std::string& message) {
   DLOG(INFO) << message;
 }

components/brave_sync/brave_sync_service_impl.h

@@ -33,6 +33,7 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSyncReadyNewToSync);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetExistingObjects);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, BackgroundSyncStarted);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, BackgroundSyncStopped);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, LoopDelayVaries);
 
 class BraveSyncServiceTest;
 
@@ -110,6 +111,7 @@ class BraveSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnGetExistingObjects);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BackgroundSyncStarted);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BackgroundSyncStopped);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, LoopDelayVaries);
   friend class ::BraveSyncServiceTest;
 
   // SyncMessageHandler overrides
@@ -157,10 +159,11 @@ class BraveSyncServiceImpl
     const std::string& deviceId,
     const std::string& objectId);
 
-  void StartLoop();
+  void StartLoop(const bool use_initial_update_interval);
   void StopLoop();
   void LoopProc();
   void LoopProcThreadAligned();
+  base::TimeDelta GetLoopDelay() const;  // For tests only
 
   void GetExistingHistoryObjects(
     const RecordsList &records,

components/brave_sync/brave_sync_service_unittest.cc

@@ -526,7 +526,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) {
   auto resolved_record = SyncRecord::Clone(*records.at(0));
   resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
   resolved_records.push_back(std::move(resolved_record));
-  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(5);
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(AtLeast(3));
   sync_service()->OnResolvedPreferences(resolved_records);
 
   auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
@@ -698,3 +698,94 @@ TEST_F(BraveSyncServiceTest, BackgroundSyncStopped) {
   sync_service()->BackgroundSyncStopped(false);
   EXPECT_FALSE(sync_service()->timer_->IsRunning());
 }
+
+TEST_F(BraveSyncServiceTest, LoopDelayVaries) {
+  // This test is almost the same as BraveSyncServiceTest::OnDeleteDevice
+  // Loop delay should be:
+  // 1 sec when there is no sync chain yet
+  // 60 sec when 2 or more devices
+  // after sync chain destroy the loop should not be running
+
+  sync_service()->BackgroundSyncStarted(false);
+
+  EXPECT_TRUE(sync_service()->timer_->IsRunning());
+  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 1u);
+
+  RecordsList records;
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "1", "device1"));
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "2", "device2"));
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "3", "device3"));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
+  sync_service()->OnResolvedPreferences(records);
+
+  sync_service()->sync_prefs_->SetThisDeviceId("1");
+  auto devices = sync_service()->sync_prefs_->GetSyncDevices();
+
+  // Have 3 devices now
+  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 60u);
+
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "3", "device3"));
+
+  using brave_sync::jslib::SyncRecord;
+  EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES",
+      ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device3"))).Times(1);
+  sync_service()->OnDeleteDevice("3");
+
+  RecordsList resolved_records;
+  auto resolved_record = SyncRecord::Clone(*records.at(2));
+  resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record));{
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
+  sync_service()->OnResolvedPreferences(resolved_records);}
+
+  auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
+  EXPECT_TRUE(DevicesContains(devices_final.get(), "1", "device1"));
+  EXPECT_TRUE(DevicesContains(devices_final.get(), "2", "device2"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
+
+  // Have 2 devices now, still expecting 60 sec delay
+  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 60u);
+
+  // Delete all remaining devices
+  resolved_records.clear();
+
+  auto resolved_record_0 = SyncRecord::Clone(*records.at(0));
+  resolved_record_0->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record_0));
+
+  auto resolved_record_1 = SyncRecord::Clone(*records.at(1));
+  resolved_record_1->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record_1));
+
+  {
+    // Expecting call of OnSyncStateChanged at least 3 times:
+    // delete "device1"
+    // delete "device2"
+    // brave_sync.enabled to false, several times
+    EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(AtLeast(3));
+    EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1));
+    sync_service()->OnResolvedPreferences(resolved_records);
+  }
+
+  devices_final = sync_service()->sync_prefs_->GetSyncDevices();
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
+
+  // We have mock client, so emulate these steps when all devices removed
+  // 1) brave_sync.enabled goes to false
+  // 2) BraveSyncClientImpl::OnSyncEnabledChanged
+  // 3) BraveSyncClientImpl::LoadOrUnloadExtension(false)
+  // 4) BraveSyncClientImpl::OnExtensionUnloaded
+  // 5) BraveSyncServiceImpl::BackgroundSyncStopped
+  sync_service()->BackgroundSyncStopped(true);
+  EXPECT_FALSE(sync_service()->timer_->IsRunning());
+}

components/brave_sync/sync_devices.h

@@ -43,6 +43,7 @@ class SyncDevices {
    std::unique_ptr<base::Value> ToValueArrOnly() const;
    std::string ToJson() const;
    size_t size() const { return devices_.size(); }
+   bool has_second_device() const { return size() >= 2; }
    void FromJson(const std::string &str_json);
    void Merge(const SyncDevice& device, int action, bool* actually_merged);