Avoid creating a RenderWidgetHostView for proxies.

Avoid creating a RenderWidgetHostView for proxies.

This change simplifies the RenderViewHost initialization path so that it can avoid creating a RenderWidgetHostView on proxies and swappedout RenderViews.

BUG=522237, 526326
Committed: https://crrev.com/bede639284f3a6ffb058809f20b73de39ed13223
Cr-Commit-Position: refs/heads/master@{#348494}

[lfg, 2015-08-28 15:53:12]

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

[lfg, 2015-08-28 15:53:25]

Please take a look!

[Charlie Reis, 2015-08-28 17:40:22]

On 2015/08/28 15:53:25, lfg wrote:
> Please take a look!

I've always had a hard time following the for_main_frame_navigation logic, so
I'm very happy to see it go away.  I'm working my way through this CL, but can
you explain how it fixes the Retina bug?  I'm not yet seeing the connection.

[lfg, 2015-08-28 17:53:55]

On 2015/08/28 17:40:22, Charlie Reis wrote:
> On 2015/08/28 15:53:25, lfg wrote:
> > Please take a look!
> 
> I've always had a hard time following the for_main_frame_navigation logic, so
> I'm very happy to see it go away.  I'm working my way through this CL, but can
> you explain how it fixes the Retina bug?  I'm not yet seeing the connection.

This doesn't fix the Retina bug yet, I just wanted to cleanup this code as I was
looking through it.

[Charlie Reis, 2015-08-28 18:58:26]

+kenrb to CC, since I'm not entirely sure I understand this (especially in
CreateRenderWidgetHostViewForRenderManager).  A few questions below.  Perhaps
some additional comments would clear things up?

On 2015/08/28 17:53:55, lfg wrote:
> This doesn't fix the Retina bug yet, I just wanted to cleanup this code as I
was
> looking through it.

