Add sequence checker to BookmarkModel

BookmarkModel can only be used from the UI thread, so this patch
enforces that with DCHECKs by adopting base/sequence_checker.h.

Such enforcement surfaces a few questionable usages in
chrome/browser/android/provider and corresponding tests, both of which
are updated. Most importantly, bookmarks::ModelLoader is plumbed
separately because it's thread safe, whereas access to BookmarkModel
is strictly limited to the UI thread.

Bug: None
Change-Id: I70e526e42982faf9e17d7d54f4a71eed34e2d5f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903130Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720861}

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java

@@ -246,15 +246,17 @@ public class BookmarkTest {
     @SmallTest
     public void testUrlComposition() {
         readPartnerBookmarks();
-        BookmarkId mobileId = mBookmarkModel.getMobileFolderId();
-        BookmarkId bookmarkBarId = mBookmarkModel.getDesktopFolderId();
-        BookmarkId otherId = mBookmarkModel.getOtherFolderId();
-        Assert.assertEquals("chrome-native://bookmarks/folder/" + mobileId,
-                BookmarkUIState.createFolderUrl(mobileId).toString());
-        Assert.assertEquals("chrome-native://bookmarks/folder/" + bookmarkBarId,
-                BookmarkUIState.createFolderUrl(bookmarkBarId).toString());
-        Assert.assertEquals("chrome-native://bookmarks/folder/" + otherId,
-                BookmarkUIState.createFolderUrl(otherId).toString());
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            BookmarkId mobileId = mBookmarkModel.getMobileFolderId();
+            BookmarkId bookmarkBarId = mBookmarkModel.getDesktopFolderId();
+            BookmarkId otherId = mBookmarkModel.getOtherFolderId();
+            Assert.assertEquals("chrome-native://bookmarks/folder/" + mobileId,
+                    BookmarkUIState.createFolderUrl(mobileId).toString());
+            Assert.assertEquals("chrome-native://bookmarks/folder/" + bookmarkBarId,
+                    BookmarkUIState.createFolderUrl(bookmarkBarId).toString());
+            Assert.assertEquals("chrome-native://bookmarks/folder/" + otherId,
+                    BookmarkUIState.createFolderUrl(otherId).toString());
+        });
     }
 
     @Test

chrome/browser/android/provider/bookmark_model_task.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/android/provider/bookmark_model_task.h"
+
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/model_loader.h"
 #include "content/public/browser/browser_thread.h"
@@ -11,13 +12,18 @@ using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 using content::BrowserThread;
 
-BookmarkModelTask::BookmarkModelTask(BookmarkModel* model) : model_(model) {
+BookmarkModelTask::BookmarkModelTask(
+    base::WeakPtr<bookmarks::BookmarkModel> model,
+    scoped_refptr<bookmarks::ModelLoader> model_loader)
+    : model_(std::move(model)) {
   // Ensure the initialization of the native bookmark model.
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(model_);
-  model_->model_loader()->BlockTillLoaded();
+  model_loader->BlockTillLoaded();
 }
 
-BookmarkModel* BookmarkModelTask::model() const {
+BookmarkModelTask::~BookmarkModelTask() = default;
+
+base::WeakPtr<BookmarkModel> BookmarkModelTask::model() const {
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
   return model_;
 }

chrome/browser/android/provider/bookmark_model_task.h

@@ -5,20 +5,28 @@
 #ifndef CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_TASK_H_
 #define CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_TASK_H_
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "components/bookmarks/browser/bookmark_model_observer.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+
+namespace bookmarks {
+class BookmarkModel;
+class ModelLoader;
+}  // namespace bookmarks
 
 // Base class for synchronous tasks that involve the bookmark model.
 // Ensures the model has been loaded before accessing it.
 // Must not be created from the UI thread.
 class BookmarkModelTask {
  public:
-  explicit BookmarkModelTask(bookmarks::BookmarkModel* model);
-  bookmarks::BookmarkModel* model() const;
+  BookmarkModelTask(base::WeakPtr<bookmarks::BookmarkModel> model,
+                    scoped_refptr<bookmarks::ModelLoader> model_loader);
+  ~BookmarkModelTask();
+
+  base::WeakPtr<bookmarks::BookmarkModel> model() const;
 
  private:
-  bookmarks::BookmarkModel* model_;
+  base::WeakPtr<bookmarks::BookmarkModel> model_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelTask);
 };

chrome/browser/android/provider/chrome_browser_provider.h

@@ -21,6 +21,11 @@
 class AndroidHistoryProviderService;
 class Profile;
 
+namespace bookmarks {
+class BookmarkModel;
+class ModelLoader;
+}  // namespace bookmarks
+
 namespace history {
 class TopSites;
 }
@@ -169,13 +174,14 @@ class ChromeBrowserProvider : public bookmarks::BaseBookmarkModelObserver,
 
   // Profile must outlive this object.
   //
-  // BookmarkModel, HistoryService and history::TopSites lifetime is bound to
-  // the lifetime of Profile, they are safe to use as long as the Profile is
-  // alive.
+  // HistoryService and history::TopSites lifetime is bound to the lifetime of
+  // Profile, they are safe to use as long as the Profile is alive.
   Profile* profile_;
-  bookmarks::BookmarkModel* bookmark_model_;
   scoped_refptr<history::TopSites> top_sites_;
 
+  base::WeakPtr<bookmarks::BookmarkModel> bookmark_model_;
+  scoped_refptr<bookmarks::ModelLoader> bookmark_model_loader_;
+
   std::unique_ptr<AndroidHistoryProviderService> service_;
 
   base::CancelableTaskTracker cancelable_task_tracker_;

chrome/browser/history/chrome_history_backend_client.cc

@@ -28,8 +28,8 @@ const base::FilePath::CharType kAndroidCacheFilename[] =
 #endif
 
 ChromeHistoryBackendClient::ChromeHistoryBackendClient(
-    bookmarks::ModelLoader* model_loader)
-    : model_loader_(model_loader) {}
+    scoped_refptr<bookmarks::ModelLoader> model_loader)
+    : model_loader_(std::move(model_loader)) {}
 
 ChromeHistoryBackendClient::~ChromeHistoryBackendClient() {
 }

chrome/browser/history/chrome_history_backend_client.h

@@ -18,7 +18,8 @@ class ModelLoader;
 // to provides access to embedder-specific features.
 class ChromeHistoryBackendClient : public history::HistoryBackendClient {
  public:
-  explicit ChromeHistoryBackendClient(bookmarks::ModelLoader* model_loader);
+  explicit ChromeHistoryBackendClient(
+      scoped_refptr<bookmarks::ModelLoader> model_loader);
   ~ChromeHistoryBackendClient() override;
 
   // history::HistoryBackendClient implementation.

chrome/browser/history/chrome_history_client.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/profiles/sql_init_error_message_ids.h"
 #include "chrome/browser/ui/profile_error_dialog.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/model_loader.h"
 #include "components/history/core/browser/history_service.h"
 
 ChromeHistoryClient::ChromeHistoryClient(

components/bookmarks/browser/bookmark_model.h

@@ -18,6 +18,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/sequence_checker.h"
 #include "base/strings/string16.h"
 #include "components/bookmarks/browser/bookmark_client.h"
 #include "components/bookmarks/browser/bookmark_node.h"
@@ -83,30 +84,49 @@ class BookmarkModel : public BookmarkUndoProvider,
             const scoped_refptr<base::SequencedTaskRunner>& ui_task_runner);
 
   // Returns true if the model finished loading.
-  bool loaded() const { return loaded_; }
+  bool loaded() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return loaded_;
+  }
 
   // Returns the object responsible for tracking loading.
-  ModelLoader* model_loader() { return model_loader_.get(); }
+  scoped_refptr<ModelLoader> model_loader();
 
   // Returns the root node. The 'bookmark bar' node and 'other' node are
   // children of the root node.
-  const BookmarkNode* root_node() const { return root_; }
+  const BookmarkNode* root_node() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return root_;
+  }
 
   // Returns the 'bookmark bar' node. This is NULL until loaded.
-  const BookmarkNode* bookmark_bar_node() const { return bookmark_bar_node_; }
+  const BookmarkNode* bookmark_bar_node() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return bookmark_bar_node_;
+  }
 
   // Returns the 'other' node. This is NULL until loaded.
-  const BookmarkNode* other_node() const { return other_node_; }
+  const BookmarkNode* other_node() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return other_node_;
+  }
 
   // Returns the 'mobile' node. This is NULL until loaded.
-  const BookmarkNode* mobile_node() const { return mobile_node_; }
+  const BookmarkNode* mobile_node() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return mobile_node_;
+  }
 
-  bool is_root_node(const BookmarkNode* node) const { return node == root_; }
+  bool is_root_node(const BookmarkNode* node) const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return node == root_;
+  }
 
   // Returns whether the given |node| is one of the permanent nodes - root node,
   // 'bookmark bar' node, 'other' node or 'mobile' node, or one of the root
   // nodes supplied by the |client_|.
   bool is_permanent_node(const BookmarkNode* node) const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     return node && (node == root_ || node->parent() == root_);
   }
 
@@ -174,23 +194,18 @@ class BookmarkModel : public BookmarkUndoProvider,
   const BookmarkNode* GetMostRecentlyAddedUserNodeForURL(const GURL& url);
 
   // Returns true if there are bookmarks, otherwise returns false.
-  // This method is thread safe.
   bool HasBookmarks();
 
   // Returns true is there is no user created bookmarks or folders.
   bool HasNoUserCreatedBookmarksOrFolders();
 
   // Returns true if the specified URL is bookmarked.
-  //
-  // If not on the main thread you *must* invoke BlockTillLoaded first.
   bool IsBookmarked(const GURL& url);
 
   // Returns, by reference in |bookmarks|, the set of bookmarked urls and their
   // titles. This returns the unique set of URLs. For example, if two bookmarks
   // reference the same URL only one entry is added not matter the titles are
   // same or not.
-  //
-  // If not on the main thread you *must* invoke BlockTillLoaded first.
   void GetBookmarks(std::vector<UrlAndTitle>* urls);
 
   // Adds a new folder node at the specified position with the given |guid| and
@@ -305,6 +320,10 @@ class BookmarkModel : public BookmarkUndoProvider,
 
   void SetUndoDelegate(BookmarkUndoDelegate* undo_delegate);
 
+  base::WeakPtr<BookmarkModel> AsWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
  private:
   friend class BookmarkCodecTest;
   friend class BookmarkModelFaviconTest;
@@ -425,6 +444,8 @@ class BookmarkModel : public BookmarkUndoProvider,
 
   scoped_refptr<ModelLoader> model_loader_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   base::WeakPtrFactory<BookmarkModel> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModel);

ios/chrome/browser/history/history_backend_client_impl.cc

@@ -10,8 +10,8 @@
 #include "url/gurl.h"
 
 HistoryBackendClientImpl::HistoryBackendClientImpl(
-    bookmarks::ModelLoader* model_loader)
-    : model_loader_(model_loader) {}
+    scoped_refptr<bookmarks::ModelLoader> model_loader)
+    : model_loader_(std::move(model_loader)) {}
 
 HistoryBackendClientImpl::~HistoryBackendClientImpl() {
 }

ios/chrome/browser/history/history_backend_client_impl.h

@@ -19,7 +19,8 @@ class ModelLoader;
 
 class HistoryBackendClientImpl : public history::HistoryBackendClient {
  public:
-  explicit HistoryBackendClientImpl(bookmarks::ModelLoader* model_loader);
+  explicit HistoryBackendClientImpl(
+      scoped_refptr<bookmarks::ModelLoader> model_loader);
   ~HistoryBackendClientImpl() override;
 
  private:

ios/chrome/browser/history/history_client_impl.cc

@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/model_loader.h"
 #include "components/history/core/browser/history_service.h"
 #include "ios/chrome/browser/history/history_backend_client_impl.h"
 #include "ios/chrome/browser/history/history_utils.h"