Fix permanent bookmark nodes being deleted

Deleting permanent folders is not handled well and violates an existing
DCHECK in BookmarkModel::RemoveNodeFromIndexRecursive(), but no layer
enforces this precondition for an API that is exposed to extensions
(and possibly other callers).

The patch now adds a DCHECK in BookmarkModel::Remove() to explicitly
make it a precondition violation, and updates the extensions codepath
to prevent such case.

Specifically, one side effect is that prior to this patch observers
were notified for OnWillRemoveBookmarks(), which could explain the
sync-related CHECK failures in the linked bug.

Bug: 1021829
Change-Id: Ia4f013258420c024c06e0d6ce54f5b1868cff62e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899374
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713967}

chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc

@@ -162,6 +162,14 @@ bookmark_manager_private::BookmarkNodeData CreateApiBookmarkNodeData(
   return node_data;
 }
 
+bool HasPermanentNodes(const std::vector<const BookmarkNode*>& list) {
+  for (const BookmarkNode* node : list) {
+    if (node->is_permanent_node())
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 BookmarkManagerPrivateEventRouter::BookmarkManagerPrivateEventRouter(
@@ -316,6 +324,10 @@ bool ClipboardBookmarkManagerFunction::CopyOrCut(bool cut,
     error_ = bookmark_keys::kModifyManagedError;
     return false;
   }
+  if (cut && HasPermanentNodes(nodes)) {
+    error_ = bookmark_keys::kModifySpecialError;
+    return false;
+  }
   bookmarks::CopyToClipboard(model, nodes, cut);
   return true;
 }

chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.h"
 
+#include "base/memory/scoped_refptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ -33,6 +34,7 @@ class BookmarkManagerPrivateApiUnitTest : public ExtensionServiceTestBase {
     node_id_ = base::NumberToString(node->id());
   }
 
+  const bookmarks::BookmarkModel* model() const { return model_; }
   std::string node_id() const { return node_id_; }
 
  private:
@@ -47,15 +49,14 @@ class BookmarkManagerPrivateApiUnitTest : public ExtensionServiceTestBase {
 // Regression test for https://crbug.com/739260.
 TEST_F(BookmarkManagerPrivateApiUnitTest, RunOnDeletedNode) {
   // Remove our only bookmark node.
-  scoped_refptr<BookmarksRemoveFunction> remove_function(
-      new BookmarksRemoveFunction());
+  auto remove_function = base::MakeRefCounted<BookmarksRemoveFunction>();
   api_test_utils::RunFunction(remove_function.get(),
                               base::StringPrintf("[\"%s\"]", node_id().c_str()),
                               profile());
 
   // Call bookmarkManagerPrivate.copy() with the removed bookmark node's id.
-  scoped_refptr<BookmarkManagerPrivateCopyFunction> copy_function(
-      new BookmarkManagerPrivateCopyFunction());
+  auto copy_function =
+      base::MakeRefCounted<BookmarkManagerPrivateCopyFunction>();
   EXPECT_EQ(
       "Could not find bookmark nodes with given ids.",
       api_test_utils::RunFunctionAndReturnError(
@@ -63,4 +64,17 @@ TEST_F(BookmarkManagerPrivateApiUnitTest, RunOnDeletedNode) {
           base::StringPrintf("[[\"%s\"]]", node_id().c_str()), profile()));
 }
 
+// Tests that calling bookmarkManagerPrivate.cut() to cut a permanent bookmark
+// node into the clipboard gracefully fails.
+// Regression test for https://crbug.com/1021829.
+TEST_F(BookmarkManagerPrivateApiUnitTest, RunCutOnPermanentNode) {
+  auto cut_function = base::MakeRefCounted<BookmarkManagerPrivateCutFunction>();
+  std::string node_id =
+      base::NumberToString(model()->bookmark_bar_node()->id());
+  EXPECT_EQ("Can't modify the root bookmark folders.",
+            api_test_utils::RunFunctionAndReturnError(
+                cut_function.get(),
+                base::StringPrintf("[[\"%s\"]]", node_id.c_str()), profile()));
+}
+
 }  // namespace extensions

components/bookmarks/browser/bookmark_model.cc

@@ -205,6 +205,7 @@ void BookmarkModel::Remove(const BookmarkNode* node) {
   DCHECK(parent);
   size_t index = size_t{parent->GetIndexOf(node)};
   DCHECK_NE(size_t{-1}, index);
+  DCHECK(!is_permanent_node(node));
 
   for (BookmarkModelObserver& observer : observers_)
     observer.OnWillRemoveBookmarks(this, parent, index, node);

components/bookmarks/browser/bookmark_model.h

@@ -126,7 +126,8 @@ class BookmarkModel : public BookmarkUndoProvider,
   bool IsDoingExtensiveChanges() const { return extensive_changes_ > 0; }
 
   // Removes |node| from the model and deletes it. Removing a folder node
-  // recursively removes all nodes. Observers are notified immediately.
+  // recursively removes all nodes. Observers are notified immediately. |node|
+  // must not be a permanent node.
   void Remove(const BookmarkNode* node);
 
   // Removes all the non-permanent bookmark nodes that are editable by the user.