Fix object ownership in ui/base/models.

components/bookmarks/browser/bookmark_model.cc

 // Copyright 2014 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 "components/bookmarks/browser/bookmark_model.h"
 
 #include <algorithm>
 #include <functional>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/string_util.h"
 #include "components/bookmarks/browser/bookmark_expanded_state_tracker.h"
 #include "components/bookmarks/browser/bookmark_index.h"
 #include "components/bookmarks/browser/bookmark_match.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_node_data.h"
 #include "components/bookmarks/browser/bookmark_storage.h"
 #include "components/bookmarks/browser/bookmark_undo_delegate.h"
@@ skipping 18 matching lines @@
 BookmarkPermanentNode* AsMutable(const BookmarkPermanentNode* node) {
   return const_cast<BookmarkPermanentNode*>(node);
 }
 
 // Comparator used when sorting permanent nodes. Nodes that are initially
 // visible are sorted before nodes that are initially hidden.
 class VisibilityComparator {
  public:
   explicit VisibilityComparator(BookmarkClient* client) : client_(client) {}
 
-  // Returns true if |n1| preceeds |n2|.
-  bool operator()(const BookmarkPermanentNode* n1,
-                  const BookmarkPermanentNode* n2) {
-    bool n1_visible = client_->IsPermanentNodeVisible(n1);
-    bool n2_visible = client_->IsPermanentNodeVisible(n2);
+  // Returns true if |n1| precedes |n2|.
+  bool operator()(const std::unique_ptr<BookmarkPermanentNode>& n1,
+                  const std::unique_ptr<BookmarkPermanentNode>& n2) {
+    bool n1_visible = client_->IsPermanentNodeVisible(n1.get());
+    bool n2_visible = client_->IsPermanentNodeVisible(n2.get());
     return n1_visible != n2_visible && n1_visible;
   }
 
  private:
   BookmarkClient* client_;
 };
 
 // Comparator used when sorting bookmarks. Folders are sorted first, then
 // bookmarks.
 class SortComparator {
  public:
   explicit SortComparator(icu::Collator* collator) : collator_(collator) {}
 
-  // Returns true if |n1| preceeds |n2|.
-  bool operator()(const BookmarkNode* n1, const BookmarkNode* n2) {
+  // Returns true if |n1| precedes |n2|.
+  bool operator()(const std::unique_ptr<BookmarkNode>& n1,
+                  const std::unique_ptr<BookmarkNode>& n2) {
     if (n1->type() == n2->type()) {
       // Types are the same, compare the names.
       if (!collator_)
         return n1->GetTitle() < n2->GetTitle();
       return base::i18n::CompareString16WithCollator(
                  *collator_, n1->GetTitle(), n2->GetTitle()) == UCOL_LESS;
     }
     // Types differ, sort such that folders come first.
     return n1->is_folder();
   }
@@ skipping 126 matching lines @@
 void BookmarkModel::Remove(const BookmarkNode* node) {
   DCHECK(loaded_);
   DCHECK(node);
   DCHECK(!is_root_node(node));
   RemoveAndDeleteNode(AsMutable(node));
 }
 
 void BookmarkModel::RemoveAllUserBookmarks() {
   std::set<GURL> removed_urls;
   struct RemoveNodeData {
-    RemoveNodeData(const BookmarkNode* parent, int index, BookmarkNode* node)
-        : parent(parent), index(index), node(node) {}
-
     const BookmarkNode* parent;
     int index;
-    BookmarkNode* node;
+    std::unique_ptr<BookmarkNode> node;
   };
   std::vector<RemoveNodeData> removed_node_data_list;
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillRemoveAllUserBookmarks(this));
 
   BeginExtensiveChanges();
   // Skip deleting permanent nodes. Permanent bookmark nodes are the root and
   // its immediate children. For removing all non permanent nodes just remove
   // all children of non-root permanent nodes.
   {
     base::AutoLock url_lock(url_lock_);
     for (int i = 0; i < root_.child_count(); ++i) {
       const BookmarkNode* permanent_node = root_.GetChild(i);
 
       if (!client_->CanBeEditedByUser(permanent_node))
         continue;
 
       for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
-        BookmarkNode* child_node = AsMutable(permanent_node->GetChild(j));
-        RemoveNodeAndGetRemovedUrls(child_node, &removed_urls);
-        removed_node_data_list.push_back(
-            RemoveNodeData(permanent_node, j, child_node));
+        std::unique_ptr<BookmarkNode> node = RemoveNodeAndGetRemovedUrls(
+            AsMutable(permanent_node->GetChild(j)), &removed_urls);
+        removed_node_data_list.push_back({permanent_node, j, std::move(node)});
       }
     }
   }
   EndExtensiveChanges();
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkAllUserNodesRemoved(this, removed_urls));
 
   BeginGroupedChanges();
-  for (const auto& removed_node_data : removed_node_data_list) {
-    undo_delegate()->OnBookmarkNodeRemoved(
-        this, removed_node_data.parent, removed_node_data.index,
-        std::unique_ptr<BookmarkNode>(removed_node_data.node));
+  for (auto& removed_node_data : removed_node_data_list) {
+    undo_delegate()->OnBookmarkNodeRemoved(this, removed_node_data.parent,
+                                           removed_node_data.index,
+                                           std::move(removed_node_data.node));
   }
   EndGroupedChanges();
 }
 
 void BookmarkModel::Move(const BookmarkNode* node,
                          const BookmarkNode* new_parent,
                          int index) {
   if (!loaded_ || !node || !IsValidIndex(new_parent, index, true) ||
       is_root_node(new_parent) || is_permanent_node(node)) {
     NOTREACHED();
@@ skipping 12 matching lines @@
   if (old_parent == new_parent &&
       (index == old_index || index == old_index + 1)) {
     // Node is already in this position, nothing to do.
     return;
   }
 
   SetDateFolderModified(new_parent, Time::Now());
 
   if (old_parent == new_parent && index > old_index)
     index--;
+
+  BookmarkNode* mutable_old_parent = AsMutable(old_parent);
+  std::unique_ptr<BookmarkNode> owned_node =
+      mutable_old_parent->Remove(AsMutable(node));
   BookmarkNode* mutable_new_parent = AsMutable(new_parent);
-  mutable_new_parent->Add(AsMutable(node), index);
+  mutable_new_parent->Add(std::move(owned_node), index);
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeMoved(this, old_parent, old_index,
                                       new_parent, index));
 }
 
 void BookmarkModel::Copy(const BookmarkNode* node,
@@ skipping 279 matching lines @@
     const BookmarkNode* parent,
     int index,
     const base::string16& title,
     const BookmarkNode::MetaInfoMap* meta_info) {
   if (!loaded_ || is_root_node(parent) || !IsValidIndex(parent, index, true)) {
     // Can't add to the root.
     NOTREACHED();
     return NULL;
   }
 
-  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), GURL());
+  std::unique_ptr<BookmarkNode> new_node =
+      base::MakeUnique<BookmarkNode>(generate_next_node_id(), GURL());
   new_node->set_date_folder_modified(Time::Now());
   // Folders shouldn't have line breaks in their titles.
   new_node->SetTitle(title);
   new_node->set_type(BookmarkNode::FOLDER);
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  return AddNode(AsMutable(parent), index, new_node);
+  return AddNode(AsMutable(parent), index, std::move(new_node));
 }
 
 const BookmarkNode* BookmarkModel::AddURL(const BookmarkNode* parent,
                                           int index,
                                           const base::string16& title,
                                           const GURL& url) {
   return AddURLWithCreationTimeAndMetaInfo(
       parent,
       index,
       title,
@@ skipping 12 matching lines @@
   if (!loaded_ || !url.is_valid() || is_root_node(parent) ||
       !IsValidIndex(parent, index, true)) {
     NOTREACHED();
     return NULL;
   }
 
   // Syncing may result in dates newer than the last modified date.
   if (creation_time > parent->date_folder_modified())
     SetDateFolderModified(parent, creation_time);
 
-  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), url);
+  std::unique_ptr<BookmarkNode> new_node =
+      base::MakeUnique<BookmarkNode>(generate_next_node_id(), url);
   new_node->SetTitle(title);
   new_node->set_date_added(creation_time);
   new_node->set_type(BookmarkNode::URL);
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  return AddNode(AsMutable(parent), index, new_node);
+  return AddNode(AsMutable(parent), index, std::move(new_node));
 }
 
 void BookmarkModel::SortChildren(const BookmarkNode* parent) {
   DCHECK(client_->CanBeEditedByUser(parent));
 
   if (!parent || !parent->is_folder() || is_root_node(parent) ||
       parent->child_count() <= 1) {
     return;
   }
 
@@ skipping 16 matching lines @@
                     BookmarkNodeChildrenReordered(this, parent));
 }
 
 void BookmarkModel::ReorderChildren(
     const BookmarkNode* parent,
     const std::vector<const BookmarkNode*>& ordered_nodes) {
   DCHECK(client_->CanBeEditedByUser(parent));
 
   // Ensure that all children in |parent| are in |ordered_nodes|.
   DCHECK_EQ(static_cast<size_t>(parent->child_count()), ordered_nodes.size());
-  for (size_t i = 0; i < ordered_nodes.size(); ++i)
-    DCHECK_EQ(parent, ordered_nodes[i]->parent());
+  for (const BookmarkNode* node : ordered_nodes)
+    DCHECK_EQ(parent, node->parent());
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillReorderBookmarkNode(this, parent));
 
