1433182 - Add a mirror corruption test with a left pane root

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler [:lina]]

In some cases, if we don't have the left pane root or "All Bookmarks" folder, the queries will be orphaned and moved to unfiled. If the left pane queries are on the server, this means we'll end up with wonky views as in bug 1297234.

We should think about adding heuristics to remove them, and these orphaned queries might be easier to detect once the left pane queries are virtualized (bug 1423896 and bug 1310295).

But, either way, it would be good to test how the mirror behaves with roots and items that shouldn't be on the server.

Comment 1

[Lina Butler [:lina]]

Standard8 is making the left pane root virtual in bug 1310295, and adding a migration that uploads tombstones for left pane items, but it's possible they'll be resurrected by an older client, or maybe Android. I think the merger will treat them as orphans and move them into "unfiled". We should add a test for this, and think about cleanup.

Comment 2

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

Attachment: Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

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

Comment 3

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

Comment on attachment 8960090 [details]
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

https://reviewboard.mozilla.org/r/228884/#review234546

::: toolkit/components/places/SyncedBookmarksMirror.jsm:963
(Diff revision 1)
>      // (Note: Timing was done before adding maybeYield calls)
> -    await inflateTree(remoteTree, pseudoTree, PlacesUtils.bookmarks.rootGuid);
>  
> +    // Because we allow the mirror to store non-syncable roots but avoid
> +    // applying them, we inflate the tree but ignore such roots when doing do.
> +    let shouldIncludeSubtree = node => {

FWIW, I needed to do this here as one of the subsumes checks failed due to the remote tree including these non-content roots.

It probably would have been nicer to include these items in the returned tree (ie, they *are* a part of the remote tree), but I couldn't see a clear way to have .subsumes implement the "ignore subtrees" logic.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8960090 [details]
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

https://reviewboard.mozilla.org/r/228884/#review234800

::: toolkit/components/places/SyncedBookmarksMirror.jsm:660
(Diff revision 1)
> +    // them so we can ignore that entire sub-tree - so such items need special
> +    // treatment.
> +    let parentGuid = validateGuid(record.parentid);
> +    if (parentGuid == PlacesUtils.bookmarks.rootGuid) {
> +        await this.db.executeCached(`
> +          REPLACE INTO structure(guid, parentGuid, position)

Let's use an `INSERT OR IGNORE` here...it doesn't hurt if we replace roots that *should* be there, just a style preference and a vague "minimize our use of `parentid`" thought.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:923
(Diff revision 1)
>      // includes items orphaned by an interrupted upload on another device.
>      // We keep the orphans in "unfiled" until the other device returns and
>      // uploads the missing parent.
> +    // Note that we avoid returning orphaned queries here - there's a really
> +    // good chance that orphaned queries are actually left-pane queries with
> +    // the left-pane root missing.

I was on the fence about it, but I think it's the right thing to do. On the one hand, if we have an "All Bookmarks" folder on the server, this will miss those queries because the orphan is the folder, not the queries. On the other, it's an easy, and, as you note in the comments, fairly reliable heuristic.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:2022
(Diff revision 1)
>  /**
> - * Sets up the syncable roots. All items in the mirror should descend from these
> - * roots. If we ever add new syncable roots to Places, this function should also
> + * Sets up the syncable roots. All items in the mirror we apply will descend
> + * from these roots - however, malformed records from the server which create
> + * a different root *will* be created in the mirror - just not applied.
> +
> + * If we ever add new syncable roots to Places, this function should also

Though this will be automagic thanks to bug 1446886. :-)

::: toolkit/components/places/tests/sync/test_bookmark_corruption.js:834
(Diff revision 1)
> +  }], { needsMerge: true }));
> +
> +  await buf.apply();
> +
> +  // should have ignored everything.
> +  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, initialTree);

Very nice!

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8960090 [details]
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

https://reviewboard.mozilla.org/r/228884/#review234546

> FWIW, I needed to do this here as one of the subsumes checks failed due to the remote tree including these non-content roots.
> 
> It probably would have been nicer to include these items in the returned tree (ie, they *are* a part of the remote tree), but I couldn't see a clear way to have .subsumes implement the "ignore subtrees" logic.

I guess `BookmarkNode` could grow an `excludeFromMerge` property that's true by default, and that `inflateTree` sets to false for all user content roots...but that seems like a lot of extra complication, and I'm not sure if it's worth it (maybe for logging, or telemetry? Though, we can wire that up using your approach, too).

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8960090 [details]
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

https://reviewboard.mozilla.org/r/228884/#review235000

Oh, MozReview...this looks great, thanks!

::: toolkit/components/places/SyncedBookmarksMirror.jsm:962
(Diff revision 1)
>      // the pseudo-tree and recursing in JS takes 30ms for 5k items.
>      // (Note: Timing was done before adding maybeYield calls)
> -    await inflateTree(remoteTree, pseudoTree, PlacesUtils.bookmarks.rootGuid);
>  
> +    // Because we allow the mirror to store non-syncable roots but avoid
> +    // applying them, we inflate the tree but ignore such roots when doing do.

Could you ignore these nodes in the loop above, or do you prefer the function? (Either is OK).

Comment 7

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

Comment on attachment 8960090 [details]
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5aa8b762d41
bookmark mirror now handles left-pane-roots on the server. r=kitcambridge

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e5aa8b762d41