Fix partner bookmarks root using wrong GUID

Partner bookmarks live outside BookmarkModel and are exposed to Java via
dedicated JNI APIs. The root is instantiated by PartnerBookmarksReader
and owned by PartnerBookmarksShim.

Prior to this patch, the construction of the root in
PartnerBookmarksReader was using BookmarkPermanentNode's constructor
with type FOLDER, which (currently) gets interpreted as managed
bookmarks (controlled by enterprise policies).

This leads to the wrong bookmark GUID being used (the one for managed
bookmarks' root) but is otherwise harmless, because the GUID is actually
never read.

The patch fixes this by avoiding the construction of a
BookmarkPermanentNode and instead constructing a regular BookmarkNode to
be used as partner bookmarks root, using a randomly-generated GUI.

Change-Id: I2234051b2a4f00b77a0d0b9391eb16f5107ee97c
Bug: 1060311
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141973
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757891}

chrome/browser/android/bookmarks/bookmark_bridge.cc

@@ -23,6 +23,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/android/chrome_jni_headers/BookmarkBridge_jni.h"
+#include "chrome/browser/android/bookmarks/partner_bookmarks_reader.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/managed_bookmark_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ -59,7 +60,6 @@ using bookmarks::android::JavaBookmarkIdGetId;
 using bookmarks::android::JavaBookmarkIdGetType;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
-using bookmarks::BookmarkPermanentNode;
 using bookmarks::BookmarkType;
 using content::BrowserThread;
 
@@ -199,7 +199,7 @@ void BookmarkBridge::LoadEmptyPartnerBookmarkShimForTesting(
   if (partner_bookmarks_shim_->IsLoaded())
       return;
   partner_bookmarks_shim_->SetPartnerBookmarksRoot(
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER));
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting());
   PartnerBookmarksShim::DisablePartnerBookmarksEditing();
   DCHECK(partner_bookmarks_shim_->IsLoaded());
 }
