Sync reset if browser was closed before the chain was fully created #3730
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,6 +121,7 @@ std::unique_ptr<SyncRecord> CreateDeleteBookmarkByObjectId( | |
| const std::string& object_id) { | ||
| auto record = std::make_unique<SyncRecord>(); | ||
| record->deviceId = brave_sync_prefs->GetThisDeviceId(); | ||
| DCHECK(!record->deviceId.empty()); | ||
| record->objectData = jslib_const::SyncObjectData_BOOKMARK; | ||
| record->objectId = object_id; | ||
| record->action = jslib::SyncRecord::Action::A_DELETE; | ||
|
|
@@ -168,11 +169,39 @@ BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile, | |
| : BraveProfileSyncService(std::move(init_params)), | ||
| brave_sync_client_(BraveSyncClient::Create(this, profile)) { | ||
| brave_sync_words_ = std::string(); | ||
| brave_sync_prefs_disk_store_ = | ||
| std::make_unique<prefs::Prefs>(sync_client_->GetPrefService()); | ||
| brave_sync_prefs_ = brave_sync_prefs_disk_store_.get(); | ||
|
|
||
| if (IsSyncSetupIncomplete()) { | ||
| // We want to reset the chain because it was not fully created | ||
| // We cannot make it here directly, because some sync classes may not be | ||
| // constructed yet | ||
| reseting_ = true; | ||
|
There was a problem hiding this comment. Is this to trigger There was a problem hiding this comment.
yes but if we don't reset devices list, UI page will never trigger BraveProfileSyncServiceImpl::OnSetupSyncHaveCode because device name is not empty https://github.com/brave/brave-core/blob/master/components/brave_sync/ui/components/modals/enterSyncCode.tsx#L92 so instead of clearing all prefs it would be enough co clean only the current device name, but I decided to clean all non-actual data |
||
| // Need to clear prefs to allow UI page give us code words | ||
| brave_sync_prefs_->Clear(); | ||
| } | ||
|
|
||
| // Moniter syncs prefs required in GetSettingsAndDevices | ||
| brave_pref_change_registrar_.Init(sync_client_->GetPrefService()); | ||
|
|
||
| model_ = BookmarkModelFactory::GetForBrowserContext(profile); | ||
|
|
||
| if (!brave_sync_prefs_->GetSeed().empty() && | ||
| !brave_sync_prefs_->GetThisDeviceName().empty()) { | ||
| brave_sync_configured_ = true; | ||
| SetupDiskPrefsObservers(); | ||
| } else { | ||
| brave_sync_prefs_ = &brave_sync_prefs_mem_store_; | ||
| brave_sync_prefs_mem_store_.AddObserver(base::BindRepeating( | ||
| &BraveProfileSyncServiceImpl::OnBraveSyncPrefsChanged, | ||
| base::Unretained(this))); | ||
| } | ||
| network_connection_tracker_->AddNetworkConnectionObserver(this); | ||
| RecordSyncStateP3A(); | ||
| } | ||
|
|
||
| void BraveProfileSyncServiceImpl::SetupDiskPrefsObservers() { | ||
| brave_pref_change_registrar_.Add( | ||
| prefs::kSyncEnabled, | ||
| base::Bind(&BraveProfileSyncServiceImpl::OnBraveSyncPrefsChanged, | ||
|
|
@@ -197,15 +226,6 @@ BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile, | |
| prefs::kSyncHistoryEnabled, | ||
| base::Bind(&BraveProfileSyncServiceImpl::OnBraveSyncPrefsChanged, | ||
| base::Unretained(this))); | ||
| } | ||
|
|
||
| void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) { | ||
|
|
@@ -214,6 +234,7 @@ void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) { | |
|
|
||
| for (auto& record : *records) { | ||
| record->deviceId = brave_sync_prefs_->GetThisDeviceId(); | ||
| DCHECK(!record->deviceId.empty()); | ||
| } | ||
| if (!records->empty()) { | ||
| if (((!brave_sync::tools::IsTimeEmpty(chain_created_time_) && | ||
|
|
@@ -232,6 +253,17 @@ BraveProfileSyncServiceImpl::~BraveProfileSyncServiceImpl() { | |
| network_connection_tracker_->RemoveNetworkConnectionObserver(this); | ||
| } | ||
|
|
||
| bool BraveProfileSyncServiceImpl::IsSyncSetupIncomplete() const { | ||
| auto devices = brave_sync_prefs_->GetSyncDevices(); | ||
| if (devices->size() < 2 && | ||
|
There was a problem hiding this comment. sync setup is not complete is more than one device? There was a problem hiding this comment. yes, sync setup is not complete when have less than 2 devices |
||
| (brave_sync_prefs_->GetSyncEnabled() || | ||
| !brave_sync_prefs_->GetSeed().empty() || | ||
| !brave_sync_prefs_->GetThisDeviceName().empty())) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| void BraveProfileSyncServiceImpl::OnSetupSyncHaveCode( | ||
| const std::string& sync_words, | ||
| const std::string& device_name) { | ||
|
|
@@ -320,6 +352,7 @@ void BraveProfileSyncServiceImpl::OnResetSync() { | |
| // We have to send delete record and wait for library deleted response then | ||
| // we can reset it by ResetSyncInternal() | ||
| const std::string device_id = brave_sync_prefs_->GetThisDeviceId(); | ||
| DCHECK(!device_id.empty()); | ||
| OnDeleteDevice(device_id); | ||
|
There was a problem hiding this comment. why do we care about deleting devices if we're resetting sync? |
||
| reseting_ = true; | ||
| } | ||
|
|
@@ -671,6 +704,11 @@ void BraveProfileSyncServiceImpl::ResetSyncInternal() { | |
| void BraveProfileSyncServiceImpl::ForceCompleteReset() { | ||
| if (reseting_) { | ||
| ResetSyncInternal(); | ||
| } else { | ||
| // Reset only Chromium part | ||
| ProfileSyncService::GetUserSettings()->SetSyncRequested(false); | ||
| syncer::SyncPrefs sync_prefs(sync_client_->GetPrefService()); | ||
| sync_prefs.SetLastSyncedTime(base::Time()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -859,6 +897,8 @@ void BraveProfileSyncServiceImpl::SendDeviceSyncRecord( | |
| void BraveProfileSyncServiceImpl::OnResolvedPreferences( | ||
| const RecordsList& records) { | ||
| const std::string this_device_id = brave_sync_prefs_->GetThisDeviceId(); | ||
| DCHECK(!this_device_id.empty()); | ||
|
|
||
| bool this_device_deleted = false; | ||
| bool contains_only_one_device = false; | ||
|
|
||
|
|
@@ -887,6 +927,7 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences( | |
| if (old_devices_size < 2 && sync_devices->size() >= 2) { | ||
| // Save chain creation time to send bookmarks 30 sec after | ||
| chain_created_time_ = base::Time::Now(); | ||
| SwitchToDiskPrefs(); | ||
| } | ||
| if (!tools::IsTimeEmpty(chain_created_time_) && | ||
| (base::Time::Now() - chain_created_time_).InSeconds() > 30u) { | ||
|
|
@@ -906,7 +947,8 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences( | |
| void BraveProfileSyncServiceImpl::OnBraveSyncPrefsChanged( | ||
| const std::string& pref) { | ||
| if (pref == prefs::kSyncEnabled) { | ||
| bool is_enabled = brave_sync_prefs_->GetSyncEnabled(); | ||
| brave_sync_client_->OnSyncEnabledChanged(is_enabled); | ||
| RecordSyncStateP3A(); | ||
| } else if (pref == prefs::kSyncDeviceList) { | ||
| RecordSyncStateP3A(); | ||
|
|
@@ -1060,8 +1102,8 @@ void BraveProfileSyncServiceImpl::ResendSyncRecords( | |
| if (node) { | ||
| records->push_back(BookmarkNodeToSyncBookmark(node)); | ||
| } else { | ||
| records->push_back(CreateDeleteBookmarkByObjectId( | ||
| brave_sync_prefs_disk_store_.get(), object_id)); | ||
| } | ||
| } | ||
| if (!records->empty()) | ||
|
|
@@ -1098,4 +1140,13 @@ void BraveProfileSyncServiceImpl::RecordSyncStateP3A() const { | |
| UMA_HISTOGRAM_EXACT_LINEAR("Brave.Sync.Status", result, 2); | ||
| } | ||
|
|
||
| void BraveProfileSyncServiceImpl::SwitchToDiskPrefs() { | ||
| brave_sync_prefs_ = brave_sync_prefs_disk_store_.get(); | ||
| brave_sync::prefs::CloneMemPrefsToDisk(&brave_sync_prefs_mem_store_, | ||
| brave_sync_prefs_disk_store_.get()); | ||
| SetupDiskPrefsObservers(); | ||
| brave_sync_prefs_mem_store_.ResetObserver(); | ||
| brave_sync_prefs_mem_store_.Clear(); | ||
| } | ||
|
|
||
| } // namespace brave_sync | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having
IsBraveSyncInitialized,IsBraveSyncConfiguredandIsSyncSetupIncompleteis just way too confusing. We need to simplify the possible states and add comments to explain what they are. Also we shouldn't mix the "positive" and "negative" forms of checks because that's also confusing to read.IsSyncSetupCompleteis the "positive" form vsIsSyncSetupIncomplete