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

Bookmarks orders sometimes are sent equal for different bookmarks items #2367

Closed
AlexeyBarabash opened this issue Dec 5, 2018 · 5 comments · Fixed by brave/brave-core#1174

Comments

@AlexeyBarabash
Copy link
Contributor

AlexeyBarabash commented Dec 5, 2018

Description

Bookmarks orders sometimes are sent equal for different bookmarks items.

Steps to Reproduce

Steps to reproduce, mostly taken from #2133

  1. Use debug build
  2. Create sync chain on device 1
  3. Join device 2 and add a bookmark on device 2, gets sync'd to device 1
  4. Import bookmarks from other browser on device 1, wait for sometime, sync's on device 2
  5. Import 5000 bookmark HTML file on device 1, wait for a really long time, at some moment ValidateFolderOrders will report invalid order.

Steps to Reproduce for QA team without taking a debug build

  1. Create sync chain on device 1
  2. Join device 2 and add a bookmark on device 2, gets sync'd to device 1
  3. Import 5000 bookmark HTML file on device 1, wait for a really long time to sync bookmarks into device 2
  4. Expand the very nested folder in bookmarks on device 2, see some bookmarks at a wrong position compare to device 1

Actual result:

Warning in ValidateFolderOrders is triggered.

Expected result:

Warning in ValidateFolderOrders is not triggered.

Reproduces how often:

Easily, but requires to wait.

Brave version (brave://version info)

Brave | 0.60.1 Chromium: 71.0.3578.80 (Developer Build) (64-bit)
Revision | 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}

Reproducible on current release:

  • Does it reproduce on brave-browser dev/beta builds?
    no, ValidateFolderOrders is not merged yet

Additional Information

Pieces of logs:
Device 1 sends

{"action":0,"bookmark":{"hideInToolbar":false,"isFolder":false,
"order":"1.1.0.3.1.1.1.1.1.1.1.1.1.1.1.1.109.6","parentFolderObjectId":{"0":243,"1":47,"2":35,"3":37,"4":31,"5":108,"6":60,"7":108,"8":44,"9":37,"10":15,"11":138,"12":236,"13":5,"14":170,"15":10},
"site":{"creationTime":0,
"customTitle":"6","favicon":"","lastAccessedTime":0,"location":"https://medium.com/@ericclemmons/javascript-fatigue-48d4011b6fc4#.uhv5554id",
"title":"6"}},"deviceId":{"0":0},"objectData":"bookmark","objectId":{"0":20,"1":8,"2":140,"3":172,"4":150,"5":146,"6":130,"7":98,"8":57,"9":128,"10":131,"11":159,"12":78,"13":40,"14":235,"15":38},"syncTimestamp":1543936897442.687}


{"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,
"order":"1.1.0.3.1.1.1.1.1.1.1.1.1.1.1.1.109.6","parentFolderObjectId":{"0":243,"1":47,"2":35,"3":37,"4":31,"5":108,"6":60,"7":108,"8":44,"9":37,"10":15,"11":138,"12":236,"13":5,"14":170,"15":10},"site":{"creationTime":0,
"customTitle":"1884","favicon":"","lastAccessedTime":0,"location":"https://www.youtube.com/watch?v=-_tvJtUHnmU",
"title":"1884"}},"deviceId":{"0":0},"objectData":"bookmark","objectId":{"0":44,"1":42,"2":122,"3":131,"4":104,"5":189,"6":40,"7":19,"8":216,"9":143,"10":240,"11":209,"12":86,"13":162,"14":62,"15":81},"syncTimestamp":1543937503598.914},

@AlexeyBarabash AlexeyBarabash added this to Untriaged backlog in Sync Dec 11, 2018
@bbondy bbondy added the priority/P3 The next thing for us to work on. It'll ride the trains. label Dec 12, 2018
@bbondy bbondy moved this from Untriaged backlog to P3, P4 & P5 backlog in Sync Dec 12, 2018
@AlexeyBarabash AlexeyBarabash self-assigned this Dec 19, 2018
@bbondy bbondy moved this from P3, P4 & P5 backlog to Inprogress in Sync Dec 20, 2018
@AlexeyBarabash
Copy link
Contributor Author

Updated with Steps to Reproduce for QA team without taking a debug build

@AlexeyBarabash
Copy link
Contributor Author

Opened a PR brave/brave-core#1174 .

Sync automation moved this from Inprogress to Completed Dec 22, 2018
@darkdh darkdh added this to the 0.59.x - Beta milestone Dec 22, 2018
@AlexeyBarabash
Copy link
Contributor Author

Yes, not yet merged to 0.59.x, but I see uplift-approved/0.59.x-Beta label, so doing merge now.

@AlexeyBarabash
Copy link
Contributor Author

Merged to 0.59.x now. Thanks @GeetaSarvadnya .

@srirambv
Copy link
Contributor

srirambv commented Jan 7, 2019

Verification passed on

Brave 0.59.18 Chromium: 72.0.3626.28 (Official Build) beta(64-bit)
Revision 997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS Linux

Verification passed on

Brave 0.59.18 Chromium: 72.0.3626.28 (Official Build) beta (64-bit)
Revision 997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS Windows

Verified passed with

Brave 0.59.18 Chromium: 72.0.3626.28 (Official Build) beta(64-bit)
Revision 997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS Mac OS X

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

Successfully merging a pull request may close this issue.

6 participants