@@ -211,8 +211,8 @@ void BookmarkBridge::LoadFakePartnerBookmarkShimForTesting(
     const JavaParamRef<jobject>& obj) {
   if (partner_bookmarks_shim_->IsLoaded())
     return;
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
   BookmarkNode* partner_bookmark_a =
       root_partner_node->Add(std::make_unique<BookmarkNode>(
           1, base::GenerateGUID(), GURL("http://www.a.com")));

chrome/browser/android/bookmarks/partner_bookmarks_reader.cc

@@ -17,7 +17,6 @@
 #include "chrome/browser/favicon/large_icon_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/large_icon_service_impl.h"
 #include "components/favicon_base/favicon_types.h"
@@ -37,7 +36,6 @@ using base::android::JavaParamRef;
 using base::android::JavaRef;
 using base::android::ScopedJavaGlobalRef;
 using bookmarks::BookmarkNode;
-using bookmarks::BookmarkPermanentNode;
 using content::BrowserThread;
 
 namespace {
@@ -110,6 +108,10 @@ const BookmarkNode* GetNodeByID(const BookmarkNode* parent, int64_t id) {
   return nullptr;
 }
 
+std::unique_ptr<BookmarkNode> CreatePartnerBookmarksRoot(int id) {
+  return std::make_unique<BookmarkNode>(id, base::GenerateGUID(), GURL());
+}
+
 }  // namespace
 
 PartnerBookmarksReader::PartnerBookmarksReader(
@@ -206,9 +208,8 @@ jlong PartnerBookmarksReader::AddPartnerBookmark(
     node_id = node->id();
     const_cast<BookmarkNode*>(parent)->Add(std::move(node));
   } else {
-    std::unique_ptr<BookmarkPermanentNode> node =
-        std::make_unique<BookmarkPermanentNode>(wip_next_available_id_++,
-                                                BookmarkNode::FOLDER);
+    std::unique_ptr<BookmarkNode> node =
+        CreatePartnerBookmarksRoot(wip_next_available_id_++);
     node_id = node->id();
     node->SetTitle(title);
     wip_partner_bookmarks_root_ = std::move(node);
@@ -216,6 +217,12 @@ jlong PartnerBookmarksReader::AddPartnerBookmark(
   return node_id;
 }
 
+// static
+std::unique_ptr<BookmarkNode>
+PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting() {
+  return CreatePartnerBookmarksRoot(/*id=*/0);
+}
+
 void PartnerBookmarksReader::GetFavicon(const GURL& page_url,
                                         Profile* profile,
                                         bool fallback_to_server,

chrome/browser/android/bookmarks/partner_bookmarks_reader.h

@@ -11,7 +11,7 @@
 
 #include "base/android/jni_weak_ref.h"
 #include "base/macros.h"
-#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 
 namespace favicon {
 class LargeIconService;
@@ -48,6 +48,9 @@ class PartnerBookmarksReader {
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
 
+  static std::unique_ptr<bookmarks::BookmarkNode>
+  CreatePartnerBookmarksRootForTesting();
+
  private:
   // These values are persisted to logs. Entries should not be renumbered and
   // numeric values should never be reused.

chrome/browser/android/bookmarks/partner_bookmarks_shim.cc

@@ -13,6 +13,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/android/bookmarks/partner_bookmarks_reader.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"

chrome/browser/android/bookmarks/partner_bookmarks_shim.h

@@ -16,7 +16,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/supports_user_data.h"
-#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/url_formatter/url_formatter.h"
 #include "net/base/escape.h"

chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc

@@ -10,9 +10,8 @@
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/android/bookmarks/partner_bookmarks_reader.h"
 #include "chrome/test/base/testing_profile.h"
-#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/browser_task_environment.h"
@@ -20,9 +19,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
-using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
-using bookmarks::BookmarkPermanentNode;
 using testing::_;
 
 class MockObserver : public PartnerBookmarksShim::Observer {
@@ -37,36 +34,17 @@ class MockObserver : public PartnerBookmarksShim::Observer {
 
 class PartnerBookmarksShimTest : public testing::Test {
  public:
-  PartnerBookmarksShimTest() : model_(nullptr) {}
+  PartnerBookmarksShimTest() = default;
 
-  TestingProfile* profile() const { return profile_.get(); }
   PartnerBookmarksShim* partner_bookmarks_shim() const {
     return PartnerBookmarksShim::BuildForBrowserContext(profile_.get());
   }
 
-  const BookmarkNode* AddBookmark(const BookmarkNode* parent,
-                                  const GURL& url,
-                                  const base::string16& title) {
-    if (!parent)
-      parent = model_->bookmark_bar_node();
-    return model_->AddURL(parent, parent->children().size(), title, url);
-  }
-
-  const BookmarkNode* AddFolder(const BookmarkNode* parent,
-                                const base::string16& title) {
-    if (!parent)
-      parent = model_->bookmark_bar_node();
-    return model_->AddFolder(parent, parent->children().size(), title);
-  }
-
  protected:
   // testing::Test
   void SetUp() override {
     profile_.reset(new TestingProfile());
     profile_->CreateBookmarkModel(true);
-
-    model_ = BookmarkModelFactory::GetForBrowserContext(profile_.get());
-    bookmarks::test::WaitForBookmarkModelToLoad(model_);
   }
 
   void TearDown() override {
@@ -80,7 +58,6 @@ class PartnerBookmarksShimTest : public testing::Test {
 
   content::BrowserTaskEnvironment task_environment_;
 
-  BookmarkModel* model_;
   MockObserver observer_;
 
  private:
@@ -88,9 +65,9 @@ class PartnerBookmarksShimTest : public testing::Test {
 };
 
 TEST_F(PartnerBookmarksShimTest, GetNodeByID) {
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
-  BookmarkPermanentNode* root_partner_node_ptr = root_partner_node.get();
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
+  BookmarkNode* root_partner_node_ptr = root_partner_node.get();
   BookmarkNode* partner_folder1 = root_partner_node->Add(
       std::make_unique<BookmarkNode>(1, base::GenerateGUID(), GURL()));
 
@@ -126,10 +103,10 @@ TEST_F(PartnerBookmarksShimTest, ObserverNotifiedOfLoadNoPartnerBookmarks) {
 
 TEST_F(PartnerBookmarksShimTest, ObserverNotifiedOfLoadWithPartnerBookmarks) {
   EXPECT_CALL(observer_, PartnerShimLoaded(_)).Times(0);
-  int64_t id = 5;
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(id++, BookmarkNode::FOLDER);
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
 
+  int64_t id = 5;
   root_partner_node->Add(std::make_unique<BookmarkNode>(
       id++, base::GenerateGUID(), GURL("http://www.a.com")));
 
@@ -147,9 +124,9 @@ TEST_F(PartnerBookmarksShimTest, RemoveBookmarks) {
   EXPECT_CALL(observer_, PartnerShimLoaded(shim)).Times(0);
   EXPECT_CALL(observer_, PartnerShimChanged(shim)).Times(0);
 
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
-  BookmarkPermanentNode* root_partner_node_ptr = root_partner_node.get();
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
+  BookmarkNode* root_partner_node_ptr = root_partner_node.get();
   root_partner_node->SetTitle(base::ASCIIToUTF16("Partner bookmarks"));
 
   BookmarkNode* partner_folder1 = root_partner_node->Add(
@@ -238,9 +215,9 @@ TEST_F(PartnerBookmarksShimTest, RenameBookmarks) {
   EXPECT_CALL(observer_, PartnerShimLoaded(shim)).Times(0);
   EXPECT_CALL(observer_, PartnerShimChanged(shim)).Times(0);
 
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
-  BookmarkPermanentNode* root_partner_node_ptr = root_partner_node.get();
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
+  BookmarkNode* root_partner_node_ptr = root_partner_node.get();
   root_partner_node->SetTitle(base::ASCIIToUTF16("Partner bookmarks"));
 
   BookmarkNode* partner_folder1 = root_partner_node->Add(
@@ -322,8 +299,8 @@ TEST_F(PartnerBookmarksShimTest, SaveLoadProfile) {
     EXPECT_CALL(observer_, PartnerShimLoaded(shim)).Times(0);
     EXPECT_CALL(observer_, PartnerShimChanged(shim)).Times(0);
 
-    std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-        std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+    std::unique_ptr<BookmarkNode> root_partner_node =
+        PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
     root_partner_node->SetTitle(base::ASCIIToUTF16("Partner bookmarks"));
 
     BookmarkNode* partner_folder1 = root_partner_node->Add(
@@ -377,8 +354,8 @@ TEST_F(PartnerBookmarksShimTest, DisableEditing) {
   EXPECT_CALL(observer_, PartnerShimLoaded(shim)).Times(0);
   EXPECT_CALL(observer_, PartnerShimChanged(shim)).Times(0);
 
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
   root_partner_node->SetTitle(base::ASCIIToUTF16("Partner bookmarks"));
 
   BookmarkNode* partner_bookmark1 =
@@ -410,8 +387,8 @@ TEST_F(PartnerBookmarksShimTest, DisableEditing) {
 }
 
 TEST_F(PartnerBookmarksShimTest, GetPartnerBookmarksMatchingProperties) {
-  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
-      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+  std::unique_ptr<BookmarkNode> root_partner_node =
+      PartnerBookmarksReader::CreatePartnerBookmarksRootForTesting();
   BookmarkNode* partner_folder1 = root_partner_node->Add(
       std::make_unique<BookmarkNode>(1, base::GenerateGUID(), GURL()));
   partner_folder1->SetTitle(base::ASCIIToUTF16("Folder1"));