1310525 - Bookmark syncing fails with > 1998 changed items

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

If there are > 999*2 bookmarks marked as changed, we will create a SQL statement with > 999 variables, causing bookmark sync to fail.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

Review commit: https://reviewboard.mozilla.org/r/86272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86272/

Comment 2

[Lina Butler (ex-Mozilla)]

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?

Comment 3

[Lina Butler (ex-Mozilla)]

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8801604 [details]
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86272/diff/1-2/

Comment 5

[Lina Butler (ex-Mozilla)]

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!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37737974ba7f
don't exceed sqlite's max variable limit when many changed bookmarks exist. r=kitcambridge

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37737974ba7f

Comment 8

[Mark Hammond [:markh] [:mhammond]]

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

Comment 9

[Gerry Chang [:gchang]]

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.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49dfb63c28b9

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49dfb63c28b9