Better remove HistoryNodes

content/renderer/history_entry.h

Index: content/renderer/history_entry.h
diff --git a/content/renderer/history_entry.h b/content/renderer/history_entry.h
index 62da86a4692b8c244aba3696cf4dfbf3b4e5f01d..c599f9964c73f6206c8e9282427543a0fc373bc8 100644
--- a/content/renderer/history_entry.h
+++ b/content/renderer/history_entry.h
@@ -38,6 +38,7 @@
 #include "base/containers/hash_tables.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
 #include "content/common/content_export.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/web/WebHistoryItem.h"
@@ -56,14 +57,13 @@ class CONTENT_EXPORT HistoryEntry {
  public:
   class HistoryNode {
    public:
-    HistoryNode(HistoryEntry* entry,
-                const blink::WebHistoryItem& item,
-                int64_t frame_id);
+    HistoryNode(const base::WeakPtr<HistoryEntry>& entry,
+                const blink::WebHistoryItem& item);
     ~HistoryNode();
 
-    HistoryNode* AddChild(const blink::WebHistoryItem& item, int64_t frame_id);
+    HistoryNode* AddChild(const blink::WebHistoryItem& item);
     HistoryNode* AddChild();
-    HistoryNode* CloneAndReplace(HistoryEntry* new_entry,
+    HistoryNode* CloneAndReplace(const base::WeakPtr<HistoryEntry>& new_entry,
                                  const blink::WebHistoryItem& new_item,
                                  bool clone_children_of_target,
                                  RenderFrameImpl* target_frame,
@@ -74,12 +74,17 @@ class CONTENT_EXPORT HistoryEntry {
     void RemoveChildren();
 
    private:
-    HistoryEntry* entry_;
+    // When a HistoryEntry is destroyed, it takes all its HistoryNodes with it.
+    // Use a WeakPtr to ensure that HistoryNodes don't try to illegally access
+    // a dying HistoryEntry, or do unnecessary work when the whole entry is
+    // being destroyed.
+    base::WeakPtr<HistoryEntry> entry_;
     scoped_ptr<ScopedVector<HistoryNode> > children_;
     blink::WebHistoryItem item_;
+    std::vector<std::string> unique_names_;

[Charlie Reis, 2015/05/18 22:48:00]

Let's add a comment about why a single HistoryNode needs to track multiple
unique names.

[Nate Chapin, 2015/05/28 21:31:50]

Done.

   };
 
-  HistoryEntry(const blink::WebHistoryItem& root, int64_t frame_id);
+  HistoryEntry(const blink::WebHistoryItem& root);
   HistoryEntry();
   ~HistoryEntry();
 
@@ -94,14 +99,12 @@ class CONTENT_EXPORT HistoryEntry {
   HistoryNode* root_history_node() const { return root_.get(); }
 
  private:
-
   scoped_ptr<HistoryNode> root_;
 
-  typedef base::hash_map<uint64_t, HistoryNode*> FramesToItems;
-  FramesToItems frames_to_items_;
-
   typedef base::hash_map<std::string, HistoryNode*> UniqueNamesToItems;
   UniqueNamesToItems unique_names_to_items_;
+
+  base::WeakPtrFactory<HistoryEntry> weak_ptr_factory_;
 };
 
 }  // namespace content