1433180 - Add a mirror corruption test with a folder that mentions a tombstone as a child

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler [:lina]]

The mirror should ignore these, and correctly order non-tombstone siblings. We might also want to have `fetchRemoteOrphans` report them.

Comment 1

[Lina Butler [:lina]]

These are common enough to where I think adding a test to `test_bookmark_corruption.js` would be worthwhile.

Comment 2

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

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #0)
> The mirror should ignore these, and correctly order non-tombstone siblings.

To clarify, what you mean here is, roughly:

  await buf.store(<an array where a folder lists a number of GUIDs as children, but one of them is a tombstone>)
  await buf.apply();
  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
    <tree that accurately represents the buffer, but the tombstone is absent from children>
  });

is that correct?

If so, I've a patch for this which also checks the changes to be uploaded after bug.apply(), but to my surprise it was empty - I was expecting that we would find the folder being re-uploaded with the correct list of children. Is that what I should have expected? :)

Also:
> We might also want to have `fetchRemoteOrphans` report them.

This makes me think I might be missing the point because it's not clear to me that, as described above, anything is actually considered an orphan.

Comment 3

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

(In reply to Mark Hammond [:markh] from comment #2)
> I was expecting that we
> would find the folder being re-uploaded with the correct list of children.
> Is that what I should have expected? :)

Thinking some more, I think that's actually what should happen in that scenario - "repair wars" seem possible if we decide to reupload every inconsistency we correctly and reliably handle without evidence the repair solves an issue for other clients (which possibly caused it in the first place!)

Comment 4

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #2)
> To clarify, what you mean here is, roughly:
> ...
> is that correct?

Yup. Specifically, I wanted to make sure `index` is contiguous in the local tree, and we didn't introduce holes if any tombstones are in the middle of the folder. That's really the only reason I thought it would be good to have a test.

> If so, I've a patch for this which also checks the changes to be uploaded
> after bug.apply(), but to my surprise it was empty - I was expecting that we
> would find the folder being re-uploaded with the correct list of children.
> Is that what I should have expected? :)

That's a good question! It's not what we do now, because `fetchRemoteTree` fixed up the in-memory tree, so it looks consistent...even though it doesn't match what's actually in the mirror database.

We have a `fetchRemoteOrphans` check that looks for missing parents and children, and an `itemsToWeaklyReupload` table. I could imagine adding the parents of `missingChildren` into that table, and maybe `missingParents`. This is basically duplicating part of repair.

But that's also tricky. First, as you point out, we might get into repair or sync wars, either with Android, other mirror-capable Desktops when they sync, or Desktop repair. Also, if we temporarily reparented orphans to unfiled, everything in `missingParents` will move there, which we don't want if we just didn't see the parent yet. We'll also want to upload both the parent and child, to fix the parent's `children`, and the child's `parentid`. `missingChildren` has the same problem with unfiled, but is probably less risky (citation needed).

So maybe the right thing to do is add a simpler test to make sure we don't do something silly, file a follow-up to figure out what to do about missing parents and children, and use the event telemetry from `fetchRemoteOrphans` and repair to guide us?

Or, if your gut feeling is that reuploading parents of `missingChildren` is safe, we could try that.

Comment 5

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

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #4)
> So maybe the right thing to do is add a simpler test to make sure we don't
> do something silly, file a follow-up to figure out what to do about missing
> parents and children, and use the event telemetry from `fetchRemoteOrphans`
> and repair to guide us?

That makes sense, thanks. I'm not sure it's worth filing a bug for that now as it isn't going to be actionable, and as you say, when we look at event telemetry we should get some concrete actions.

Comment 6

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

Attachment: Bug 1433180 - Ensure the bookmark mirror handles a child which is a tombstone.

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

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8961222 [details]
Bug 1433180 - Ensure the bookmark mirror handles a child which is a tombstone.

https://reviewboard.mozilla.org/r/229998/#review235690


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/tests/sync/test_bookmark_corruption.js:849
(Diff revision 1)
> +          guid: "bookmarkAAAA",
> +          type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
> +          url: "http://example.com/a",
> +          index: 0,
> +          title: "Bookmark A",
> +        },{

Error: A space is required after ','. [eslint: comma-spacing]

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8961222 [details]
Bug 1433180 - Ensure the bookmark mirror handles a child which is a tombstone.

https://reviewboard.mozilla.org/r/229998/#review235688

Yay, thanks!

::: toolkit/components/places/tests/sync/head_sync.js:88
(Diff revision 1)
>      return itemInfo;
>    }
>    let root = await PlacesUtils.promiseBookmarksTree(rootGuid);
>    let actual = bookmarkNodeToInfo(root);
>    if (!ObjectUtils.deepEqual(actual, expected)) {
> -    info(`Expected structure for ${rootGuid}: ${JSON.stringify(expected)}`);
> +    info(`Expected structure for ${rootGuid}: ${CanonicalJSON.stringify(expected)}`);

Nice! I also like http://tlrobinson.net/projects/javascript-fun/jsondiff/ for comparing outputs.

Comment 9

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

Comment on attachment 8961222 [details]
Bug 1433180 - Ensure the bookmark mirror handles a child which is a tombstone.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fc6879a995e
Ensure the bookmark mirror handles a child which is a tombstone. r=kitcambridge

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2fc6879a995e