Closed Bug 1444262 Opened 6 years ago Closed 6 years ago

A bookmark that later becomes a query breaks the mirror

Categories

(Firefox :: Sync, defect, P2)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

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

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror.
59 bytes, text/x-review-board-request
lina
: review+
Details 
STR:
* Create a new bookmark in the UI.
* Wait for Sync to complete.
* Change the URL to a valid query URL (ie, a URL starting with place:)
* Sync again.

Expected:
* everything works.

Actual:
* Exception syncing:

> 1520468286316	Sync.ErrorHandler	DEBUG	bookmarks failed: Error: Can't merge different item kinds (resource://gre/modules/SyncedBookmarksMirror.jsm:3366:11) JS Stack trace: ConsistencyError@SyncedBookmarksMirror.jsm:829:19
twoWayMerge@SyncedBookmarksMirror.jsm:3366:11 ...

The issue is that a bookmark is initially created with about:blank as the URI, so ends up with type="bookmark" in the mirror. When the place: URL is used, it ends up as type="query", so the merger gets upset.

The same issue happens for livemarks, but that appears to already be handled and have a test.

Kit, what do you think of this patch? (Note it also changes some log.trace calls with log.warn which make sense to me, but may not to you :)
Attachment #8957394 - Attachment is obsolete: true
Attachment #8957394 - Flags: review?(kit)
Comment on attachment 8957393 [details]
Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror.

https://reviewboard.mozilla.org/r/226326/#review232250

This looks good to me, and thanks for the test! I think we can factor this logic out into a separate method and clean up `twoWayMerge`, but, if you think that would make things worse, please re-flag me, and we can land your patch as-is.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3407
(Diff revision 2)
>        MirrorLog.error("Merging local folder ${localNode} and remote " +
>                        "non-folder ${remoteNode}", { localNode, remoteNode });
>        throw new SyncedBookmarksMirror.ConsistencyError(
>          "Can't merge folder and non-folder");
>      }
> +    // We are merging 2 non-folders. We allow bookmarks and queries to be

I wonder if we can simplify this function a bit, and handle livemarks the same way. How do you feel about removing the livemark check in the block above, adding a `hasCompatibleKind(otherNode)` method to `BookmarkNode` with logic for queries and livemarks, and checking that here? If we're not merging two folders, and the kinds aren't compatible, we throw.
Attachment #8957393 - Flags: review?(kit)
Priority: -- → P2
Comment on attachment 8957393 [details]
Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror.

https://reviewboard.mozilla.org/r/226326/#review233768


Code analysis found 5 defects in this patch:
 - 5 defects 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/SyncedBookmarksMirror.jsm:3420
(Diff revision 3)
> +    if (!localNode.hasCompatibleKind(remoteNode)) {
> +      MirrorLog.error("Merging local ${localNode} and remote ${remoteNode} " +
> +                      "with different kinds", { localNode, remoteNode });
> +      this.telemetryEvents.push({
> +        value: "kind-mismatch",
> +        extra: { local: ""+localNode.kind, remote: ""+remoteNode.kind },

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3420
(Diff revision 3)
> +    if (!localNode.hasCompatibleKind(remoteNode)) {
> +      MirrorLog.error("Merging local ${localNode} and remote ${remoteNode} " +
> +                      "with different kinds", { localNode, remoteNode });
> +      this.telemetryEvents.push({
> +        value: "kind-mismatch",
> +        extra: { local: ""+localNode.kind, remote: ""+remoteNode.kind },

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:372
(Diff revision 3)
> +        extra.local == SyncedBookmarksMirror.KIND.LIVEMARK &&
> +        extra.remote && typeof extra.remote == "string" &&
> +        extra.remote == SyncedBookmarksMirror.KIND.FOLDER) {
> +      sawMismatchEvent = true;
> +    }
> +  }

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:494
(Diff revision 3)
> +        extra.local == SyncedBookmarksMirror.KIND.BOOKMARK &&
> +        extra.remote && typeof extra.remote == "string" &&
> +        extra.remote == SyncedBookmarksMirror.KIND.FOLDER) {
> +      sawMismatchEvent = true;
> +    }
> +  }

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:510
(Diff revision 3)
> +          url: "about:blank",
> +        },
> +      ],
> +    });
> +
> +    let changes = await buf.apply();

