Sync reset if browser was closed before the chain was fully created #3730

AlexeyBarabash · 2019-10-17T09:37:39Z

brave/brave-browser#6504

Submitter Checklist:

Test Plan:

From brave/brave-browser#6504

Prepare clear profiles deviceA and deviceB
Launch profile A, create new sync chain, ensure sync code is shown, but don't copy the code. Close deviceA.
Launch profile B, create new sync chain, copy sync code, don't close.
Launch profile A, enter sync code

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

AlexeyBarabash · 2019-10-17T12:11:57Z

CI marked this PR as UNSTABLE because BraveSystemRequestHandlerTest.AddBraveServiceKeyHeader had not passed

darkdh

Close browser is not the only way to interrupt sync chain setup process, closing sync page can do that. So we might also want to do the check when SyncUI is closed.
That needs ~SyncUIDOMHandler to inform BraveProfileSyncServiceImpl by OnSyncPageClosed and then we can do the check there. We can even only check incomplete sync chain setup there because when browser shutdown, the destructor will also be triggered.
It is also better to do this in shutdown rather than startup to prevent unnecessary loading sync extension.

darkdh · 2019-10-17T16:55:09Z

components/brave_sync/brave_profile_sync_service_impl.cc

+    // 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;


Is this to trigger ResetSyncInternal() by ForceCompleteReset()?
If so, then we don't need to clear pref here https://github.com/brave/brave-core/pull/3730/files#diff-ab344835b09fb6ac1c601af787b116bbR184

Is this to trigger ResetSyncInternal() by ForceCompleteReset()?

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

AlexeyBarabash · 2019-10-17T19:21:58Z

Yes, also we can have this situation when browser process is killed or after power failure. So I don't want remove this check at startup.

Thanks for pointing to more elegant way in ~SyncUIDOMHandler , will expand the PR.

AlexeyBarabash · 2019-10-21T14:20:41Z

I had made reset of sync prefs at BraveProfileSyncServiceImpl::Shutdown.

WebUI pages, if they passed sync words, and then are closed, they don't prevent the chain from creation. Another thing - we can have more than one opened brave://sync pages.

STR(1) to see the tab of brave://sync can be closed while chain creation.

1. Prepare clear profiles deviceA and deviceB
2. Launch profile A, create new sync chain, copy the sync code
3. Launch profile B, connect to the sync chain, paste sync code, don't wait for completion of chain creation, close the brave://sync tab.
4. On device A the brave://sync page shows two devices in chain
5. On device B open brave://sync tab and see two devices in chain

STR(2) to show an interesting case

1. Prepare clear profiles deviceA and deviceB
2. Launch profile A, create new sync chain, copy the sync code
3. Launch profile B, connect to the sync chain, paste sync code, press connect
4. When there will be two devices on device B, close device A
5. Launch device A, it is important the browser build don't have the commit of 2e5df197b0e53e612e9f75240132c8e94f53a1ba . The page brave://sync will show the sync is not initialized
6. Wait and after ~ 1 minute device A will show the sync chain is created with devices A and B

So with STR(2) the chain will be created. But we cannot distinguish such case and in the worst case the state of sync will be broken. So I made reset of sync prefs at BraveProfileSyncServiceImpl::Shutdown.

darkdh

for the brave://sync closed case, I meant

create sync chain on device A
don't connect any device to device A
close brave://sync on device A
And sync is still enabled with only 1 device on sync chain even though sync UI is showing

components/brave_sync/brave_profile_sync_service_impl.cc

AlexeyBarabash · 2019-10-21T18:35:44Z

Ah, you meant that.

IsSyncSetupIncomplete sounds better, modified, thanks.

…when browser starts

…tart

…led but the chain is not fully created

…when the chain is not fully created

AlexeyBarabash · 2019-11-05T11:25:42Z

Updated the PR to keep brave sync prefs in memory until the sync chain would have two or more devices.

…ly created

darkdh · 2019-11-05T21:51:01Z

components/brave_sync/brave_sync_prefs_mem_store.h

+  // We are using latest device record until chain is not fully created
+  base::Time latest_device_record_time_;
+
+  NamedChangeCallback obs_;


only allows one observer?

Yes, as it is used only by one method in BraveProfileSyncServiceImpl, I decided to keep just one.

darkdh · 2019-11-05T21:51:33Z

components/brave_sync/brave_sync_prefs_mem_store.h

+  base::Time latest_device_record_time_;
+
+  NamedChangeCallback obs_;
+  void FireCallback(const std::string& name);


This should be NotifyObserver or NotifyObservers

I agree, thanks

darkdh · 2019-11-05T21:56:58Z

components/brave_sync/brave_sync_prefs_mem_store.h