-  AsMutable(parent)->SetChildren(
-      *(reinterpret_cast<const std::vector<BookmarkNode*>*>(&ordered_nodes)));
+  BookmarkNode* mutable_parent = AsMutable(parent);
+  for (size_t i = 0; i < ordered_nodes.size() - 1; ++i) {

[sky, 2016/09/29 22:06:48]

if ordered_nodes is empty this code is problematic.

[Avi (use Gerrit), 2016/09/29 22:59:58]

Done.

+    std::unique_ptr<BookmarkNode> node =
+        mutable_parent->Remove(AsMutable(ordered_nodes[i]));

[sky, 2016/09/29 22:06:48]

This implementation is way more expensive than the old.

[Avi (use Gerrit), 2016/09/29 22:59:58]

Yes. The only thought I have on how to do better is to pass the vector into the
TreeNode and have it rearrange the children. WDYT?

[Avi (use Gerrit), 2016/09/30 00:16:48]

Actually I have an idea.

+    mutable_parent->Add(std::move(node), i);
+  }
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeChildrenReordered(this, parent));
 }
 
 void BookmarkModel::SetDateFolderModified(const BookmarkNode* parent,
                                           const Time time) {
@@ skipping 45 matching lines @@
     case BookmarkNode::OTHER_NODE:
       return other_node_;
     case BookmarkNode::MOBILE:
       return mobile_node_;
     default:
       NOTREACHED();
       return NULL;
   }
 }
 
