1301622 - Have Sync do the right thing when places maintenance runs

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

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.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 2

[Lina Butler [:lina]]

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.

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1301622 - Teach Places maintenance about Sync.

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

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8919092 [details]
Bug 1301622 - Teach Places maintenance about Sync.

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8919092 [details]
Bug 1301622 - Teach Places maintenance about Sync.

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

Comment 7

[Lina Butler [:lina]]

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!

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8919092 [details]
Bug 1301622 - Teach Places maintenance about Sync.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1126763734c0
Teach Places maintenance about Sync. r=mak

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1126763734c0