1546035 - Can't merge local kind Livemark and remote kind Folder

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Asif Youssuff]

Attachment: error-sync-1555884575869.txt

Seeing errors when using the services.sync.engine.bookmarks.buffer preference enabled.

See attached log.

Comment 1

[Lina Butler [:lina]]

Thanks for the report! Looks like we're going to need to handle these, after all. 😅 Since Firefox doesn't support livemarks anymore, I think we can:

1. Export livemarks before syncing, if we haven't already. There's a LiveBookmarkMigrator.jsm module to do just that, and a pref that we can check to make sure we only do this once.
2. Treat all livemarks, local and remote, as non-syncable, so that they're deleted on both sides.

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1546035 - Remove local and remote livemarks when syncing. r?mak!,tcsc!

This commit exports livemarks before syncing for the first time, to
avoid losing livemarks that Sync may have resurrected. As of
https://github.com/mozilla/dogear/pull/37, Dogear treats livemarks as
non-syncable, and deletes them locally and remotely.

This also bumps the mirror schema version, to trigger a first sync.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Lina Cambridge (she/her) [:lina] from comment #1)

1. Export livemarks before syncing, if we haven't already. There's a LiveBookmarkMigrator.jsm module to do just that, and a pref that we can check to make sure we only do this once.

May Sync resurrect livemarks after the browserGlue LiveBookmarkMigrator.migrate() call was invoked already? In such a case, by invoking it again here, don't we risk to overwrite the first backup and cause a worse dataloss?
What's the exact timeline of events here?

Comment 4

[Lina Butler [:lina]]

(In reply to Marco Bonardo [::mak] (Away 22 -28 Apr) from comment #3)

May Sync resurrect livemarks after the browserGlue LiveBookmarkMigrator.migrate() call was invoked already? In such a case, by invoking it again here, don't we risk to overwrite the first backup and cause a worse dataloss?

I don't think so; it looks like the migrator uses openUnique here, and will create a new file with a suffix?

What's the exact timeline of events here?

I think this can happen if you:

• Synced with an older version of the mirror (in bug 1477671, we marked remote livemarks for deletion, but left local ones alone, and we didn't stop writing livemark annos until bug 1512868), and...
• The server has a folder record for something that's a livemark. IIRC, the old addLivemark code used to insert a normal folder, then set the annotations on it. It seems really implausible, but the only thing I can think of is that Sync fetched the record before the anno was added (bug 632287, but that's from before we switched to the new tracker).

It might be fine if we don't run the migration, assume that the previous migrate call is enough, and delete any resurrected livemarks...but I thought it'd make sense to do just in case.

Comment 5

[Asif Youssuff]

Just wanted to throw out a possible issue to watch out for -- if bookmark backups (either exported or in browser) contain livemarks (not sure that they do), only checking once may recreate the issue if someone restores bookmarks.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Asif Youssuff from comment #5)

Just wanted to throw out a possible issue to watch out for -- if bookmark backups (either exported or in browser) contain livemarks (not sure that they do), only checking once may recreate the issue if someone restores bookmarks.

livemarks in backups are restored as normal empty folders, iirc, so it should not be a big deal.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76e8b769a7c0
Remove local and remote livemarks when syncing. r=mak,tcsc

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/76e8b769a7c0