-void BookmarkModel::RestoreRemovedNode(
-    const BookmarkNode* parent,
-    int index,
-    std::unique_ptr<BookmarkNode> scoped_node) {
-  BookmarkNode* node = scoped_node.release();
-  AddNode(AsMutable(parent), index, node);
+void BookmarkModel::RestoreRemovedNode(const BookmarkNode* parent,
+                                       int index,
+                                       std::unique_ptr<BookmarkNode> node) {
+  BookmarkNode* node_ptr = node.get();
+  AddNode(AsMutable(parent), index, std::move(node));
 
   // We might be restoring a folder node that have already contained a set of
   // child nodes. We need to notify all of them.
-  NotifyNodeAddedForAllDescendents(node);
+  NotifyNodeAddedForAllDescendents(node_ptr);
 }
 
 void BookmarkModel::NotifyNodeAddedForAllDescendents(const BookmarkNode* node) {
   for (int i = 0; i < node->child_count(); ++i) {
     FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                       BookmarkNodeAdded(this, node, i));
     NotifyNodeAddedForAllDescendents(node->GetChild(i));
   }
 }
 
@@ skipping 44 matching lines @@
     tracked_objects::ScopedTracker tracking_profile2(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("467179 BookmarkModel::DoneLoading2"));
 
     // If bookmarks file changed externally, the IDs may have changed
     // externally. In that case, the decoder may have reassigned IDs to make
     // them unique. So when the file has changed externally, we should save the
     // bookmarks file to persist new IDs.
     if (store_.get())
       store_->ScheduleSave();
   }