Ah, ok, thanks.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:84: CREATE_RF_HIDDEN = 1 <<
2,
I don't think these enum values are serialized or exposed anywhere, are they? 
Maybe we should shift them over.

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1712: if
(!render_view_host->GetView())
nit: This needs braces, since the body doesn't fit on one line.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1739: proxy_routing_id);
We're only calling this if proxy_routing_id is MSG_ROUTING_NONE, so maybe it
would be clearer to pass that as the parameter.  (I'm finding things hard to
follow, since it's tricky to spot a case that proxy_routing_id is valid.  I'm
guessing that there's some call to CreateRenderViewForRenderManager that passes
it?)

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:125: virtual bool
CreateRenderWidgetHostViewForRenderManager(
Please add a comment indicating that cases that proxy_routing_id will be passed
and what it's for.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:302:
GpuSurfaceTracker::Get()->SetSurfaceHandle(
Is this case ever reached?  I don't see a path where it could be.

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4348: int proxy_routing_id) {
I'm confused.  proxy_routing_id isn't used in this method, other than to tell
whether it's MSG_ROUTING_NONE.  Should it just be a bool indicating which case
we care about?

https://codereview.chromium.org/1306053008/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4357: } else if
(proxy_routing_id == MSG_ROUTING_NONE) {
If it's not MSG_ROUTING_NONE (and we're not in a guest), then we don't do
anything here?  Which case is that?

[lfg, 2015-08-28 21:18:40]

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:84: CREATE_RF_HIDDEN = 1 <<
2,
On 2015/08/28 18:58:26, Charlie Reis wrote:
> I don't think these enum values are serialized or exposed anywhere, are they? 
> Maybe we should shift them over.

Done.

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1712: if
(!render_view_host->GetView())
On 2015/08/28 18:58:26, Charlie Reis wrote:
> nit: This needs braces, since the body doesn't fit on one line.

Done.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1739: proxy_routing_id);
On 2015/08/28 18:58:26, Charlie Reis wrote:
> We're only calling this if proxy_routing_id is MSG_ROUTING_NONE, so maybe it
> would be clearer to pass that as the parameter.  (I'm finding things hard to
> follow, since it's tricky to spot a case that proxy_routing_id is valid.  I'm
> guessing that there's some call to CreateRenderViewForRenderManager that
passes
> it?)

No, I switched the interface to take a bool. What we are really interested is
whether the main frame is a proxy or not. When it's a proxy, we don't want to
create a View.

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:125: virtual bool
CreateRenderWidgetHostViewForRenderManager(
On 2015/08/28 18:58:26, Charlie Reis wrote:
> Please add a comment indicating that cases that proxy_routing_id will be
passed
> and what it's for.

Removed the proxy_routing_id.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:302:
GpuSurfaceTracker::Get()->SetSurfaceHandle(
On 2015/08/28 18:58:26, Charlie Reis wrote:
> Is this case ever reached?  I don't see a path where it could be.

The only place that can call this function with a proxy_routing_id !=
MSG_ROUTING_NONE is CreateRenderViewForRenderManager in WebContentsImpl.

Usually, the SurfaceHandles are set from the RenderWidgetHostView (in SetView).
Since this patch is allowing the creation of a RenderViewHost without a
RenderWidgetHostView, I had to change it to initialize the SurfaceHandle as a
null surface (as opposed to empty, which is initialized above).

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

https://codereview.chromium.org/1306053008/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4348: int proxy_routing_id) {
On 2015/08/28 18:58:26, Charlie Reis wrote:
> I'm confused.  proxy_routing_id isn't used in this method, other than to tell
> whether it's MSG_ROUTING_NONE.  Should it just be a bool indicating which case
> we care about?

Changed it to pass a bool.

https://codereview.chromium.org/1306053008/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4357: } else if
(proxy_routing_id == MSG_ROUTING_NONE) {
On 2015/08/28 18:58:26, Charlie Reis wrote:
> If it's not MSG_ROUTING_NONE (and we're not in a guest), then we don't do
> anything here?  Which case is that?

The guest case is an unfortunate exception, where the RenderViewHost contains a
proxy, but still needs a view since the main frame is still rendered.

We don't do anything here on two cases: 1. swappedout. 2. RenderViews in a
subframe process. In both cases, they aren't used for rendering, in the second
case the local root's widget will be used.

[Charlie Reis, 2015-08-31 19:02:51]

Thanks for explaining, but I'm still having a very hard time understanding how
this works.  I wonder if we can restructure it (and add comments) to make it
clearer.

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1737: false);
How do we know main_frame_is_proxy is false?  This could be a CreateRenderFrame
call for a subframe, when the main frame has a proxy.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1761: if
(proxy_routing_id == MSG_ROUTING_NONE && !render_view_host->GetView()) {
Maybe using |!swapped_out| would be clearer than |proxy_routing_id ==
MSG_ROUTING_NONE|?  I had to stare at this to see that we're only creating the
RWHV if the frame is active (and doesn't yet have a view).

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1763: false);
Again, this could be a subframe, so I'm not sure how we know that
main_frame_is_proxy is false.

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:125: virtual bool
CreateRenderWidgetHostViewForRenderManager(
I still think we really need comments here.  It's proving very difficult to
understand what this function does or what parameters it expects.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:302:
GpuSurfaceTracker::Get()->SetSurfaceHandle(
Please leave a comment so others will understand why this is needed.

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4348: bool
main_frame_is_proxy) {
Hmm, I'm not happy with this.  The name leads to some confusion at the call
sites (as I noted earlier), since it's always false for subframes even if the
main frame is a proxy.  Seems like we're trying to pass for_active_main_frame or
something like that?  That's the one case it will be true.

But on the flip side, the main frame proxy case seems like it makes this method
a no-op.  For a method named CreateRWHV, maybe it would be clearer if the
callers checked that in advance when deciding whether to call it?  Or does the
guest case make that too tricky?

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4365: return !!rwh_view;
No one seems to check the return value.  Seems like it should be a void method.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4377: render_view_host,
proxy_routing_id != MSG_ROUTING_NONE);
What if this were for a subframe?  We would be passing in true for a subframe
proxy_routing_id, not main_frame_is_proxy.

[lfg, 2015-09-02 00:32:17]

Please take another look!

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1737: false);
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> How do we know main_frame_is_proxy is false?  This could be a
CreateRenderFrame
> call for a subframe, when the main frame has a proxy.

You're right, we don't know that. What ends up happening is that it was creating
the RWHV even for main frames, when the subframes were getting initialized.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1761: if
(proxy_routing_id == MSG_ROUTING_NONE && !render_view_host->GetView()) {
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> Maybe using |!swapped_out| would be clearer than |proxy_routing_id ==
> MSG_ROUTING_NONE|?  I had to stare at this to see that we're only creating the
> RWHV if the frame is active (and doesn't yet have a view).

Done.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1763: false);
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> Again, this could be a subframe, so I'm not sure how we know that
> main_frame_is_proxy is false.

Same as above. I've added a IsMainFrame() check, which should fix this.

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:125: virtual bool
CreateRenderWidgetHostViewForRenderManager(
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> I still think we really need comments here.  It's proving very difficult to
> understand what this function does or what parameters it expects.

I removed the parameter and simplified the function.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:302:
GpuSurfaceTracker::Get()->SetSurfaceHandle(
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> Please leave a comment so others will understand why this is needed.

Done.

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

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4348: bool
main_frame_is_proxy) {
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> Hmm, I'm not happy with this.  The name leads to some confusion at the call
> sites (as I noted earlier), since it's always false for subframes even if the
> main frame is a proxy.  Seems like we're trying to pass for_active_main_frame
or
> something like that?  That's the one case it will be true.
> 
> But on the flip side, the main frame proxy case seems like it makes this
method
> a no-op.  For a method named CreateRWHV, maybe it would be clearer if the
> callers checked that in advance when deciding whether to call it?  Or does the
> guest case make that too tricky?

Done.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4365: return !!rwh_view;
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> No one seems to check the return value.  Seems like it should be a void
method.

Done.

https://codereview.chromium.org/1306053008/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4377: render_view_host,
proxy_routing_id != MSG_ROUTING_NONE);
On 2015/08/31 19:02:51, Charlie Reis (OOO till 9-8) wrote:
> What if this were for a subframe?  We would be passing in true for a subframe
> proxy_routing_id, not main_frame_is_proxy.

This can't be a subframe and have a proxy_routing_id. The subframes RenderView
get initialized in RenderFrameHostManager::CreateRenderFrameProxy (in
site-per-process mode).

Now, if this was a subframe, which could happen if the process crashed between
the initialization in CreateRenderFrameProxy and calling InitRenderView again.
For the purposes of this initialization it doesn't matter, because we don't get
the RenderProcessGone between the two calls to InitRenderView.

I agree this is a bit ugly and not ideal, but I'm not sure on how to improve
this for subframes. Ideally we shouldn't be calling InitRenderView() twice.

Another option is to pass IsMainFrame() into this function and always skip the
RWHV for subframes. WDYT?

[lfg, 2015-09-02 16:19:12]

On 2015/09/02 00:32:17, lfg wrote:
> This can't be a subframe and have a proxy_routing_id. The subframes RenderView
> get initialized in RenderFrameHostManager::CreateRenderFrameProxy (in
> site-per-process mode).
> 
> Now, if this was a subframe, which could happen if the process crashed between
> the initialization in CreateRenderFrameProxy and calling InitRenderView again.
> For the purposes of this initialization it doesn't matter, because we don't
get
> the RenderProcessGone between the two calls to InitRenderView.
> 
> I agree this is a bit ugly and not ideal, but I'm not sure on how to improve
> this for subframes. Ideally we shouldn't be calling InitRenderView() twice.
> 
> Another option is to pass IsMainFrame() into this function and always skip the
> RWHV for subframes. WDYT?

Actually, I thought about this yesterday, and I think we should only call
InitRenderView from CreateRenderFrame if we are creating the main frame. It
never makes sense to call InitRenderView with the wrong proxy_routing_id.

[Charlie Reis, 2015-09-02 20:13:08]

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

[Charlie Reis, 2015-09-02 20:13:08]

Thanks, this seems much clearer to me.  LGTM apart from one question below.

Also, I wonder if Nasko wants to give this a sanity check, since I think he
remembers a bit more of the context here (and I was chatting with him about it
yesterday).

https://codereview.chromium.org/1306053008/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1306053008/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1768: success =
InitRenderView(render_view_host, proxy_routing_id);
Sanity check: What happens if this is a subframe and the renderer process isn't
live (e.g., it crashed on the first subframe and this is the second)?  Before,
we probably passed the wrong IDs.  Now, I wonder if we'll end up with no
RenderView?

[lfg, 2015-09-02 21:56:37]

On 2015/09/02 20:13:08, Charlie Reis (OOO till 9-8) wrote:
>
https://codereview.chromium.org/1306053008/diff/100001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_manager.cc:1768: success =
> InitRenderView(render_view_host, proxy_routing_id);
> Sanity check: What happens if this is a subframe and the renderer process
isn't
> live (e.g., it crashed on the first subframe and this is the second)?  Before,
> we probably passed the wrong IDs.  Now, I wonder if we'll end up with no
> RenderView?

I don't think this can happen. I tried forcing a process crash in exact that
situation, what happens is that the IPC error is not processed until after all
the frames are initialized, and then we do a proper cleanup. (and when reloading
the next frame, it should recreate the entire frame tree).

Maybe for sanity we chould have a DCHECK that IsRenderViewLive is true when we
are not in the main frame case?

[Charlie Reis, 2015-09-02 22:01:39]

On 2015/09/02 21:56:37, lfg wrote:
> On 2015/09/02 20:13:08, Charlie Reis (OOO till 9-8) wrote:
> >
>
https://codereview.chromium.org/1306053008/diff/100001/content/browser/frame_...
> > content/browser/frame_host/render_frame_host_manager.cc:1768: success =
> > InitRenderView(render_view_host, proxy_routing_id);
> > Sanity check: What happens if this is a subframe and the renderer process
> isn't
> > live (e.g., it crashed on the first subframe and this is the second)? 
Before,
> > we probably passed the wrong IDs.  Now, I wonder if we'll end up with no
> > RenderView?
> 
> I don't think this can happen. I tried forcing a process crash in exact that
> situation, what happens is that the IPC error is not processed until after all
> the frames are initialized, and then we do a proper cleanup. (and when
reloading
> the next frame, it should recreate the entire frame tree).
> 
> Maybe for sanity we chould have a DCHECK that IsRenderViewLive is true when we
> are not in the main frame case?

Yes, a DCHECK sounds fine to me.

[lfg, 2015-09-02 22:49:08]

lfg@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[lfg, 2015-09-02 22:49:08]

+dmazzoni: Please review accessibility changes in render_frame_host_impl.cc

[nasko, 2015-09-02 22:54:55]

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1381:
->render_view_host_->GetView());
From Ken: I would suggest he use FTN->current_frame_host()->GetView() to get the
RWHV that is relevant for a given frame tree node.

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1777:
delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
It looks like we don't delete the RWHV once it is created. I assume this is
known and on purpose?

https://codereview.chromium.org/1306053008/diff/140001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:300: // If this is not an
active RenderView, then we need to create a
nit: s/RenderView/RenderViewHost/ here and below.

[lfg, 2015-09-02 23:17:11]

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1381:
->render_view_host_->GetView());
On 2015/09/02 22:54:54, nasko (slow to review) wrote:
> From Ken: I would suggest he use FTN->current_frame_host()->GetView() to get
the
> RWHV that is relevant for a given frame tree node.

I don't think this would work properly, because I think
RenderWidgetHostViewChildFrame doesn't implement CreateBrowserAccessibility
properly. That's why I added dmazzoni@ to review this.

One thing is that it seems that CreateBrowserAccessibility is only implemented
in the RWHV so it's easier to create platform-specific
BrowserAccessibilityManagers, it seems that there's no other dependencies on
RWHV. I'm wondering if it's worth it having it there in the first place.

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1777:
delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
On 2015/09/02 22:54:54, nasko (slow to review) wrote:
> It looks like we don't delete the RWHV once it is created. I assume this is
> known and on purpose?

It's not a major issue if we create a RWHV that's not being used to render, I
believe the only consequence is leaking memory.

That said, I haven't looked at the situation you described, which is when a
RenderViewHost is being used to render and it stops being used (I think this
could happen with opener relationships+navigations in other tabs). I'll take a
look at this tomorrow.

[nasko, 2015-09-02 23:36:06]

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1306053008/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1777:
delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
On 2015/09/02 23:17:11, lfg wrote:
> On 2015/09/02 22:54:54, nasko (slow to review) wrote:
> > It looks like we don't delete the RWHV once it is created. I assume this is
> > known and on purpose?
> 
> It's not a major issue if we create a RWHV that's not being used to render, I
> believe the only consequence is leaking memory.
> 
> That said, I haven't looked at the situation you described, which is when a
> RenderViewHost is being used to render and it stops being used (I think this
> could happen with opener relationships+navigations in other tabs). I'll take a
> look at this tomorrow.

Let's say we are on site A and have a subframe for B. This will cause RVH for B
to be created with NULL_TRANSPORT surface. Then we navigate the top-level page
to B. The RVH will get the RWHV created, but will this update the surface to be
a functioning one? In addition with openers I think it is quite possible for RVH
to do any state transition.

[lfg, 2015-09-03 18:04:42]

On 2015/09/02 23:36:06, nasko (slow to review) wrote:
> Let's say we are on site A and have a subframe for B. This will cause RVH for
B
> to be created with NULL_TRANSPORT surface. Then we navigate the top-level page
> to B. The RVH will get the RWHV created, but will this update the surface to
be
> a functioning one? In addition with openers I think it is quite possible for
RVH
> to do any state transition.

This is not an issue because when the RWHV gets created it'll update the surface
in RenderWidgetHostImpl::SetView().

[lfg, 2015-09-03 20:20:28]

On 2015/09/02 23:17:11, lfg wrote:
> On 2015/09/02 22:54:54, nasko (slow to review) wrote:
> > It looks like we don't delete the RWHV once it is created. I assume this is
> > known and on purpose?
> 
> It's not a major issue if we create a RWHV that's not being used to render, I
> believe the only consequence is leaking memory.
> 
> That said, I haven't looked at the situation you described, which is when a
> RenderViewHost is being used to render and it stops being used (I think this
> could happen with opener relationships+navigations in other tabs). I'll take a
> look at this tomorrow.

I tried this, the only way I found to leak the RWHV is quite complicated:

- Navigate to site A with iframe to site B. (this creates 2 RVHs, 1
RHWVPlatform, 1 RWHVChildFrame and 2 processes).
- Open new window to site A with iframe to site B. (this creates 2RVHs,
1RWHVPlatform, 1RWHVChildFrame and no new processes).
- Navigate new window to site B. (this creates 1 RWHVPlatform, destroys 1
RHWVChildFrame).

The different windows are needed to keep the RVH/processes alive while
navigating the main frame. That said, the RWHV is only leaked until the RVH is
destroyed.

[dmazzoni, 2015-09-11 18:13:22]

lgtm for accessibility changes in RFHI.

I think I should refactor it as a follow-up. Only the main frame's
BrowserAccessibilityManager needs information about the RWHV when it's
constructed. It'd be more clear if we used the same constructor/factory each
time, and pass the RWHV as well, which would be allowed to be null if this is
not the main frame.

But for now this change is fine, it shouldn't break anything.

[lfg, 2015-09-11 21:13:07]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-09-11 21:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1306053008/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-11 21:15:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306053008/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306053008/160001

[commit-bot: I haz the power, 2015-09-11 21:54:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-11 21:55:11]

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

[commit-bot: I haz the power, 2015-09-23 12:26:41]

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