commit 8092ca9c82bc57e2b60e8286aab1197c5f10f49c
author Saqib Amjad <saqiba@microsoft.com> Fri Apr 05 16:53:21 2024
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Apr 05 16:53:21 2024
tree 7f0a44030c96925d4df67b61c842cf8ca68f2e70
parent 09dd4ecebc2b02d1d7c872c68cefc3599d8b6b96

[bookmarks] Disallow moving a folder to itself or its descendant.

The browser window hangs if chrome.bookmarks.move public
JavaScript API is called with the same `parentId` as the `id`
of the node being moved (id "20" in the example below):

chrome.bookmarks.move("20", { "index": 1, "parentId": "20");

When a node is moved, it is first removed from its old parent and
then added to the new parent. In this case, BookmarkModel::Move
ends up adding the node as a child of itself. This leads to a couple of issues:
1. The following DCHECK is violated in BookmarkModel::Move in debug builds:

    DCHECK(!new_parent->HasAncestor(node));

2. GetSelfOrAncestorPermanentNode gets into an infinite loop in release builds causing the browser window to freeze:

    while (!node->is_permanent_node()) {
      CHECK(node->parent());
      node = node->parent();  <== parent is the same as node itself
    }

Since this is a public API, a buggy extension can easily cause the
browser to hang in release builds.

The bug can be fixed in BookmarkModel but in my opinion, the API
should return an error in this case since the caller is calling it
incorrectly. I have therefore updated BookmarksMoveFunction to return
an error when the destination is the same as the node.

The change also disallows moving a folder to its descendant as that
also leads to the same problem.

Fixed: 332743093
Change-Id: I66b8b93ebac6c5a31e9a0d0524f606f2eedad070
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5422430
Commit-Queue: Cliff Smolinsky <cliffsmo@microsoft.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1283202}