-  bookmark_bar_node_ = details->release_bb_node();
-  other_node_ = details->release_other_folder_node();
-  mobile_node_ = details->release_mobile_folder_node();
-  index_.reset(details->release_index());
+  std::unique_ptr<BookmarkPermanentNode> owned_bb_node =
+      details->owned_bb_node();
+  std::unique_ptr<BookmarkPermanentNode> owned_other_folder_node =
+      details->owned_other_folder_node();
+  std::unique_ptr<BookmarkPermanentNode> owned_mobile_folder_node =
+      details->owned_mobile_folder_node();
+  index_ = details->owned_index();
+
+  bookmark_bar_node_ = owned_bb_node.get();

[sky, 2016/09/29 22:06:48]

Can you set these members when you add them below?

[Avi (use Gerrit), 2016/09/29 22:59:58]

Not sure what you mean. Have something like

  root_children.push_back(std::move(owned_bb_node));
  bookmark_bar_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_other_folder_node));
  other_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_mobile_folder_node));
  mobile_node_ = root_children.back().get();

? That doesn't read as much cleaner to me, but if you prefer...

[sky, 2016/09/30 03:07:05]

My mistake. What you have is fine.

+  other_node_ = owned_other_folder_node.get();
+  mobile_node_ = owned_mobile_folder_node.get();
 
   // Get any extra nodes and take ownership of them at the |root_|.
-  std::vector<BookmarkPermanentNode*> extra_nodes;
-  details->release_extra_nodes(&extra_nodes);
+  std::vector<std::unique_ptr<BookmarkPermanentNode>> extra_nodes =
+      details->owned_extra_nodes();
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile3(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("467179 BookmarkModel::DoneLoading3"));
 
   // WARNING: order is important here, various places assume the order is
   // constant (but can vary between embedders with the initial visibility
   // of permanent nodes).
-  std::vector<BookmarkPermanentNode*> root_children;
-  root_children.push_back(bookmark_bar_node_);
-  root_children.push_back(other_node_);
-  root_children.push_back(mobile_node_);
-  for (size_t i = 0; i < extra_nodes.size(); ++i)
-    root_children.push_back(extra_nodes[i]);
+  std::vector<std::unique_ptr<BookmarkPermanentNode>> root_children;
+  root_children.push_back(std::move(owned_bb_node));
+  root_children.push_back(std::move(owned_other_folder_node));
+  root_children.push_back(std::move(owned_mobile_folder_node));
+  std::move(extra_nodes.begin(), extra_nodes.end(),
+            std::back_inserter(root_children));
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile4(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("467179 BookmarkModel::DoneLoading4"));
 
   std::stable_sort(root_children.begin(),
                    root_children.end(),
                    VisibilityComparator(client_.get()));
   for (size_t i = 0; i < root_children.size(); ++i)
-    root_.Add(root_children[i], static_cast<int>(i));
+    root_.Add(std::move(root_children[i]), static_cast<int>(i));
 
   root_.SetMetaInfoMap(details->model_meta_info_map());
   root_.set_sync_transaction_version(details->model_sync_transaction_version());
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile5(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("467179 BookmarkModel::DoneLoading5"));
 
   {
     base::AutoLock url_lock(url_lock_);
     // Update nodes_ordered_by_url_set_ from the nodes.
     PopulateNodesByURL(&root_);
   }
 
   loaded_ = true;
 
   loaded_signal_.Signal();
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/467179
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile6(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("467179 BookmarkModel::DoneLoading6"));
 
   // Notify our direct observers.
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkModelLoaded(this, details->ids_reassigned()));
 }
 
