Make sure to update the BookmarkIndex on a URL Change

components/bookmarks/browser/bookmark_model.cc

Index: components/bookmarks/browser/bookmark_model.cc
diff --git a/components/bookmarks/browser/bookmark_model.cc b/components/bookmarks/browser/bookmark_model.cc
index 60e143caf4f38ccb184bca21ef82b37750874da4..3ec8670701343756a52c3ae39c99169861234607 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -371,9 +371,9 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) {
 
   {
     base::AutoLock url_lock(url_lock_);
-    RemoveNodeFromURLSet(mutable_node);
+    RemoveNodeFromInternalMaps(mutable_node);
     mutable_node->set_url(url);
-    nodes_ordered_by_url_set_.insert(mutable_node);
+    AddNodeToInternalMaps(mutable_node);
   }
 
   if (store_.get())
@@ -617,12 +617,6 @@ const BookmarkNode* BookmarkModel::AddURLWithCreationTimeAndMetaInfo(
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  {
-    // Only hold the lock for the duration of the insert.
-    base::AutoLock url_lock(url_lock_);
-    nodes_ordered_by_url_set_.insert(new_node);
-  }
-
   return AddNode(AsMutable(parent), index, new_node);
 }
 
@@ -740,9 +734,8 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
 
   url_lock_.AssertAcquired();
   if (node->is_url()) {
-    RemoveNodeFromURLSet(node);
+    RemoveNodeFromInternalMaps(node);
     removed_urls->insert(node->url());
-    index_->Remove(node);
   }
 
   CancelPendingFaviconLoadRequests(node);
@@ -837,7 +830,8 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
       BookmarkNodeRemoved(this, parent, index, node.get(), removed_urls));
 }
 
-void BookmarkModel::RemoveNodeFromURLSet(BookmarkNode* node) {
+void BookmarkModel::RemoveNodeFromInternalMaps(BookmarkNode* node) {
+  index_->Remove(node);
   // NOTE: this is called in such a way that url_lock_ is already held. As
   // such, this doesn't explicitly grab the lock.
   NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(node);
@@ -886,11 +880,21 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  index_->Add(node);
+  if (node->type() == BookmarkNode::URL) {

[danduong, 2014/11/04 01:48:06]

Is this right? It seems to match the previous logic closest.

[sky, 2014/11/04 03:41:45]

I don't think there is a good reason why this is after notifying observers. I
think you should move your code to 879ish. And yes, this looks right.

[danduong, 2014/11/04 03:52:48]

Done.

+    base::AutoLock url_lock(url_lock_);
+    AddNodeToInternalMaps(node);

[danduong, 2014/11/04 01:48:05]

Moving this here actually makes you hold on to url_lock_ slightly longer for the
index_->Add call. Is that OK?

[sky, 2014/11/04 03:41:45]

I don't think it'll make much of a difference.

+  } else {
+    index_->Add(node);
+  }
 
   return node;
 }
 
+void BookmarkModel::AddNodeToInternalMaps(BookmarkNode* node) {
+  index_->Add(node);
+  nodes_ordered_by_url_set_.insert(node);
+}
+
 bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,
                                  int index,
                                  bool allow_end) {