Better remove HistoryNodes

Better remove HistoryNodes

Clean up some technical debt related to removing children. Currently we do an expensive traversal to clear the unique name and frame id maps of stale references to children being removed. Stash some state on HistoryNodes to make it faster and more readable.

Committed: https://crrev.com/740b2ff5e2da9041c3d3e80cb612c82e27d15300
Cr-Commit-Position: refs/heads/master@{#332272}

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

japhet@chromium.org changed reviewers:
+ creis@chromium.org

[Nate Chapin, 2015-05-13 19:15:56]

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:78: base::WeakPtr<HistoryEntry> entry_;
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]

Ooh, fun bug.  Can you split the memory leak fix into a separate CL than the
cleanup?  I think the leak fix is a good candidate for merging, and the rest
feels riskier to me.

Mostly looks good.

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.cc:110: void
HistoryEntry::HistoryNode::set_item(const WebHistoryItem& item) {
Can this be called more than once per HistoryNode?  Why?

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.cc:154: entry_->frames_to_items_.erase(id);
nit: Wrong indent.  (git cl format can help.)

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:78: base::WeakPtr<HistoryEntry> entry_;
On 2015/05/13 19:15:55, Nate Chapin wrote:
> 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.

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

Also, s/doing/don't/

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
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]

This is issue is now for the cleanup, not the bug fix. Will upload a separate
patch shortly.

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.cc:110: void
HistoryEntry::HistoryNode::set_item(const WebHistoryItem& item) {
On 2015/05/13 21:33:37, Charlie Reis wrote:
> Can this be called more than once per HistoryNode?  Why?

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

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.cc:154: entry_->frames_to_items_.erase(id);
On 2015/05/13 21:33:37, Charlie Reis wrote:
> nit: Wrong indent.  (git cl format can help.)

Done.

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:78: base::WeakPtr<HistoryEntry> entry_;
On 2015/05/13 21:33:37, Charlie Reis wrote:
> On 2015/05/13 19:15:55, Nate Chapin wrote:
> > 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.
> 
> Ack.  Please make this a comment in the code itself.
> 
> Also, s/doing/don't/

Done.

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
On 2015/05/13 21:33:37, Charlie Reis wrote:
> Why is there a list of these per HistoryNode?  HistoryNode is specific to one
> frame.  (It's worth explaining in a comment.)

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]

Great, thanks.

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
On 2015/05/13 23:23:21, Nate Chapin wrote:
> On 2015/05/13 21:33:37, Charlie Reis wrote:
> > Why is there a list of these per HistoryNode?  HistoryNode is specific to
one
> > frame.  (It's worth explaining in a comment.)
> 
> 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.

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.

[Nate Chapin, 2015-05-15 19:10:47]

On 2015/05/13 23:40:15, Charlie Reis wrote:
> Great, thanks.
> 
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> File content/renderer/history_entry.h (right):
> 
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
> On 2015/05/13 23:23:21, Nate Chapin wrote:
> > On 2015/05/13 21:33:37, Charlie Reis wrote:
> > > Why is there a list of these per HistoryNode?  HistoryNode is specific to
> one
> > > frame.  (It's worth explaining in a comment.)
> > 
> > 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.
> 
> 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.

Ok. I'm reaching the conclusion that the frame sequence number stuff isn't worth
the trouble. Only one layout test fails with it removed, and that's fixed by
hard-coding the main frame -> root HistoryNode mapping. PTAL.

[Nate Chapin, 2015-05-15 23:39:58]

On 2015/05/15 19:10:47, Nate Chapin wrote:
> On 2015/05/13 23:40:15, Charlie Reis wrote:
> > Great, thanks.
> > 
> >
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> > File content/renderer/history_entry.h (right):
> > 
> >
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> > content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
> > On 2015/05/13 23:23:21, Nate Chapin wrote:
> > > On 2015/05/13 21:33:37, Charlie Reis wrote:
> > > > Why is there a list of these per HistoryNode?  HistoryNode is specific
to
> > one
> > > > frame.  (It's worth explaining in a comment.)
> > > 
> > > 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.
> > 
> > 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.
> 
> Ok. I'm reaching the conclusion that the frame sequence number stuff isn't
worth
> the trouble. Only one layout test fails with it removed, and that's fixed by
> hard-coding the main frame -> root HistoryNode mapping. PTAL.

FYI, the part of that I broke out (the memory leak fix) has spawned some
crashers: https://code.google.com/p/chromium/issues/detail?id=488470 and
https://code.google.com/p/chromium/issues/detail?id=488421.

I strongly suspect it just uncovered some inadequacies in
HistoryNode::RemoveChildren() that this patch will fix. I'm going to be OOO all
of next week, so please put this in the CQ if it's good to land as is.

[Charlie Reis, 2015-05-18 21:45:03]

On 2015/05/15 23:39:58, Nate Chapin wrote:
> On 2015/05/15 19:10:47, Nate Chapin wrote:
> > On 2015/05/13 23:40:15, Charlie Reis wrote:
> > > Great, thanks.
> > > 
> > >
> >
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> > > File content/renderer/history_entry.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1138543002/diff/60001/content/renderer/histor...
> > > content/renderer/history_entry.h:82: std::vector<uint64_t> frame_ids_;
> > > On 2015/05/13 23:23:21, Nate Chapin wrote:
> > > > On 2015/05/13 21:33:37, Charlie Reis wrote:
> > > > > Why is there a list of these per HistoryNode?  HistoryNode is specific
> to
> > > one
> > > > > frame.  (It's worth explaining in a comment.)
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > Ok. I'm reaching the conclusion that the frame sequence number stuff isn't
> worth
> > the trouble. Only one layout test fails with it removed, and that's fixed by
> > hard-coding the main frame -> root HistoryNode mapping. PTAL.
> 
> FYI, the part of that I broke out (the memory leak fix) has spawned some
> crashers: https://code.google.com/p/chromium/issues/detail?id=488470 and
> https://code.google.com/p/chromium/issues/detail?id=488421.
> 
> I strongly suspect it just uncovered some inadequacies in
> HistoryNode::RemoveChildren() that this patch will fix. I'm going to be OOO
all
> of next week, so please put this in the CQ if it's good to land as is.

I have lingering questions about this CL (e.g., how it affects serialized data
and how it resolves the crashes), so I'm not comfortable just landing it while
you're away.  I'll revert r329763 for now, and we can land both when you're
back.  I'll post more comments here after taking care of that one.

[Charlie Reis, 2015-05-18 22:48:01]

Ok, I've reverted the memory leak CL in https://crrev.com/330435.  We can reland
it once we understand the bigger picture of how this fits and whether it would
solve all the crashes.  (I'm still not sure how that CL is connected to this
one.)

For this one, I'd love to see FSNs be removed in favor of a more reliable
approach, since I'm trying to port this logic to the browser process right now. 
(For now, I'm using FrameTreeNode ID, which doesn't work if you navigate to a
new main frame and back, since the subframes get re-created with new FTN IDs.)

However, I think solely removing FSNs is likely to regress
https://crbug.com/393227, as noted below.  Is it possible to use something like
the order of the children if there's no name match?

I'm also concerned that we still have FSNs in the page_state_serialization.cc
logic (from https://codereview.chromium.org/412303002/).  Are there other uses
of them besides the ones being removed here?  If we're going to remove them, it
seems like we should clean them up entirely.

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
File content/renderer/history_entry.cc (left):

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
content/renderer/history_entry.cc:204:
frames_to_items_[GetFrameMap()[frame->GetRoutingID()]])
Won't removing FSNs cause a regression of https://crbug.com/393227 and
https://crbug.com/395097?  I'm sad we don't have tests for those.

This was introduced in https://codereview.chromium.org/412303002/ because we
couldn't find which frame to load a history item in after restoring a previous
session if the frame's name changed.  I don't see how we'll get that case right
now.

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
content/renderer/history_entry.h:84: std::vector<std::string> unique_names_;
Let's add a comment about why a single HistoryNode needs to track multiple
unique names.

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

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
File content/renderer/history_entry.cc (left):

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
content/renderer/history_entry.cc:204:
frames_to_items_[GetFrameMap()[frame->GetRoutingID()]])
On 2015/05/18 22:48:00, Charlie Reis wrote:
> Won't removing FSNs cause a regression of https://crbug.com/393227 and
> https://crbug.com/395097  I'm sad we don't have tests for those.
> 
> This was introduced in https://codereview.chromium.org/412303002/ because we
> couldn't find which frame to load a history item in after restoring a previous
> session if the frame's name changed.  I don't see how we'll get that case
right
> now.

All of the bugs I can find are a failure to correctly navigate the main frame,
hence the special case this CL adds. I tested them manually and they still
worked, FWIW.

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
File content/renderer/history_entry.h (right):

https://codereview.chromium.org/1138543002/diff/140001/content/renderer/histo...
content/renderer/history_entry.h:84: std::vector<std::string> unique_names_;
On 2015/05/18 22:48:00, Charlie Reis wrote:
> Let's add a comment about why a single HistoryNode needs to track multiple
> unique names.

Done.

[Charlie Reis, 2015-06-01 18:52:13]

Ok, so this handles main frames as a special case, and relies exclusively on the
unique name for finding which HistoryItem to use for a subframe, right?

I suppose that's sane, since there's only so much we can do if the page has
changed when we try to restore it.  (Either the names or the order of the frames
could have changed, and in some cases it's impossible to map the old
HistoryItems to the new frame tree.)

I definitely like the simpler approach and appreciate the cleanup.  I'm not sure
yet if I can safely use unique name in the browser process with OOPIFs, but it's
at least a direction for me to focus on.

Are there some existing tests that would catch regressions here?  If so, LGTM.

[Nate Chapin, 2015-06-01 18:57:01]

On 2015/06/01 18:52:13, Charlie Reis wrote:
> Ok, so this handles main frames as a special case, and relies exclusively on
the
> unique name for finding which HistoryItem to use for a subframe, right?
> 
> I suppose that's sane, since there's only so much we can do if the page has
> changed when we try to restore it.  (Either the names or the order of the
frames
> could have changed, and in some cases it's impossible to map the old
> HistoryItems to the new frame tree.)
> 
> I definitely like the simpler approach and appreciate the cleanup.  I'm not
sure
> yet if I can safely use unique name in the browser process with OOPIFs, but
it's
> at least a direction for me to focus on.
> 
> Are there some existing tests that would catch regressions here?  If so, LGTM.

The layout tests are not exactly complete, but they're a pretty solid set of
regression tests at this point.

[Charlie Reis, 2015-06-01 19:19:00]

On 2015/06/01 18:57:01, Nate Chapin wrote:
> On 2015/06/01 18:52:13, Charlie Reis wrote:
> > Ok, so this handles main frames as a special case, and relies exclusively on
> the
> > unique name for finding which HistoryItem to use for a subframe, right?
> > 
> > I suppose that's sane, since there's only so much we can do if the page has
> > changed when we try to restore it.  (Either the names or the order of the
> frames
> > could have changed, and in some cases it's impossible to map the old
> > HistoryItems to the new frame tree.)
> > 
> > I definitely like the simpler approach and appreciate the cleanup.  I'm not
> sure
> > yet if I can safely use unique name in the browser process with OOPIFs, but
> it's
> > at least a direction for me to focus on.
> > 
> > Are there some existing tests that would catch regressions here?  If so,
LGTM.
> 
> The layout tests are not exactly complete, but they're a pretty solid set of
> regression tests at this point.

Ok, but let's be sure to add explicit tests if we get regressions from this. 
LGTM.

[Nate Chapin, 2015-06-01 19:23:15]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-06-01 19:28:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138543002/180001

[commit-bot: I haz the power, 2015-06-02 00:46:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-02 00:46:58]

Patchset 10 (id:??) landed as
https://crrev.com/740b2ff5e2da9041c3d3e80cb612c82e27d15300
Cr-Commit-Position: refs/heads/master@{#332272}