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..82dfb53a9e8ae70466bd59e6de37aae916bde7e0 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,14 @@ class CONTENT_EXPORT HistoryEntry {
  public:
   class HistoryNode {
    public:
-    HistoryNode(HistoryEntry* entry,
+    HistoryNode(const base::WeakPtr<HistoryEntry>& entry,
                 const blink::WebHistoryItem& item,
                 int64_t frame_id);
     ~HistoryNode();
 
     HistoryNode* AddChild(const blink::WebHistoryItem& item, int64_t frame_id);
     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,9 +75,11 @@ class CONTENT_EXPORT HistoryEntry {
     void RemoveChildren();
 
    private:
-    HistoryEntry* entry_;
+    base::WeakPtr<HistoryEntry> entry_;

[Nate Chapin, 2015/05/13 19:15:55]

When a HistoryEntry is destroyed, it takes all its HistoryNodes with it. Ensure
that HistoryNodes doing try to illegally access a dying HistoryEntry, or do
unnecessary work when the whole entry is being destroyed.

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

Ack.  Please make this a comment in the code itself.

Also, s/doing/don't/

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

Done.

     scoped_ptr<ScopedVector<HistoryNode> > children_;
     blink::WebHistoryItem item_;
+    std::vector<std::string> unique_names_;
+    std::vector<uint64_t> frame_ids_;

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

Why is there a list of these per HistoryNode?  HistoryNode is specific to one
frame.  (It's worth explaining in a comment.)

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

I had a DCHECK in set_item() to verifying that the new item's
frameSequenceNumber was the same as the old item's (if the old item is
non-null). It crashed a few tests, and I didn't immediately see why. I can dig
deeper if you think that's worthwhile.

[Charlie Reis, 2015/05/13 23:40:15]

That would help me immensely, since I'm blocked on that in the
FrameNavigationEntry work.  (I'd like to use frame sequence number instead of
FrameTreeNode ID to identify which FrameNavigationEntry to use, but I haven't
been able to understand why the frame sequence number changes.)

It also would let us put a comment here to help others understand it.

   };
 
   HistoryEntry(const blink::WebHistoryItem& root, int64_t frame_id);
@@ -94,7 +97,6 @@ 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;
@@ -102,6 +104,8 @@ class CONTENT_EXPORT HistoryEntry {
 
   typedef base::hash_map<std::string, HistoryNode*> UniqueNamesToItems;
   UniqueNamesToItems unique_names_to_items_;
+
+  base::WeakPtrFactory<HistoryEntry> weak_ptr_factory_;
 };
 
 }  // namespace content