Fix RenderFrameHostChanged WebContentsObserver methods

Fix RenderFrameHostChanged WebContentsObserver methods

This CL makes the RenderFrameHostChanged observer method behave consistently and as described in the WebContentsObserver header file.

BUG=450799
Committed: https://crrev.com/f5940b9f08a6a6a43edb49c2805d0d518697e7c9
Cr-Commit-Position: refs/heads/master@{#318792}

[nasko, 2015-02-27 20:00:48]

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

[nasko, 2015-02-27 20:00:49]

Hey Charlie,
Can you review this CL for me?
Thanks in advance!
Nasko

[Charlie Reis, 2015-03-02 18:38:26]

Cool!  One question about ordering and a few minor comments.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:189:
frame_tree_->UnregisterRenderFrameHost(this);
Moving this makes me nervous about UaFs.  Are we sure that no observers below
(e.g., of RenderFrameDeleted) will try to access the RenderFrameHost or
RenderViewHost?

At the least, we should add a comment about why this new ordering is important
if the old way didn't work.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:113: // Note: do it only
for subframes, as the main frame case is taken care of
"Note: do it" -> "Do this"

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:244: // See
https://crbug.com/462682
nit: End with period.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:141: //
NotificationService. See https://crbug.com/462682
nit: End with period.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:144: RenderFrameHost*
new_host) = 0;
I feel like it's worth using RVH here instead of RFH.  That will make it more
obvious what it's used for and why it must go away.

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1273: // Ensure that observers
are notified of the creation of this WebContents'
nit: WebContents's

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1275: //
NotifySwappedFromRenderManager expects view_ to be created and it happens
nit: to already be
nit: and that happens

[nasko, 2015-03-02 19:30:54]

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:189:
frame_tree_->UnregisterRenderFrameHost(this);
On 2015/03/02 18:38:25, Charlie Reis wrote:
> Moving this makes me nervous about UaFs.  Are we sure that no observers below
> (e.g., of RenderFrameDeleted) will try to access the RenderFrameHost or
> RenderViewHost?

I don't know if observers will need to access RenderViewHost, though we are
weaning off of it, so I'd like to understand of there are such cases.
In terms of RenderFrameHost, observers won't see RenderFrameDeleted as part o
fthis call, it is right below it.

> At the least, we should add a comment about why this new ordering is important
> if the old way didn't work.

I think the main goal of this is to align with how OnRenderProcessGone works,
which does FTN scrub before firing off the RenderFrameDeleted observer.
If it makes you nervous, I can look into not making this change in this CL and
see if causes any issues.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:113: // Note: do it only
for subframes, as the main frame case is taken care of
On 2015/03/02 18:38:25, Charlie Reis wrote:
> "Note: do it" -> "Do this"

Done.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:244: // See
https://crbug.com/462682
On 2015/03/02 18:38:25, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:141: //
NotificationService. See https://crbug.com/462682
On 2015/03/02 18:38:26, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:144: RenderFrameHost*
new_host) = 0;
On 2015/03/02 18:38:26, Charlie Reis wrote:
> I feel like it's worth using RVH here instead of RFH.  That will make it more
> obvious what it's used for and why it must go away.

Done.

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1273: // Ensure that observers
are notified of the creation of this WebContents'
On 2015/03/02 18:38:26, Charlie Reis wrote:
> nit: WebContents's

Done.

https://codereview.chromium.org/950223006/diff/110009/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1275: //
NotifySwappedFromRenderManager expects view_ to be created and it happens
On 2015/03/02 18:38:26, Charlie Reis wrote:
> nit: to already be
> nit: and that happens

Done.

[Charlie Reis, 2015-03-02 22:44:31]

Thanks.  All looks good apart from the issue below.

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:189:
frame_tree_->UnregisterRenderFrameHost(this);
On 2015/03/02 19:30:53, nasko wrote:
> On 2015/03/02 18:38:25, Charlie Reis wrote:
> > Moving this makes me nervous about UaFs.  Are we sure that no observers
below
> > (e.g., of RenderFrameDeleted) will try to access the RenderFrameHost or
> > RenderViewHost?
> 
> I don't know if observers will need to access RenderViewHost, though we are
> weaning off of it, so I'd like to understand of there are such cases.
> In terms of RenderFrameHost, observers won't see RenderFrameDeleted as part o
> fthis call, it is right below it.

Yes, that's my concern, though.

I just looked more closely, and lots of WebContentsObservers use the
RenderFrameHost that's passed to RenderFrameDeleted.  (I see people calling its
GetParent, GetProcess, GetRoutingID, etc.)  It seems quite dangerous to delete
its RenderViewHost before we call RenderFrameDeleted.

 
> > At the least, we should add a comment about why this new ordering is
important
> > if the old way didn't work.
> 
> I think the main goal of this is to align with how OnRenderProcessGone works,
> which does FTN scrub before firing off the RenderFrameDeleted observer.
> If it makes you nervous, I can look into not making this change in this CL and
> see if causes any issues.

I'm not sure I understand the OnRenderProcessGone comparison, but I would feel
better about leaving UnregisterRenderFrameHost after RenderFrameDeleted.

[Charlie Reis, 2015-03-02 22:58:49]

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/950223006/diff/110009/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:189:
frame_tree_->UnregisterRenderFrameHost(this);
On 2015/03/02 22:44:30, Charlie Reis wrote:
> On 2015/03/02 19:30:53, nasko wrote:
> > On 2015/03/02 18:38:25, Charlie Reis wrote:
> > > Moving this makes me nervous about UaFs.  Are we sure that no observers
> below
> > > (e.g., of RenderFrameDeleted) will try to access the RenderFrameHost or
> > > RenderViewHost?
> > 
> > I don't know if observers will need to access RenderViewHost, though we are
> > weaning off of it, so I'd like to understand of there are such cases.
> > In terms of RenderFrameHost, observers won't see RenderFrameDeleted as part
o
> > fthis call, it is right below it.
> 
> Yes, that's my concern, though.
> 
> I just looked more closely, and lots of WebContentsObservers use the
> RenderFrameHost that's passed to RenderFrameDeleted.  (I see people calling
its
> GetParent, GetProcess, GetRoutingID, etc.)  It seems quite dangerous to delete
> its RenderViewHost before we call RenderFrameDeleted.
> 
>  
> > > At the least, we should add a comment about why this new ordering is
> important
> > > if the old way didn't work.
> > 
> > I think the main goal of this is to align with how OnRenderProcessGone
works,
> > which does FTN scrub before firing off the RenderFrameDeleted observer.
> > If it makes you nervous, I can look into not making this change in this CL
and
> > see if causes any issues.
> 
> I'm not sure I understand the OnRenderProcessGone comparison, but I would feel
> better about leaving UnregisterRenderFrameHost after RenderFrameDeleted.

Oops, this didn't come up in the patch diffs, so I didn't realize you'd already
reverted it.  LGTM!

[nasko, 2015-03-02 23:00:00]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-03-02 23:01:06]

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

[commit-bot: I haz the power, 2015-03-02 23:04:12]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-03-02 23:04:59]

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