Better remove HistoryNodes

content/renderer/history_entry.h

 // 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, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
  * Redistribution and use in source and binary forms, with or without
@@ skipping 20 matching lines @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef CONTENT_RENDERER_HISTORY_ENTRY_H_
 #define CONTENT_RENDERER_HISTORY_ENTRY_H_
 
 #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"
 
 namespace blink {
 class WebFrame;
 }
 
 namespace content {
 class RenderFrameImpl;
 class RenderViewImpl;
 
 const int kInvalidFrameRoutingID = -1;
 
 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,
                                  RenderFrameImpl* current_frame);
     blink::WebHistoryItem& item() { return item_; }
     void set_item(const blink::WebHistoryItem& item);
     std::vector<HistoryNode*>& children() const { return children_->get(); }
     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);
   HistoryEntry();
   ~HistoryEntry();
 
   HistoryEntry* CloneAndReplace(const blink::WebHistoryItem& newItem,
                                 bool clone_children_of_target,
                                 RenderFrameImpl* target_frame,
                                 RenderViewImpl* render_view);
 
   HistoryNode* GetHistoryNodeForFrame(RenderFrameImpl* frame);
   blink::WebHistoryItem GetItemForFrame(RenderFrameImpl* frame);
   const blink::WebHistoryItem& root() const { return root_->item(); }
   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
 
 #endif  // CONTENT_RENDERER_HISTORY_ENTRY_H_