Cache RenderFrameHost's parent so it doesn't change over time.

Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea
Cr-Commit-Position: refs/heads/master@{#401198}

[Charlie Reis, 2016-06-21 22:22:30]

Description was changed from

==========
Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
==========

to

==========
Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-06-21 22:30:18]

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

[Charlie Reis, 2016-06-21 22:30:19]

Alex, can you take a look?  This is another small piece to land before the
pending delete FTN CL.  Shouldn't be any different today, but it will matter
when RFHs can live on after their parent FTN swaps away.

[Charlie Reis, 2016-06-21 23:10:56]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 23:12:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2086083003/1

[alexmos, 2016-06-21 23:36:03]

LGTM with one quick question.

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:263: parent_ =
frame_tree_node_->parent()->current_frame_host();
Could this ever be incorrect if we try to create a new child frame in an unload
handler?  Or is that disallowed?  (I realize this will match our old behavior in
that case, just curious what should happen.)

[dcheng, 2016-06-21 23:39:47]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-06-21 23:39:48]

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:263: parent_ =
frame_tree_node_->parent()->current_frame_host();
On 2016/06/21 23:36:02, alexmos wrote:
> Could this ever be incorrect if we try to create a new child frame in an
unload
> handler?  Or is that disallowed?  (I realize this will match our old behavior
in
> that case, just curious what should happen.)

We don't allow frames to be attached in frames that are detaching. Might not be
a bad idea to have some enforcement in the browser though.

[Charlie Reis, 2016-06-21 23:47:23]

Thanks!

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2086083003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:263: parent_ =
frame_tree_node_->parent()->current_frame_host();
On 2016/06/21 23:39:47, dcheng wrote:
> On 2016/06/21 23:36:02, alexmos wrote:
> > Could this ever be incorrect if we try to create a new child frame in an
> unload
> > handler?  Or is that disallowed?  (I realize this will match our old
behavior
> in
> > that case, just curious what should happen.)
> 
> We don't allow frames to be attached in frames that are detaching. Might not
be
> a bad idea to have some enforcement in the browser though.

Yeah, I think that's something we'll want to enforce in the pending delete FTN
CL, in case there's a race.  For now, it should be safe.

[Charlie Reis, 2016-06-21 23:49:28]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-06-21 23:49:35]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-06-21 23:51:02]

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

[commit-bot: I haz the power, 2016-06-22 01:38:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 01:38:21]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Reis, 2016-06-22 02:34:20]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-06-22 02:34:57]

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

[commit-bot: I haz the power, 2016-06-22 03:42:05]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-22 03:45:03]

Description was changed from

==========
Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea
Cr-Commit-Position: refs/heads/master@{#401198}
==========

[commit-bot: I haz the power, 2016-06-22 03:45:04]

Patchset 1 (id:??) landed as
https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea
Cr-Commit-Position: refs/heads/master@{#401198}