Better remove HistoryNodes

content/renderer/history_entry.cc

Index: content/renderer/history_entry.cc
diff --git a/content/renderer/history_entry.cc b/content/renderer/history_entry.cc
index 7686165f4eea2c131e1cb51a800383429b6277f9..3289c91d2001a5e2fb027be9816ff8c96b282c5b 100644
--- a/content/renderer/history_entry.cc
+++ b/content/renderer/history_entry.cc
@@ -66,7 +66,7 @@ HistoryEntry::HistoryNode* HistoryEntry::HistoryNode::AddChild() {
 }
 
 HistoryEntry::HistoryNode* HistoryEntry::HistoryNode::CloneAndReplace(
-    HistoryEntry* new_entry,
+    const base::WeakPtr<HistoryEntry>& new_entry,
     const WebHistoryItem& new_item,
     bool clone_children_of_target,
     RenderFrameImpl* target_frame,
@@ -111,13 +111,17 @@ void HistoryEntry::HistoryNode::set_item(const WebHistoryItem& item) {
   // The previous HistoryItem might not have had a target set, or it might be
   // different than the current one.
   entry_->unique_names_to_items_[item.target().utf8()] = this;
+  unique_names_.push_back(item.target().utf8());
+
   entry_->frames_to_items_[item.frameSequenceNumber()] = this;
+  frame_ids_.push_back(item.frameSequenceNumber());
+
   item_ = item;
 }
 
-HistoryEntry::HistoryNode::HistoryNode(HistoryEntry* entry,
-                                       const WebHistoryItem& item,
-                                       int64_t frame_id)
+HistoryEntry::HistoryNode::HistoryNode(
+    const base::WeakPtr<HistoryEntry>& entry, const WebHistoryItem& item,
+    int64_t frame_id)
     : entry_(entry), item_(item) {
   if (frame_id != kInvalidFrameRoutingID) {
     // Each history item is given a frame sequence number on creation.
@@ -129,59 +133,52 @@ HistoryEntry::HistoryNode::HistoryNode(HistoryEntry* entry,
     else if (!item_.isNull())
         item_.setFrameSequenceNumber(GetFrameMap()[frame_id]);
     entry_->frames_to_items_[GetFrameMap()[frame_id]] = this;
+    frame_ids_.push_back(GetFrameMap()[frame_id]);
   }
 
-  if (!item_.isNull())
+  if (!item_.isNull()) {
     entry_->unique_names_to_items_[item_.target().utf8()] = this;
+    unique_names_.push_back(item.target().utf8());
+  }
   children_.reset(new ScopedVector<HistoryNode>);
 }
 
 HistoryEntry::HistoryNode::~HistoryNode() {
+  if (!entry_)
+    return;
+
+  for (uint64_t id : frame_ids_) {
+    HistoryEntry::FramesToItems::iterator it =
+        entry_->frames_to_items_.find(id);
+    if (it != entry_->frames_to_items_.end() && it->second == this)
+        entry_->frames_to_items_.erase(id);

[Charlie Reis, 2015/05/13 21:33:37]

nit: Wrong indent.  (git cl format can help.)

[Nate Chapin, 2015/05/13 23:23:21]

Done.

+  }
+
+  for (std::string name : unique_names_) {
+    HistoryEntry::UniqueNamesToItems::iterator it =
+        entry_->unique_names_to_items_.find(name);
+    if (it != entry_->unique_names_to_items_.end() && it->second == this)
+        entry_->unique_names_to_items_.erase(name);
+  }
 }
 
 void HistoryEntry::HistoryNode::RemoveChildren() {
-  // TODO(japhet): This is inefficient. Figure out a cleaner way to ensure
-  // this HistoryNode isn't cached anywhere.
-  std::vector<uint64_t> frames_to_remove;
-  std::vector<std::string> unique_names_to_remove;
-  for (size_t i = 0; i < children().size(); i++) {
-    children().at(i)->RemoveChildren();
-
-    HistoryEntry::FramesToItems::iterator frames_end =
-        entry_->frames_to_items_.end();
-    HistoryEntry::UniqueNamesToItems::iterator unique_names_end =
-        entry_->unique_names_to_items_.end();
-    for (HistoryEntry::FramesToItems::iterator it =
-             entry_->frames_to_items_.begin();
-         it != frames_end;
-         ++it) {
-      if (it->second == children().at(i))
-        frames_to_remove.push_back(GetFrameMap()[it->first]);
-    }
-    for (HistoryEntry::UniqueNamesToItems::iterator it =
-             entry_->unique_names_to_items_.begin();
-         it != unique_names_end;
-         ++it) {
-      if (it->second == children().at(i))
-        unique_names_to_remove.push_back(it->first);
-    }
-  }
-  for (unsigned i = 0; i < frames_to_remove.size(); i++)
-    entry_->frames_to_items_.erase(frames_to_remove[i]);
-  for (unsigned i = 0; i < unique_names_to_remove.size(); i++)
-    entry_->unique_names_to_items_.erase(unique_names_to_remove[i]);
   children_.reset(new ScopedVector<HistoryNode>);
 }
 
-HistoryEntry::HistoryEntry() {
-  root_.reset(new HistoryNode(this, WebHistoryItem(), kInvalidFrameRoutingID));
+HistoryEntry::HistoryEntry()
+    : weak_ptr_factory_(this) {
+  root_.reset(new HistoryNode(weak_ptr_factory_.GetWeakPtr(),
+                              WebHistoryItem(),
+                              kInvalidFrameRoutingID));
 }
 
 HistoryEntry::~HistoryEntry() {
 }
 
-HistoryEntry::HistoryEntry(const WebHistoryItem& root, int64_t frame_id) {
-  root_.reset(new HistoryNode(this, root, frame_id));
+HistoryEntry::HistoryEntry(const WebHistoryItem& root, int64_t frame_id)
+    : weak_ptr_factory_(this) {
+  root_.reset(new HistoryNode(weak_ptr_factory_.GetWeakPtr(), root, frame_id));
 }
 
 HistoryEntry* HistoryEntry::CloneAndReplace(const WebHistoryItem& new_item,
@@ -190,7 +187,7 @@ HistoryEntry* HistoryEntry::CloneAndReplace(const WebHistoryItem& new_item,
                                             RenderViewImpl* render_view) {
   HistoryEntry* new_entry = new HistoryEntry();
   new_entry->root_.reset(
-      root_->CloneAndReplace(new_entry,
+      root_->CloneAndReplace(new_entry->weak_ptr_factory_.GetWeakPtr(),
                              new_item,
                              clone_children_of_target,
                              target_frame,