Closed Bug 1433180 Opened 3 years ago Closed 2 years ago

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

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)

The mirror should ignore these, and correctly order non-tombstone siblings. We might also want to have `fetchRemoteOrphans` report them.
Mentor: kit
Priority: -- → P3
These are common enough to where I think adding a test to `test_bookmark_corruption.js` would be worthwhile.
Priority: P3 → P2
Assignee: nobody → markh
Iteration: --- → 61.1 - Mar 26
(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.
Flags: needinfo?(kit)
(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!)
(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.
Flags: needinfo?(kit)
(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 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 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.
Attachment #8961222 - Flags: review?(kit) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/2fc6879a995e
Ensure the bookmark mirror handles a child which is a tombstone. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/2fc6879a995e
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.