Closed Bug 1454864 Opened 6 years ago Closed 6 years ago

Improve merging of non-syncable items

Categories

(Firefox :: Sync, enhancement, P2)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(3 files)

Bug 1454864 - Improve merging of non-syncable items in the bookmarks mirror.
59 bytes, text/x-review-board-request
markh
: review+
tcsc
: review+
Details
Bug 1454864 - Upload tombstones for non-syncable items.
59 bytes, text/x-review-board-request
markh
: review+
tcsc
: review+
Details
Bug 1454864 - Simplify bookmark trees, merge states, and orphan relocation.
59 bytes, text/x-review-board-request
tcsc
: review+
Details 
Excluding non-syncable items from the local tree has some interesting consequences if we only know the items are non-syncable on one side.

In the case where we have a complete non-syncable subtree locally, but an incomplete one remotely, we'll corrupt positions for the non-syncable items when we apply the merge tree. (They still exist in Places, though their siblings might not, and none of them are part of the merged tree).

I think we can repurpose our orphan checking logic to check and unconditionally delete all items that are non-syncable on either side. Deleting them outright might cause data loss, but they're already an edge case, easier than moving to unfiled, and consistent with the migration added in bug 1310295.
Comment on attachment 8968767 [details]
Bug 1454864 - Upload tombstones for non-syncable items.

https://reviewboard.mozilla.org/r/237460/#review244058

These 2 patches do increase the complexity, but also do make the mirror more correct. As usual, the quality of the patch is excellent!
Attachment #8968767 - Flags: review?(markh) → review+
Comment on attachment 8968766 [details]
Bug 1454864 - Improve merging of non-syncable items in the bookmarks mirror.

https://reviewboard.mozilla.org/r/237458/#review244046

::: toolkit/components/places/SyncedBookmarksMirror.jsm:74
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyGetter(this, "MirrorLog", () =>
>    Log.repository.getLogger("Sync.Engine.Bookmarks.Mirror")
>  );
>  
> -XPCOMUtils.defineLazyGetter(this, "UserContentRootsAsSqlList", () =>
> +// A recursive CTE for all synced items in Places. We start with the roots,

nit: CTE is something I had to google :) Could you please expand it?
Attachment #8968766 - Flags: review?(markh) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93eea5f99ab7
Improve merging of non-syncable items in the bookmarks mirror. r=markh
https://hg.mozilla.org/integration/autoland/rev/b497152c37e8
Upload tombstones for non-syncable items. r=markh
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e02766aa3bc
Backed out 2 changesets as requested by dev kitcambridge
Backed out in https://hg.mozilla.org/integration/autoland/rev/8e02766aa3bc because I realized reviewing Thom's patch for bug 1343103 that `guidsToDeleteRemotely` is unnecessary (we should just use `moz_bookmarks_deleted`), and adding to both `deleteLocally` and `deleteRemotely` is unnecessary and confusing (bug 1455790).

I also want to triple-check, after sleeping on it, that it's sufficient to only upload tombstones for `remoteDeletions`. Are there any cases where we add to `localDeletions` and *not* `remoteDeletions`, but where we *should*? I think the test does a good job of exercising this, and `subsumes` ensures we do *something* with every GUID, but I want to be certain.
Priority: -- → P2
Comment on attachment 8968766 [details]
Bug 1454864 - Improve merging of non-syncable items in the bookmarks mirror.

https://reviewboard.mozilla.org/r/237458/#review247150

Looks good. I assume it's ok that this only applies to the new engine (PlacesSyncUtils has similar with recursive queries...).

I also think that this might cause validation to complain about some things unnecessarially, but I could be wrong.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:90
(Diff revision 3)
> + * items might be orphaned in "unfiled", in which case they're seen as syncable
> + * locally. If the server has the missing parents and roots, we'll determine
> + * that the items are non-syncable when merging, remove them from Places, and
> + * upload tombstones to the server.
> + */
> +XPCOMUtils.defineLazyGetter(this, "LocalItemsSQLFragment", () => `

I guess this is better all in one place than the duplication we had...

::: toolkit/components/places/SyncedBookmarksMirror.jsm:2990
(Diff revision 3)
>   * A node in a local or remote bookmark tree. Nodes are lightweight: they carry
>   * enough information for the merger to resolve trivial conflicts without
>   * querying the mirror or Places for the complete value state.
>   */
>  class BookmarkNode {
> -  constructor(guid, age, kind, needsMerge = false, level = 0) {
> +  constructor(guid, age, kind, needsMerge = false, level = 0,

nit: I'd probably prefer `needsMerge`, `level`, and `isSyncable` be an options argument at this point, unless you can think of a reason not to.
Attachment #8968766 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8968767 [details]
Bug 1454864 - Upload tombstones for non-syncable items.

https://reviewboard.mozilla.org/r/237460/#review247152

This looks good, itemsToRemove simplifies a lot of the buffer code around deletions (even if I do find triggers extremely tricky to reason about).
Attachment #8968767 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8971680 [details]
Bug 1454864 - Simplify bookmark trees, merge states, and orphan relocation.

https://reviewboard.mozilla.org/r/240450/#review247154

This looks fine in so far as it seems mostly to be refactoring, moving code around, or deleting code.

::: toolkit/components/places/SyncedBookmarksMirror.jsm
(Diff revision 1)
>        case BookmarkMergeState.TYPE.LOCAL:
>          return "Structure: Local";
>        case BookmarkMergeState.TYPE.REMOTE:
>          return "Structure: Remote";
>        case BookmarkMergeState.TYPE.NEW:
> -        // We intentionally don't log the new structure node here, since

What's wrong with this comment now?
Attachment #8971680 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8971680 [details]
Bug 1454864 - Simplify bookmark trees, merge states, and orphan relocation.

https://reviewboard.mozilla.org/r/240450/#review247154

Thanks for all the reviews!

> What's wrong with this comment now?

We don't build new structure nodes anymore (I suspect this is going to give us a bit of a perf boost in cases where we have structure conflicts...`toBookmarkNode` did a lot of busy work to build the node, when we didn't need it in the first place), so there's nothing more to log here.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c470f035fe63
Simplify bookmark trees, merge states, and orphan relocation. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/6cc5d9e64f7b
Improve merging of non-syncable items in the bookmarks mirror. r=markh,tcsc
https://hg.mozilla.org/integration/autoland/rev/56e5751b3a4d
Upload tombstones for non-syncable items. r=markh,tcsc
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: