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

Do not generate dummy originator_cache_guid and random originator_client_item_id in GetUpdatesResponse #4623

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Feb 13, 2020

to prevent ReplaceBookmarkNodeGUID from triggering for every updates.
We will see "Update is missing requirements for bookmark position."
error in debug build though but it is ok because we currently don't
rely on UniquePosition to decide bookmark position. And we don't
propagate correct value of these two ids between devices.

Resolves brave/brave-browser#8228

Submitter Checklist:

Test Plan:

  1. Go to about:flags#brave-sync to enable it on device A and device B
  2. Sync device A and device B
  3. Create bookmark A1, A2 on device A
  4. When they show up on device B
  5. Reorder them to be A2, A1 (drag A1 or A2)
  6. Changes should be reflected on device A without crash

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.

@darkdh darkdh self-assigned this Feb 13, 2020
@darkdh darkdh force-pushed the replace-bookmark-node-guid-crash branch from 059339c to e098083 Compare February 13, 2020 04:36
…client_item_id` in GetUpdatesResponse

to prevent ReplaceBookmarkNodeGUID from triggering for every updates.
We will see "Update is missing requirements for bookmark position."
error in debug build though but it is ok because we currently don't
rely on UniquePosition to decide bookmark position. And we don't
propagate correct value of these two ids between devices.
@darkdh darkdh force-pushed the replace-bookmark-node-guid-crash branch from e098083 to 3356c7c Compare February 13, 2020 04:45
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

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

++
I verified test plan for this PR, but I couldn't see the crash with the same steps on master

@darkdh
Copy link
Member Author

darkdh commented Feb 13, 2020

I have no STR to reproduce the crash but this is to prevent unnecessary calling ReplaceBookmarkNodeGUID for every updates. And test plan is to make sure it can still apply update and maintain order

@LaurenWags
Copy link
Member

LaurenWags commented Feb 14, 2020

Verified using

Brave 1.6.30 Chromium: 80.0.3987.106 (Official Build) nightly (64-bit)
Revision f68069574609230cf9b635cd784cfb1bf81bb53a-refs/branch-heads/3987@{#882}
OS macOS Version 10.14.6 (Build 18G103)

Device A: macOS x64 10.14.6
Device B Ubuntu VM 16.04

Screen Shot 2020-02-14 at 9 38 57 AM

Verification passed on

Brave 1.6.31 Chromium: 80.0.3987.106 (Official Build) nightly (64-bit)
Revision f68069574609230cf9b635cd784cfb1bf81bb53a-refs/branch-heads/3987@{#882}
OS Ubuntu 18.04 LTS

Both Devices: Ubuntu 18.04 LTS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on ReplaceBookmarkNodeGUID
5 participants