Closed Bug 1433182 Opened 3 years ago Closed 2 years ago

Add a mirror corruption test with a left pane root

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Iteration:
61.1 - Mar 26
Tracking Status
firefox61 --- fixed

People

(Reporter: lina, Assigned: markh, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Priority: -- → P3
Mentor: kit
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.
Priority: P3 → P2
Assignee: nobody → markh
Iteration: --- → 61.1 - Mar 26
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 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 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 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).
Attachment #8960090 - Flags: review?(kit) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/e5aa8b762d41
bookmark mirror now handles left-pane-roots on the server. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/e5aa8b762d41
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.