Closed Bug 1546035 Opened 9 months ago Closed 9 months ago

Can't merge local kind Livemark and remote kind Folder

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: yoasif, Assigned: Lina)

References

Details

Attachments

(2 files)

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

See attached log.

Blocks: 1482608

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.
Assignee: nobody → lina
Status: NEW → ASSIGNED

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.

Attachment #9060207 - Attachment description: Bug 1546035 - Back up livemarks before a first sync. r?mak!,tcsc! → Bug 1546035 - Remove local and remote livemarks when syncing. r?mak!,tcsc!
Priority: -- → P2

(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?

(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.

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.

(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.

Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76e8b769a7c0
Remove local and remote livemarks when syncing. r=mak,tcsc
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.