bookmarks: Fix theoretical race when changing titles

A BookmarkNode's title can only be changed from the UI thread, but is
also read by the history backend thread via
HistoryBookmarkModel::GetBookmarks(). Hence, a lock must be acquired
before changing the title.

In practice, this race should be extremely unlikely (if not impossible)
since assignment of base::string16 may as well be atomic in practice.

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

components/bookmarks/browser/bookmark_model.cc

@@ -388,7 +388,7 @@ void BookmarkModel::SetTitle(const BookmarkNode* node,
   // title changed but should be excluded from the index.
   if (node->is_url())
     titled_url_index_->Remove(node);
-  AsMutable(node)->SetTitle(title);
+  url_index_->SetTitle(AsMutable(node), title);
   if (node->is_url())
     titled_url_index_->Add(node);
 

components/bookmarks/browser/url_index.cc

@@ -54,6 +54,13 @@ void UrlIndex::SetUrl(BookmarkNode* node, const GURL& url) {
   AddImpl(node);
 }
 
+void UrlIndex::SetTitle(BookmarkNode* node, const base::string16& title) {
+  // Acquiring the lock is necessary to avoid races with
+  // UrlIndex::GetBookmarks().
+  base::AutoLock url_lock(url_lock_);
+  node->SetTitle(title);
+}
+
 void UrlIndex::GetNodesWithIconUrl(const GURL& icon_url,
                                    std::set<const BookmarkNode*>* nodes) {
   base::AutoLock url_lock(url_lock_);

components/bookmarks/browser/url_index.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/history_bookmark_model.h"
@@ -49,7 +50,10 @@ class UrlIndex : public HistoryBookmarkModel {
   std::unique_ptr<BookmarkNode> Remove(BookmarkNode* node,
                                        std::set<GURL>* removed_urls);
 
+  // Mutation of bookmark node fields that are exposed to HistoryBookmarkModel,
+  // which means must acquire a lock. Must be called from the UI thread.
   void SetUrl(BookmarkNode* node, const GURL& url);
+  void SetTitle(BookmarkNode* node, const base::string16& title);
 
   // Returns the nodes whose icon_url is |icon_url|.
   void GetNodesWithIconUrl(const GURL& icon_url,