Better remove HistoryNodes

content/renderer/history_entry.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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 48 matching lines @@
     int64_t frame_id) {
   children_->push_back(new HistoryNode(entry_, item, frame_id));
   return children_->back();
 }
 
 HistoryEntry::HistoryNode* HistoryEntry::HistoryNode::AddChild() {
   return AddChild(WebHistoryItem(), kInvalidFrameRoutingID);
 }
 
 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,
     RenderFrameImpl* current_frame) {
   bool is_target_frame = target_frame == current_frame;
   const WebHistoryItem& item_for_create = is_target_frame ? new_item : item_;
   HistoryNode* new_history_node = new HistoryNode(
       new_entry, item_for_create, current_frame->GetRoutingID());
 
   if (is_target_frame && clone_children_of_target && !item_.isNull()) {
@@ skipping 20 matching lines @@
                                               new_item,
                                               clone_children_of_target,
                                               target_frame,
                                               child_render_frame);
       new_history_node->children_->push_back(new_child_node);
     }
   }
   return new_history_node;
 }
 
 void HistoryEntry::HistoryNode::set_item(const WebHistoryItem& item) {

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

Can this be called more than once per HistoryNode?  Why?

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

I would have confidently said "only once per node" before
https://codereview.chromium.org/1112823005, now I'm not sure.

   // 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.
     // If we've already mapped this frame id to a sequence number, standardize
     // this item to that sequence number. Otherwise, map the frame id to this
     // item's existing sequence number.
     if (GetFrameMap()[frame_id] == 0)
         GetFrameMap()[frame_id] = item_.frameSequenceNumber();
     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,
                                             bool clone_children_of_target,
                                             RenderFrameImpl* target_frame,
                                             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,
                              render_view->GetMainRenderFrame()));
   return new_entry;
 }
 
 HistoryEntry::HistoryNode* HistoryEntry::GetHistoryNodeForFrame(
     RenderFrameImpl* frame) {
   if (HistoryNode* history_node =
           frames_to_items_[GetFrameMap()[frame->GetRoutingID()]])
     return history_node;
   return unique_names_to_items_[frame->GetWebFrame()->uniqueName().utf8()];
 }
 
 WebHistoryItem HistoryEntry::GetItemForFrame(RenderFrameImpl* frame) {
   if (HistoryNode* history_node = GetHistoryNodeForFrame(frame))
     return history_node->item();
   return WebHistoryItem();
 }
 
 }  // namespace content