Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration of duplicated bookmarks for sync (2) #4940

Merged
merged 3 commits into from
Mar 25, 2020

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Mar 13, 2020

consider creation time can be equal; fixes brave-browser#8358

Resolves brave/brave-browser#8358

Submitter Checklist:

Test Plan:

I. Test of object id migration

  1. Build browser without bookmarks metainfo fix and without object_id migration

  2. Enable sync on devices A and B

  3. Create sync chain between devices A and B

  4. On device A create bookmarks A.com, AA.com, AAA.com; wait until they will not appear on device B

  5. Disable sync through flags on device A

  6. Restart browser on device A

  7. On device A select 3 bookmarks, copy and paste them 3 times: before original, inside original and after original

  8. Enable sync through flags on device A

  9. Close browser device A

  10. Build browser with bookmarks metainfo fix and with object_id migration

  11. Open browser device B

  12. Order of bookmarks between A and B must be the same. It takes 10 minutes for changes to appear on device B because of resend.

For step 14 need to wait 10 minutes, because 1st attempt of send updates fails because js lib is not yet initialized, so actual send happens during re-send procedure.

II. Test to ensure orders are ok if bookmarks are moved when sync is off

  1. Create sync chain between A and B
  2. Create on device A these bookmarks on following order:
A1.com
A2.com
A3.com
A4.com
  1. Wait till they would be be synced to device B
  2. Disable sync on device A
  3. Arrange bookmarks on device A as:
A4.com
A2.com
A1.com
A3.com
  1. Enable sync on device A
  2. Expected/Actual: device B to have bookmarks in the same order.

For step 7 need to wait 10 minutes, because 1st attempt of send updates fails because js lib is not yet initialized, so actual send happens during re-send procedure.

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 AlexeyBarabash added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows CI/skip-ios Do not run CI builds for iOS labels Mar 13, 2020
@AlexeyBarabash AlexeyBarabash self-assigned this Mar 13, 2020
@AlexeyBarabash
Copy link
Contributor Author

PR is in progress.
I made it now to get CI - built for Linux deb packages to try it on @fmarier profile.

This PR still needs to

  1. Update testcases for duplicated bookmarks which have equal ctime
  2. Add migration and test for broken bookmarks orders

cc @jsecretan

@AlexeyBarabash AlexeyBarabash force-pushed the sync_dup_obj_id_migration2 branch 2 times, most recently from f0537fc to a4fdcc2 Compare March 18, 2020 15:17
@AlexeyBarabash
Copy link
Contributor Author

CI failed because fetched wrong chromium, 80.0.3987.132 instead of 80.0.3987.149, removed b-b and restarted

@AlexeyBarabash AlexeyBarabash force-pushed the sync_dup_obj_id_migration2 branch 5 times, most recently from 3263d1f to 7007cb1 Compare March 24, 2020 22:14
@AlexeyBarabash AlexeyBarabash changed the title On migration of duplicated bookmarks for sync 2 Migration of duplicated bookmarks for sync (2) Mar 24, 2020
@AlexeyBarabash
Copy link
Contributor Author

This PR was tested on profile of @fmarier , which crashed with previous attempt of fix (#4816).

@AlexeyBarabash AlexeyBarabash added feature/sync and removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Mar 24, 2020
@AlexeyBarabash AlexeyBarabash added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Mar 24, 2020
@AlexeyBarabash AlexeyBarabash added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Mar 24, 2020
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm now

@AlexeyBarabash AlexeyBarabash removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Mar 25, 2020
@AlexeyBarabash
Copy link
Contributor Author

@fmarier verified the last change works good on his broken profile.

CI failed because I didn't update test. Now it is updated. Removed all CI-skip-xxxx labels.

@AlexeyBarabash
Copy link
Contributor Author

macOS build failed with weird error

11:08:00  ../../chrome/common/chrome_content_client.h:17:10: fatal error: 'chrome/common/buildflags.h' file not found
11:08:00  #include "chrome/common/buildflags.h"
11:08:00           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

All other is green: https://ci.brave.com/job/brave-browser-build-pr/job/sync_dup_obj_id_migration2/ .
Skipping all except macOS and restarting.

@AlexeyBarabash AlexeyBarabash added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 25, 2020
@AlexeyBarabash
Copy link
Contributor Author

CI completed with status UNSTABLE
Failed browser-test on macOS

14:41:12  [511/511] BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (TIMED OUT)
14:41:12  1 test timed out:
14:41:12      BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2762)

@AlexeyBarabash
Copy link
Contributor Author

The same error

16:48:22  [511/511] BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (TIMED OUT)
16:48:22  1 test timed out:
16:48:22      BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2762)

@AlexeyBarabash
Copy link
Contributor Author

Removed b-b branch and rebased to get another try with CI.

@AlexeyBarabash AlexeyBarabash merged commit dc75961 into master Mar 25, 2020
@AlexeyBarabash AlexeyBarabash deleted the sync_dup_obj_id_migration2 branch March 25, 2020 19:00
@AlexeyBarabash AlexeyBarabash added this to the 1.8.x - Nightly milestone Mar 25, 2020
bsclifton pushed a commit that referenced this pull request Apr 9, 2020
Migration of duplicated bookmarks for sync (2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS feature/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix desktop profiles which are crashing because of sync object id duplication
2 participants