Error: 'changes' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8957393 [details]
Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror.

https://reviewboard.mozilla.org/r/226326/#review234364

Awesome, thanks, Mark! Feel free to take or drop nits as you see fit.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:401
(Diff revision 4)
>                                        remoteTree, newRemoteContents);
> -      let mergedRoot = await merger.merge();
> +      let mergedRoot;
> +      try {
> +        mergedRoot = await merger.merge();
> +      } finally {
> -      for (let { value, extra } of merger.telemetryEvents) {
> +        for (let { value, extra } of merger.telemetryEvents) {

Oh, nice catch!

::: toolkit/components/places/SyncedBookmarksMirror.jsm:2933
(Diff revision 4)
> +   * - Local folders are compatible with remote livemarks, but not vice-versa
> +   * - Bookmarks and queries are always compatible.
> +   *
> +   * @return {Boolean}
> +   */
> +  hasCompatibleKind(remoteNode) {

Micro-nit: Let's call this `otherNode`.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3435
(Diff revision 4)
>          MirrorLog.trace("Merging folders ${localNode} and ${remoteNode}",
>                          { localNode, remoteNode });
>          await this.mergeChildListsIntoMergedNode(mergedNode, localNode, remoteNode);
>          return mergedNode;
>        }
> -
> +      // Otherwise it must be a livemark.

Unless you think the logging would be useful to keep (we don't log for queries, so I'd be inclined to drop it), I think you can drop this entire block, including the `return`, let it fall through to the end, and move this comment into `hasCompatibleKind`.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3447
(Diff revision 4)
> -          value: "kind",
> -          extra: { local: "folder", remote: "folder" },
> -        });
> -        return mergedNode;
> +      return mergedNode;
> -      }
> +    }
> -
> +    // Otherwise are compatible types, so return the merged node.

Micro-nit: types -> kinds, let's also mention we don't need to walk children for these kinds.

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:392
(Diff revision 4)
> +      }],
> +    });
> +    await PlacesTestUtils.markBookmarksAsSynced();
> +
> +    info("Make remote changes");
> +    await buf.store([{

Nit: Same comment as `bookmarkBBBB` below; `livemarkAAAA` is technically an orphan, the `parentid` is ignored, and the `parentName` doesn't match up.

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:444
(Diff revision 4)
> +
> +    // Now pretend that same records are already on the server.
> +    await buf.store([{
> +      id: "menu",
> +      type: "folder",
> +      children: ["bookmarkAAAA"],

Nit: Let's add `bookmarkBBBB` here, too. It's technically not necessary for this test, but the merger will treat B as an orphan, and my inner quibbler would feel better if that weren't a factor in this test. :-)

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:447
(Diff revision 4)
> +      id: "menu",
> +      type: "folder",
> +      children: ["bookmarkAAAA"],
> +    }, {
> +      id: "bookmarkAAAA",
> +      parentId: PlacesSyncUtils.bookmarks.guidToRecordId(PlacesUtils.bookmarks.menuGuid),

Nit: "menu" is shorter, but this property is spelled `parentid`, and the merger ignores it, anyway. :-)

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:520
(Diff revision 4)
> +      id: "menu",
> +      type: "folder",
> +      children: ["AAAAAAAAAAAA"],
> +    }, {
> +      id: "AAAAAAAAAAAA",
> +      parentId: PlacesSyncUtils.bookmarks.guidToRecordId(PlacesUtils.bookmarks.menuGuid),

Ditto.
Attachment #8957393 - Flags: review?(kit) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/41c707ae4d6d
ensure that a bookmark that later becomes a query doesn't break the mirror. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/41c707ae4d6d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: