Fix ChromeBrowserProvider bookmark observer thread issue.

The BookmarkModelObserverTask added a bookmark model observer
on a background thread, which is not allowed.

In both cases of Remove and Update, there is no need for listening
to the observer as we can just track whether we are updating the
bookmark node internally.  This is consistent with how
AddBookmarkTask already worked.

BUG=838988

Change-Id: I43bf054b70e78907a99ba2a11fac62df0af6bc67
Reviewed-on: https://chromium-review.googlesource.com/1089497Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565667}

chrome/browser/BUILD.gn

@@ -2178,8 +2178,8 @@ jumbo_split_static_library("browser") {
       "android/profiles/profile_manager_utils.cc",
       "android/provider/blocking_ui_thread_async_request.cc",
       "android/provider/blocking_ui_thread_async_request.h",
-      "android/provider/bookmark_model_observer_task.cc",
-      "android/provider/bookmark_model_observer_task.h",
+      "android/provider/bookmark_model_task.cc",
+      "android/provider/bookmark_model_task.h",
       "android/provider/chrome_browser_provider.cc",
       "android/provider/chrome_browser_provider.h",
       "android/provider/run_on_ui_thread_blocking.h",

chrome/browser/android/provider/bookmark_model_observer_task.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/android/provider/bookmark_model_observer_task.h"
-
-#include "components/bookmarks/browser/bookmark_model.h"
-#include "components/bookmarks/browser/model_loader.h"
-#include "content/public/browser/browser_thread.h"
-
-using bookmarks::BookmarkModel;
-using bookmarks::BookmarkNode;
-using content::BrowserThread;
-
-BookmarkModelTask::BookmarkModelTask(BookmarkModel* model)
-    : model_(model) {
-  // Ensure the initialization of the native bookmark model.
-  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(model_);
-  model_->model_loader()->BlockTillLoaded();
-}
-
-BookmarkModel* BookmarkModelTask::model() const {
-  return model_;
-}
-
-BookmarkModelObserverTask::BookmarkModelObserverTask(
-    BookmarkModel* bookmark_model)
-    : BookmarkModelTask(bookmark_model) {
-  model()->AddObserver(this);
-}
-
-BookmarkModelObserverTask::~BookmarkModelObserverTask() {
-  model()->RemoveObserver(this);
-}
-
-void BookmarkModelObserverTask::BookmarkModelLoaded(BookmarkModel* model,
-                                                    bool ids_reassigned) {}
-
-void BookmarkModelObserverTask::BookmarkNodeMoved(
-    BookmarkModel* model,
-    const BookmarkNode* old_parent,
-    int old_index,
-    const BookmarkNode* new_parent,
-    int new_index) {
-}
-
-void BookmarkModelObserverTask::BookmarkNodeAdded(BookmarkModel* model,
-                                                  const BookmarkNode* parent,
-                                                  int index) {
-}
-
-void BookmarkModelObserverTask::BookmarkNodeRemoved(
-    BookmarkModel* model,
-    const BookmarkNode* parent,
-    int old_index,
-    const BookmarkNode* node,
-    const std::set<GURL>& removed_urls) {
-}
-
-void BookmarkModelObserverTask::BookmarkAllUserNodesRemoved(
-    BookmarkModel* model,
-    const std::set<GURL>& removed_urls) {
-}
-
-void BookmarkModelObserverTask::BookmarkNodeChanged(BookmarkModel* model,
-                                                    const BookmarkNode* node) {
-}
-
-void BookmarkModelObserverTask::BookmarkNodeFaviconChanged(
-    BookmarkModel* model,
-    const BookmarkNode* node) {
-}
-
-void BookmarkModelObserverTask::BookmarkNodeChildrenReordered(
-    BookmarkModel* model,
-    const BookmarkNode* node) {
-}

chrome/browser/android/provider/bookmark_model_observer_task.h

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_OBSERVER_TASK_H_
-#define CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_OBSERVER_TASK_H_
-
-#include "base/compiler_specific.h"
-#include "base/macros.h"
-#include "components/bookmarks/browser/bookmark_model_observer.h"
-
-// 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;
-
- private:
-  bookmarks::BookmarkModel* model_;
-
-  DISALLOW_COPY_AND_ASSIGN(BookmarkModelTask);
-};
-
-// Base class for bookmark model tasks that observe for model updates.
-class BookmarkModelObserverTask : public BookmarkModelTask,
-                                  public bookmarks::BookmarkModelObserver {
- public:
-  explicit BookmarkModelObserverTask(bookmarks::BookmarkModel* bookmark_model);
-  ~BookmarkModelObserverTask() override;
-
-  // bookmarks::BookmarkModelObserver:
-  void BookmarkModelLoaded(bookmarks::BookmarkModel* model,
-                           bool ids_reassigned) override;
-  void BookmarkNodeMoved(bookmarks::BookmarkModel* model,
-                         const bookmarks::BookmarkNode* old_parent,
-                         int old_index,
-                         const bookmarks::BookmarkNode* new_parent,
-                         int new_index) override;
-  void BookmarkNodeAdded(bookmarks::BookmarkModel* model,
-                         const bookmarks::BookmarkNode* parent,
-                         int index) override;
-  void BookmarkNodeRemoved(bookmarks::BookmarkModel* model,
-                           const bookmarks::BookmarkNode* parent,
-                           int old_index,
-                           const bookmarks::BookmarkNode* node,
-                           const std::set<GURL>& removed_urls) override;
-  void BookmarkAllUserNodesRemoved(bookmarks::BookmarkModel* model,
-                                   const std::set<GURL>& removed_urls) override;
-  void BookmarkNodeChanged(bookmarks::BookmarkModel* model,
-                           const bookmarks::BookmarkNode* node) override;
-  void BookmarkNodeFaviconChanged(bookmarks::BookmarkModel* model,
-                                  const bookmarks::BookmarkNode* node) override;
-  void BookmarkNodeChildrenReordered(
-      bookmarks::BookmarkModel* model,
-      const bookmarks::BookmarkNode* node) override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(BookmarkModelObserverTask);
-};
-
-#endif  // CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_OBSERVER_TASK_H_

chrome/browser/android/provider/bookmark_model_task.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// 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"
+
+using bookmarks::BookmarkModel;
+using bookmarks::BookmarkNode;
+using content::BrowserThread;
+
+BookmarkModelTask::BookmarkModelTask(BookmarkModel* model) : model_(model) {
+  // Ensure the initialization of the native bookmark model.
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(model_);
+  model_->model_loader()->BlockTillLoaded();
+}
+
+BookmarkModel* BookmarkModelTask::model() const {
+  return model_;
+}

chrome/browser/android/provider/bookmark_model_task.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#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"
+
+// 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;
+
+ private:
+  bookmarks::BookmarkModel* model_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkModelTask);
+};
+
+#endif  // CHROME_BROWSER_ANDROID_PROVIDER_BOOKMARK_MODEL_TASK_H_

chrome/browser/android/provider/chrome_browser_provider.cc

@@ -20,7 +20,7 @@
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/provider/blocking_ui_thread_async_request.h"
-#include "chrome/browser/android/provider/bookmark_model_observer_task.h"
+#include "chrome/browser/android/provider/bookmark_model_task.h"
 #include "chrome/browser/android/provider/run_on_ui_thread_blocking.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/managed_bookmark_service_factory.h"
@@ -216,80 +216,69 @@ class AddBookmarkTask : public BookmarkModelTask {
 };
 
 // Utility method to remove a bookmark.