-void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
-  std::unique_ptr<BookmarkNode> node(delete_me);
+void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* node_ptr) {
+  std::unique_ptr<BookmarkNode> node;
 
-  const BookmarkNode* parent = node->parent();
+  const BookmarkNode* parent = node_ptr->parent();
   DCHECK(parent);
-  int index = parent->GetIndexOf(node.get());
+  int index = parent->GetIndexOf(node_ptr);
   DCHECK_NE(-1, index);
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
-                    OnWillRemoveBookmarks(this, parent, index, node.get()));
+                    OnWillRemoveBookmarks(this, parent, index, node_ptr));
 
   std::set<GURL> removed_urls;
   {
     base::AutoLock url_lock(url_lock_);
-    RemoveNodeAndGetRemovedUrls(node.get(), &removed_urls);
+    node = RemoveNodeAndGetRemovedUrls(node_ptr, &removed_urls);
   }
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(
       BookmarkModelObserver,
       observers_,
       BookmarkNodeRemoved(this, parent, index, node.get(), removed_urls));
 
   undo_delegate()->OnBookmarkNodeRemoved(this, parent, index, std::move(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.
   url_lock_.AssertAcquired();
   NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(node);
   DCHECK(i != nodes_ordered_by_url_set_.end());
   // i points to the first node with the URL, advance until we find the
   // node we're removing.
   while (*i != node)
     ++i;
   nodes_ordered_by_url_set_.erase(i);
 }
 
-void BookmarkModel::RemoveNodeAndGetRemovedUrls(BookmarkNode* node,
-                                                std::set<GURL>* removed_urls) {
+std::unique_ptr<BookmarkNode> BookmarkModel::RemoveNodeAndGetRemovedUrls(
+    BookmarkNode* node_ptr,
+    std::set<GURL>* removed_urls) {
   // NOTE: this method should be always called with |url_lock_| held.
   // This method does not explicitly acquires a lock.
   url_lock_.AssertAcquired();
   DCHECK(removed_urls);
-  BookmarkNode* parent = node->parent();
+  BookmarkNode* parent = node_ptr->parent();
   DCHECK(parent);
-  parent->Remove(node);
-  RemoveNode(node, removed_urls);
+  std::unique_ptr<BookmarkNode> node = parent->Remove(node_ptr);
+  RemoveNode(node_ptr, removed_urls);
   // RemoveNode adds an entry to removed_urls for each node of type URL. As we
   // allow duplicates we need to remove any entries that are still bookmarked.
   for (std::set<GURL>::iterator i = removed_urls->begin();
        i != removed_urls->end();) {
     if (IsBookmarkedNoLock(*i)) {
       // When we erase the iterator pointing at the erasee is
       // invalidated, so using i++ here within the "erase" call is
       // important as it advances the iterator before passing the
       // old value through to erase.
       removed_urls->erase(i++);
     } else {
       ++i;
     }
   }
+
+  return node;
 }
 
 BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
                                      int index,
-                                     BookmarkNode* node) {
-  parent->Add(node, index);
+                                     std::unique_ptr<BookmarkNode> node) {
+  BookmarkNode* node_ptr = node.get();
+  parent->Add(std::move(node), index);
 
   if (store_.get())
     store_->ScheduleSave();
 
   {
     base::AutoLock url_lock(url_lock_);
-    AddNodeToInternalMaps(node);
+    AddNodeToInternalMaps(node_ptr);
   }
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  return node;
+  return node_ptr;
 }
 
 void BookmarkModel::AddNodeToInternalMaps(BookmarkNode* node) {
   url_lock_.AssertAcquired();
   if (node->is_url()) {
     index_->Add(node);
     nodes_ordered_by_url_set_.insert(node);
   }
   for (int i = 0; i < node->child_count(); ++i)
     AddNodeToInternalMaps(node->GetChild(i));
@@ skipping 118 matching lines @@
   undo_delegate_ = undo_delegate;
   if (undo_delegate_)
     undo_delegate_->SetUndoProvider(this);
 }
 
 BookmarkUndoDelegate* BookmarkModel::undo_delegate() const {
   return undo_delegate_ ? undo_delegate_ : empty_undo_delegate_.get();
 }
 
 }  // namespace bookmarks