Closed Bug 1310525 Opened 3 years ago Closed 3 years ago

Bookmark syncing fails with > 1998 changed items

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

If there are > 999*2 bookmarks marked as changed, we will create a SQL statement with > 999 variables, causing bookmark sync to fail.
Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

https://reviewboard.mozilla.org/r/86272/#review85124

::: services/sync/modules/engines/bookmarks.js:552
(Diff revision 1)
>      // of bound parameters per query.
>      for (let startIndex = 0;
>           startIndex < modifiedGUIDs.length;
>           startIndex += SQLITE_MAX_VARIABLE_NUMBER) {
>  
> -      let chunkLength = Math.min(startIndex + SQLITE_MAX_VARIABLE_NUMBER,
> +      let chunkLength = Math.min(SQLITE_MAX_VARIABLE_NUMBER,

Looking at it closer, this logic was totally wrong. :-(

I think it needs to be `Math.min(SQLITE_MAX_VARIABLE_NUMBER, modifiedGUIDs.length - startIndex)`, so the last chunk should only hold the number of remaining elements. Does that look right to you?
Assignee: nobody → markh
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

https://reviewboard.mozilla.org/r/86272/#review85292
Attachment #8801604 - Flags: review?(kcambridge)
Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

https://reviewboard.mozilla.org/r/86272/#review85348

Awesome, thanks!
Attachment #8801604 - Flags: review?(kcambridge) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/37737974ba7f
don't exceed sqlite's max variable limit when many changed bookmarks exist. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/37737974ba7f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

Approval Request Comment
[Feature/regressing bug #]: Bug 274496
[User impact if declined]: Sync failures in rare cases.
[Describe test coverage new/current, TreeHerder]: tests pass
[Risks and why]: Low risk limited to sync
[String/UUID change made/needed]: None
Attachment #8801604 - Flags: approval-mozilla-aurora?
Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

Fix an issue related to bookmark syncing. Take it in 51 aurora.
Attachment #8801604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.