1582377 - Fixes for errors in new bookmarks mirror engine telemetry

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler (ex-Mozilla)]

I spotted a few errors in new bookmark sync telemetry. Explanations for the fixes to follow with each commit.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1582377 - Don't recreate existing bookmarks mirror indexes in schema migrations. r?markh!

Downgrading a profile and syncing downgrades the bookmarks mirror
schema version, but leaves the new indexes in place. This causes the
migration logic to throw an "index already exists" error on upgrade.
The fix is to add an IF NOT EXISTS, to avoid recreating the index.

Depends on D46433

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1582377 - Catch invalid URLs caused by bookmarks mirror tag query rewriting. r?markh!

Rewriting a tag query might produce an invalid URL; for example, if the
tag name contains special characters. We don't currently URL-encode tag
folder names in queries (bug 1449939), so our only option is to flag the
query as invalid. This should fix the "<URL> is not a valid URL" and
"url is null" errors seen in telemetry.

Depends on D46434

Comment 3

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1582377 - Use `i32` instead of `i64` for bookmarks mirror Boolean columns. r?markh!

Depends on D46435

Comment 4

[Lina Butler (ex-Mozilla)]

[Tracking Requested - why for this release]: We'll want to uplift these fixes to 70, since we'd like to turn on new bookmark sync by default. All of these are small, well-scoped fixes covered by tests, and avoid breaking bookmark syncing for certain users (folks with tags containing ampersands or non-ASCII characters, and those who downgrade their profiles).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2122076416b4
Don't recreate existing bookmarks mirror indexes in schema migrations. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/6fdb8149f1d5
Catch invalid URLs caused by bookmarks mirror tag query rewriting. r=tcsc

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2122076416b4
https://hg.mozilla.org/mozilla-central/rev/6fdb8149f1d5

Comment 7

[Julien Cristau [:jcristau]]

Please file uplift requests when you get a chance.

Comment 8

[Lina Butler (ex-Mozilla)]

Comment on attachment 9093835 [details]
Bug 1582377 - Catch invalid URLs caused by bookmarks mirror tag query rewriting. r?markh!

Beta/Release Uplift Approval Request

• User impact if declined: The first patch fixes an issue where new bookmark sync breaks for users who downgrade their profiles (for example, using the same profile with Beta, Nightly, and Release). The second fixes an issue where an invalid character in a bookmark tag could cause the entire sync to fail.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are small, and have xpcshell tests. We'll be keeping an eye on engine error rates to ensure they decrease accordingly.
• String changes made/needed:

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9093835 [details]
Bug 1582377 - Catch invalid URLs caused by bookmarks mirror tag query rewriting. r?markh!

Fix for bookmarks sync error, has test coverage, let's uplift for beta 10.

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/db46e193a5c6
https://hg.mozilla.org/releases/mozilla-beta/rev/23e4deb8581f