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

Sync duplicated object id migration (3) #5139

Merged
merged 3 commits into from
Apr 10, 2020
Merged

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Apr 1, 2020

Resolves brave/brave-browser#8358
Resolves brave/brave-browser#9110

Submitter Checklist:

Test Plan:

See brave/brave-browser#8358
Also see brave/brave-browser#9110

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 Apr 1, 2020
@AlexeyBarabash AlexeyBarabash self-assigned this Apr 1, 2020
@AlexeyBarabash AlexeyBarabash changed the title Sync dup obj id migration3 Sync duplicated object id migration (3) Apr 1, 2020
@AlexeyBarabash AlexeyBarabash force-pushed the sync_dup_obj_id_migration3 branch 3 times, most recently from 865ae19 to afc5311 Compare April 9, 2020 11:50
@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Apr 9, 2020
@bsclifton bsclifton self-requested a review April 9, 2020 16:08
@bsclifton bsclifton 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 Apr 9, 2020
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Reviewed changes - works great 😄 I added a check for the Sync feature flag and then added a unit test for this too

@bsclifton bsclifton removed the CI/skip Do not run CI builds (except noplatform) label Apr 9, 2020
@bsclifton bsclifton added this to the 1.9.x - Nightly milestone Apr 9, 2020
@bsclifton bsclifton mentioned this pull request Apr 9, 2020
32 tasks
@AlexeyBarabash
Copy link
Contributor Author

309ea38 LGTM

AlexeyBarabash and others added 2 commits April 9, 2020 11:54
- Prevents brave/brave-browser#9110 (or similar) from happening
- Added unit test to ensure migration not done if sync disabled
@bsclifton
Copy link
Member

CI had a failure; deleted auto-created brave-browser branch, rebased this branch against master, and pushed 👍

@bsclifton
Copy link
Member

CI passed on all platforms, except for Android. Seems there was a signing issue during create_dist:
https://ci.brave.com/job/brave-browser-build-pr/job/sync_dup_obj_id_migration3/9/execution/node/463/log/

@bsclifton bsclifton added CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Apr 9, 2020
@bsclifton
Copy link
Member

Android is building fine, tests pass- there's just a problem with create_dist (and the Jenkins setup). This appears to be happening with other PRs. Should be good to merge! 👍

@iefremov this should fix some of the crashes you've seen reported, I believe 😄

@btlechowski
Copy link

LGTM

Verification passed on

Brave 1.9.15 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
Revision 32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS Ubuntu 18.04 LTS

Verified test plan from #5139

Steps used for brave/brave-browser#8358

  1. Install 1.3.118
  2. Go to about:flags#brave-sync and enable sync
  3. Create a sync chain
  4. Go to brave://bookmarks/
  5. Add a bookmark
  6. Exit browser
  7. Copy content of Fix desktop profiles which are crashing because of sync object id duplication brave-browser#8358 (comment) to Bookmarks file
  8. Upgrade to 1.9.15

Reproduced the crash after upgrade to 1.7.90
Verified no crash after upgrade to 1.9.15

Test case - disabled sync

  1. Install 1.3.118
  2. Go to about:flags#brave-sync and enable sync
  3. Create a sync chain
  4. Go to brave://bookmarks/
  5. Add a bookmark
  6. Exit browser
  7. Copy content of Fix desktop profiles which are crashing because of sync object id duplication brave-browser#8358 (comment) to Bookmarks file
  8. Upgrade to 1.9.15
  9. Run brave with flag --disable-sync

Verified no crash after upgrade to 1.9.15

Verified test plan from brave/brave-browser#9110

Reproduced the crash after upgrade to 1.7.90
Verified no crash after upgrade to 1.9.15
Verified no crash after upgrade to 1.9.15 with flag --disable-sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
4 participants