1443388 - Mirror should omit `folderName` for non-tag queries

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Lina Butler (ex-Mozilla)]

Mark found this while chasing down another bug. Our existing engine omits the folder name if it's blank (https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/places/PlacesSyncUtils.jsm#1919-1921), but the mirror will upload an empty string (https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/places/SyncedBookmarksMirror.jsm#1632,1683).

The old engine's validation logic is overly zealous (https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/places/PlacesUtils.jsm#269), and will throw for records with `folderName: ""`.

iOS is OK with either: https://github.com/mozilla-mobile/firefox-ios/blob/f7107d9d2f753e1990dadeb77461dd0c3beb19d4/Sync/BookmarkPayload.swift#L392

Let's loosen up the validation logic, and also change the mirror to only include `folderName` if it's not empty.

This is also a good chance to check if we're including other empty fields that can be omitted.

Comment 1

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

I'm putting up a very simple patch for this - it enforces the "no empty-string folderName" when reading records from the server and just before writing them. The patch also prevents SyncedBoomarksMirror.jsm from creating such outgoing records, but the additional check before putting them on the server seems an extra layer of defense. With this change there's no need to loosen the validators.

Kit, let me know if you think this is OK, or whether handling them at a lower level seems necessary.

Comment 2

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

Attachment: Bug 1443388 - don't allow empty folderName strings in to or out of Sync.

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

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8956613 [details]
Bug 1443388 - don't allow empty folderName strings in to or out of Sync.

https://reviewboard.mozilla.org/r/225550/#review231432

Thanks! The general approach sounds great to me, but the mirror won't quite do what we want.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1632
(Diff revision 1)
>    async fetchLocalChangeRecords() {
>      let changeRecords = {};
>  
>      let itemRows = await this.db.execute(`
>        SELECT id, syncChangeCounter, guid, isDeleted, type, isQuery,
> -             smartBookmarkName, IFNULL(tagFolderName, "") AS tagFolderName,
> +             smartBookmarkName, tagFolderName,

I think this will upload `folderName: null`, which will also cause the validation function to throw. Would you also mind adding a test for tag queries to https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/sync/test_bookmark_kinds.js, too? I'm sorry we don't have one already...

Comment 4

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

Comment on attachment 8956613 [details]
Bug 1443388 - don't allow empty folderName strings in to or out of Sync.

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

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8956613 [details]
Bug 1443388 - don't allow empty folderName strings in to or out of Sync.

https://reviewboard.mozilla.org/r/225550/#review231922

Awesome, thank you for the test, the comment, and the fix for the params!

Comment 6

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

Comment on attachment 8956613 [details]
Bug 1443388 - don't allow empty folderName strings in to or out of Sync.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/885b39af0ef1
don't allow empty folderName strings in to or out of Sync. r=kitcambridge

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/885b39af0ef1