+
+namespace prefs {
+
+class PrefsMemStore : public PrefsBase {


why do we need PrefsBase? PrefsMemStore can be a subclass of Prefs and you can still do the switch.

Yes, it was not required, but since PrefsMemStore would override all methods of Prefs, I did as it is there now.

bridiver · 2019-11-05T22:10:00Z

components/brave_sync/brave_profile_sync_service_impl.cc

      std::make_unique<prefs::Prefs>(sync_client_->GetPrefService());
+  brave_sync_prefs_ = brave_sync_prefs_disk_store_.get();
+
+  if (IsSyncSetupIncomplete()) {


having IsBraveSyncInitialized, IsBraveSyncConfigured and IsSyncSetupIncomplete is 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. IsSyncSetupComplete is the "positive" form vs IsSyncSetupIncomplete

bridiver · 2019-11-05T22:10:50Z

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -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 &&


sync setup is not complete is more than one device?

yes, sync setup is not complete when have less than 2 devices

bridiver · 2019-11-05T22:13:23Z

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -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);


why do we care about deleting devices if we're resetting sync?

darkdh

To summarize the discussion on Slack,

We should only have states like enabled/initializing/initialized/ready
Getting rids of brave_sync::Prefs class because it's mostly just calling get/set on the pref service anyway
Only state that has transient data is initializing (seed and device id)
Getting rids of all two devices check, ex. waiting for 2nd device when setup, reset sync chain when it comes down to 1 device, ... etc. Let UI flow handle the "at two devices on sync chain" requirement

AlexeyBarabash · 2019-11-06T13:31:10Z

PR requires a lot of changes, so the reviewed points will be removed or redone.

AlexeyBarabash · 2019-11-13T22:08:02Z

Closing this, as prefs are redone in #3988 according to #3730 (review)

AlexeyBarabash added the feature/sync label Oct 17, 2019

AlexeyBarabash requested review from darkdh and mkarolin October 17, 2019 09:37

AlexeyBarabash self-assigned this Oct 17, 2019

darkdh reviewed Oct 17, 2019

View reviewed changes

AlexeyBarabash force-pushed the sync_reset_issue_6504 branch from 79757a8 to 9450b81 Compare October 21, 2019 14:14

AlexeyBarabash requested a review from darkdh October 21, 2019 14:20

darkdh reviewed Oct 21, 2019

View reviewed changes

components/brave_sync/brave_profile_sync_service_impl.cc Outdated Show resolved Hide resolved

AlexeyBarabash force-pushed the sync_reset_issue_6504 branch from 9450b81 to 836ecff Compare October 21, 2019 18:33

AlexeyBarabash requested review from darkdh and bridiver October 21, 2019 18:36

AlexeyBarabash force-pushed the sync_reset_issue_6504 branch from 836ecff to 07d9e40 Compare October 31, 2019 09:24

AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Oct 31, 2019

AlexeyBarabash added 4 commits November 5, 2019 13:14

Mark for sync chain reset and clear prefs if chain contains 1 device …

62b691a

…when browser starts

Testcase for chain reset if we have one device left after browser res…

3e479d7

…tart

Resetting sync in BraveProfileSyncServiceImpl::Shutdown if it is enab…

0a26226

…led but the chain is not fully created

Testcase for resetting sync at BraveProfileSyncServiceImpl::Shutdown …

d235975

…when the chain is not fully created

AlexeyBarabash force-pushed the sync_reset_issue_6504 branch from 9324f5b to 4bed52e Compare November 5, 2019 11:18

AlexeyBarabash added enhancement and removed CI/skip Do not run CI builds (except noplatform) labels Nov 5, 2019

AlexeyBarabash added 2 commits November 5, 2019 16:43

Save sync prefs in memory if chain is not fully created

b560dc4

Updated testcases for saving sync prefs in memory if chain is not ful…

99a262b

…ly created

AlexeyBarabash force-pushed the sync_reset_issue_6504 branch from 4bed52e to 99a262b Compare November 5, 2019 14:46

darkdh reviewed Nov 5, 2019

View reviewed changes

bridiver reviewed Nov 5, 2019

View reviewed changes

darkdh reviewed Nov 6, 2019

View reviewed changes

AlexeyBarabash closed this Nov 13, 2019

AlexeyBarabash deleted the sync_reset_issue_6504 branch November 14, 2019 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync reset if browser was closed before the chain was fully created #3730

Sync reset if browser was closed before the chain was fully created #3730

AlexeyBarabash commented Oct 17, 2019

AlexeyBarabash commented Oct 17, 2019

darkdh left a comment

darkdh Oct 17, 2019

AlexeyBarabash Oct 17, 2019 •

edited

Loading

AlexeyBarabash commented Oct 17, 2019

AlexeyBarabash commented Oct 21, 2019

darkdh left a comment

AlexeyBarabash commented Oct 21, 2019

AlexeyBarabash commented Nov 5, 2019

darkdh Nov 5, 2019

AlexeyBarabash Nov 6, 2019

darkdh Nov 5, 2019

AlexeyBarabash Nov 6, 2019

darkdh Nov 5, 2019

AlexeyBarabash Nov 6, 2019

bridiver Nov 5, 2019

bridiver Nov 5, 2019

AlexeyBarabash Nov 6, 2019

bridiver Nov 5, 2019

darkdh left a comment

AlexeyBarabash commented Nov 6, 2019

AlexeyBarabash commented Nov 13, 2019

Sync reset if browser was closed before the chain was fully created #3730

Sync reset if browser was closed before the chain was fully created #3730

Conversation

AlexeyBarabash commented Oct 17, 2019

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

AlexeyBarabash commented Oct 17, 2019

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

AlexeyBarabash commented Oct 17, 2019

AlexeyBarabash commented Oct 21, 2019

darkdh left a comment

Choose a reason for hiding this comment

AlexeyBarabash commented Oct 21, 2019

AlexeyBarabash commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

AlexeyBarabash commented Nov 6, 2019

AlexeyBarabash commented Nov 13, 2019

AlexeyBarabash Oct 17, 2019 •

edited

Loading