Closed Bug 1301622 Opened 3 years ago Closed 2 years ago

Have Sync do the right thing when places maintenance runs

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: markh, Assigned: Lina)

References

Details

(Whiteboard: [data-integrity][sync:bookmarks])

Attachments

(1 file)

PlacesDBUtils.jsm is where the places maintenance code is. It may make a number of changes to the DB without sending observer notifications (eg, it makes its own attempt at fixing orphans and moving them to the unfiled folder, it may change the title of the roots, etc)

When maintenance is complete it send a notification "places-maintenance-finished" - at this time Sync should "do the right thing" - what that is isn't clear.

Kit, note also that IIUC, your tracker patch isn't touching this file, but it probably should (ie, it probably should still increment the change counter in many cases) - or maybe that's the *only* action we should take in this bug? Not clear yet, but I thought I'd get this on file before it slips my mind.
note that in an ideal world, PlacesDBUtils shouldn't do anything, it's just for emergency cases, but sure, maybe we could do something to make it work better with Sync.
Its main problem is that it should be refactored, the code is old and ugly, and some of those tasks are taking too much time (telemetry tells me for some users it can take 10s).
Some tasks could probably also be removed since they were covering past bugs.
Depends on: 1258127
Priority: -- → P3
Whiteboard: [data-integrity][sync:bookmarks]
While I'm working on maintenance tasks, I'll just snag this. We'll want an extra trigger to insert tombstones for any synced bookmarks that we delete, and also bump the parent's change counter for moves, but this doesn't seem too onerous.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8919092 [details]
Bug 1301622 - Teach Places maintenance about Sync.

https://reviewboard.mozilla.org/r/190016/#review195914

this whole code really needs a rewrite :/

::: toolkit/components/places/PlacesDBUtils.jsm:278
(Diff revision 2)
> +    // parent. The "sync tombstone" trigger inserts tombstones for deleted
> +    // synced bookmarks.
> +    cleanupStatements.push({
> +      query:
> +      `CREATE TEMP TRIGGER IF NOT EXISTS moz_bm_sync_change_temp_trigger
> +       AFTER UPDATE of guid, parent ON moz_bookmarks

what about the position of bookmarks? you don't care if we change it?
Attachment #8919092 - Flags: review?(mak77) → review+
Comment on attachment 8919092 [details]
Bug 1301622 - Teach Places maintenance about Sync.

https://reviewboard.mozilla.org/r/190016/#review195914

> what about the position of bookmarks? you don't care if we change it?

I bumped the parent's counter in `moz_bm_reindex_temp_trigger`, but, in retrospect, this trigger is a better place for that. Updated!
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1126763734c0
Teach Places maintenance about Sync. r=mak
https://hg.mozilla.org/mozilla-central/rev/1126763734c0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.