-class RemoveBookmarkTask : public BookmarkModelObserverTask {
+class RemoveBookmarkTask : public BookmarkModelTask {
  public:
   explicit RemoveBookmarkTask(BookmarkModel* model)
-      : BookmarkModelObserverTask(model),
-        deleted_(0),
-        id_to_delete_(kInvalidBookmarkId) {}
-  ~RemoveBookmarkTask() override {}
+      : BookmarkModelTask(model) {}
 
   int Run(const int64_t id) {
-    id_to_delete_ = id;
-    RunOnUIThreadBlocking::Run(
-        base::Bind(&RemoveBookmarkTask::RunOnUIThread, model(), id));
-    return deleted_;
+    bool did_delete = false;
+    RunOnUIThreadBlocking::Run(base::Bind(&RemoveBookmarkTask::RunOnUIThread,
+                                          model(), id, &did_delete));
+    return did_delete ? 1 : 0;
   }
 
-  static void RunOnUIThread(BookmarkModel* model, const int64_t id) {
+  static void RunOnUIThread(BookmarkModel* model,
+                            const int64_t id,
+                            bool* did_delete) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     const BookmarkNode* node = bookmarks::GetBookmarkNodeByID(model, id);
-    if (node && node->parent())
+    if (node && node->parent()) {
       model->Remove(node);
+      *did_delete = true;
+    }
   }
 
-  // Verify that the bookmark was actually removed. Called synchronously.
-  void BookmarkNodeRemoved(BookmarkModel* bookmark_model,
-                           const BookmarkNode* parent,
-                           int old_index,
-                           const BookmarkNode* node,
-                           const std::set<GURL>& removed_urls) override {
-    if (bookmark_model == model() && node->id() == id_to_delete_)
-        ++deleted_;
-  }
-
- private:
-  int deleted_;
-  int64_t id_to_delete_;
-
   DISALLOW_COPY_AND_ASSIGN(RemoveBookmarkTask);
 };
 
 // Utility method to update a bookmark.
-class UpdateBookmarkTask : public BookmarkModelObserverTask {
+class UpdateBookmarkTask : public BookmarkModelTask {
  public:
   explicit UpdateBookmarkTask(BookmarkModel* model)
-      : BookmarkModelObserverTask(model),
-        updated_(0),
-        id_to_update_(kInvalidBookmarkId){}
-  ~UpdateBookmarkTask() override {}
+      : BookmarkModelTask(model) {}
 
   int Run(const int64_t id,
           const base::string16& title,
           const base::string16& url,
           const int64_t parent_id) {
-    id_to_update_ = id;
-    RunOnUIThreadBlocking::Run(
-        base::Bind(&UpdateBookmarkTask::RunOnUIThread,
-                   model(), id, title, url, parent_id));
-    return updated_;
+    bool did_update = false;
+    RunOnUIThreadBlocking::Run(base::Bind(&UpdateBookmarkTask::RunOnUIThread,
+                                          model(), id, title, url, parent_id,
+                                          &did_update));
+    return did_update ? 1 : 0;
   }
 
   static void RunOnUIThread(BookmarkModel* model,
                             const int64_t id,
                             const base::string16& title,
                             const base::string16& url,
-                            const int64_t parent_id) {
+                            const int64_t parent_id,
+                            bool* did_update) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     const BookmarkNode* node = bookmarks::GetBookmarkNodeByID(model, id);
     if (node) {
-      if (node->GetTitle() != title)
+      if (node->GetTitle() != title) {
         model->SetTitle(node, title);
+        *did_update = true;
+      }
 
       if (node->type() == BookmarkNode::URL) {
         GURL bookmark_url = ParseAndMaybeAppendScheme(url, kDefaultUrlScheme);
-        if (bookmark_url != node->url())
+        if (bookmark_url != node->url()) {
           model->SetURL(node, bookmark_url);
+          *did_update = true;
+        }
       }
 
       if (parent_id >= 0 &&
@@ -297,23 +286,14 @@ class UpdateBookmarkTask : public BookmarkModelObserverTask {
         const BookmarkNode* new_parent =
             bookmarks::GetBookmarkNodeByID(model, parent_id);
 
-        if (new_parent)
+        if (new_parent) {
           model->Move(node, new_parent, 0);
+          *did_update = true;
+        }
       }
     }
   }
 
-  // Verify that the bookmark was actually updated. Called synchronously.
-  void BookmarkNodeChanged(BookmarkModel* bookmark_model,
-                           const BookmarkNode* node) override {
-    if (bookmark_model == model() && node->id() == id_to_update_)
-      ++updated_;
-  }
-
- private:
-  int updated_;
-  int64_t id_to_update_;
-
   DISALLOW_COPY_AND_ASSIGN(UpdateBookmarkTask);
 };