Ignore DevTools messages from the old inspected RVH after navigation

Ignore DevTools messages from the old inspected RVH after navigation

DevToolsAgent and DevToolsClient now use main RenderFrame for
exchanging messages with their hosts. This way we always have source
RenderFrameHost when dispatching DevTools message on the browser side and can detect that the message was sent by old renderer after DevTools client already attached to the new renderer after navigation. Messages from old main renderer are ignored just like it had been before crrev.com/288297. This also allows to use RFH instead of RVH in many places on the browser side.

BUG=435025
Committed: https://crrev.com/65e008fd79d83908525452783452e69af8e7299c
Cr-Commit-Position: refs/heads/master@{#305619}

[yurys, 2014-11-20 13:30:05]

yurys@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2014-11-20 13:39:34]

lgtm

[yurys, 2014-11-20 13:47:09]

yurys@chromium.org changed reviewers:
+ jam@chromium.org

[yurys, 2014-11-20 13:47:09]

@jam: please review content/browser/web_contents/web_contents_impl.cc

[dgozman, 2014-11-20 18:33:12]

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

[dgozman, 2014-11-20 18:33:13]

Doesn't this sound like a WCO bug? Why is there an IPC from the old RVH after
RenderViewHostChanged call?

[jam, 2014-11-20 21:19:45]

given that RVH is going away, using it here would only be a short-term fix. Can
this be fixed in other ways?

[Charlie Reis, 2014-11-20 23:11:09]

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

[Charlie Reis, 2014-11-20 23:11:10]

On 2014/11/20 18:33:13, dgozman wrote:
> Doesn't this sound like a WCO bug? Why is there an IPC from the old RVH after
> RenderViewHostChanged call?

Unfortunately, I'm not sure we can avoid this.  RenderFrameHosts run their
unload handler in the background *after* the new RenderFrameHost has committed,
after an optimization from clamy@ earlier this year.  This means that the old
RFH/RVH will continue sending IPCs after Render{Frame,View}HostChanged.

It would be nice if there were a limited set of things that an unload handler
could do, allowing us to filter any other IPCs from the old RFH/RVH.  However, I
don't think that's an option.

On 2014/11/20 21:19:45, jam wrote:
> given that RVH is going away, using it here would only be a short-term fix.
Can
> this be fixed in other ways?

Maybe we can route the messages through the RFH, as suggested below?

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (left):

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:332: bool
RenderViewDevToolsAgentHost::OnMessageReceived(
Can all of your messages go through here instead?  Then you can check whether
render_frame_host->GetRenderViewHost() is the RVH you expect.

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

https://codereview.chromium.org/744653002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:491: if
(RenderViewDevToolsAgentHost::DispatchIPCMessage(
I don't think we should be adding this here; it doesn't scale to other message
handlers.

[yurys, 2014-11-21 08:14:14]

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (left):

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:332: bool
RenderViewDevToolsAgentHost::OnMessageReceived(
On 2014/11/20 23:11:10, Charlie Reis wrote:
> Can all of your messages go through here instead?  Then you can check whether
> render_frame_host->GetRenderViewHost() is the RVH you expect.

How can we force all messages go this path and not the one below? I see that all
of the messages that are actually dispatched (i.e. DispatchIPCMessage returns
true) go through the method below which doesn't accept render_frame_host

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

https://codereview.chromium.org/744653002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:491: if
(RenderViewDevToolsAgentHost::DispatchIPCMessage(
On 2014/11/20 23:11:10, Charlie Reis wrote:
> I don't think we should be adding this here; it doesn't scale to other message
> handlers.

This's just where it was before crrev.com/288297. I don't mind finding a better
place for it. WebContentsObserver::OnMessageReceived above can be called without
render_frame_host so it doesn't suit for it.

[Charlie Reis, 2014-11-22 00:38:17]

Thanks for clarifying-- a few notes below.

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (left):

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:332: bool
RenderViewDevToolsAgentHost::OnMessageReceived(
On 2014/11/21 08:14:14, yurys wrote:
> On 2014/11/20 23:11:10, Charlie Reis wrote:
> > Can all of your messages go through here instead?  Then you can check
whether
> > render_frame_host->GetRenderViewHost() is the RVH you expect.
> 
> How can we force all messages go this path and not the one below? I see that
all
> of the messages that are actually dispatched (i.e. DispatchIPCMessage returns
> true) go through the method below which doesn't accept render_frame_host

I think they have to be sent through RenderFrame{Host} rather than
RenderView{Host}.  See these lines in WebContentsImpl::OnMessageReceived:

  WebContentsObserver* observer;
  if (render_frame_host) {
    while ((observer = it.GetNext()) != NULL)
      if (observer->OnMessageReceived(message, render_frame_host))
        return true;
  } else {
    while ((observer = it.GetNext()) != NULL)
      if (observer->OnMessageReceived(message))
        return true;
  }

That includes any messages forwarded to WebContents from RFH rather than from
RVH.  It will need to include all messages eventually, as RVH goes away.

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

https://codereview.chromium.org/744653002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:491: if
(RenderViewDevToolsAgentHost::DispatchIPCMessage(
On 2014/11/21 08:14:14, yurys wrote:
> On 2014/11/20 23:11:10, Charlie Reis wrote:
> > I don't think we should be adding this here; it doesn't scale to other
message
> > handlers.
> 
> This's just where it was before crrev.com/288297. I don't mind finding a
better
> place for it. WebContentsObserver::OnMessageReceived above can be called
without
> render_frame_host so it doesn't suit for it.

Ah, I didn't realize it was here before.  I'm hesitant to add it back if we can
easily move the affected IPCs to RFH (necessary for OOPIF anyway).

(Maybe we can do this as a short term fix while those are moved, but let's avoid
that if we can.)

[yurys, 2014-11-24 13:19:30]

I changed DevToolsAgent and DevToolsClient to use main RenderFrame for
exchanging messages with their hosts. This way we always have source
RenderFrameHost when dispatching DevTools message on the browser side. Also
DevToolsFrontendHost is now passed WebContents instead of RVH when it is being
created. Please take a look.

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (left):

https://codereview.chromium.org/744653002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:332: bool
RenderViewDevToolsAgentHost::OnMessageReceived(
On 2014/11/22 00:38:17, Charlie Reis wrote:
> On 2014/11/21 08:14:14, yurys wrote:
> > On 2014/11/20 23:11:10, Charlie Reis wrote:
> > > Can all of your messages go through here instead?  Then you can check
> whether
> > > render_frame_host->GetRenderViewHost() is the RVH you expect.
> > 
> > How can we force all messages go this path and not the one below? I see that
> all
> > of the messages that are actually dispatched (i.e. DispatchIPCMessage
returns
> > true) go through the method below which doesn't accept render_frame_host
> 
> I think they have to be sent through RenderFrame{Host} rather than
> RenderView{Host}.  See these lines in WebContentsImpl::OnMessageReceived:
> 
>   WebContentsObserver* observer;
>   if (render_frame_host) {
>     while ((observer = it.GetNext()) != NULL)
>       if (observer->OnMessageReceived(message, render_frame_host))
>         return true;
>   } else {
>     while ((observer = it.GetNext()) != NULL)
>       if (observer->OnMessageReceived(message))
>         return true;
>   }
> 
> That includes any messages forwarded to WebContents from RFH rather than from
> RVH.  It will need to include all messages eventually, as RVH goes away.

Done.

[pfeldman, 2014-11-24 13:31:19]

lgtm w/ nits

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:340: if
(render_frame_host != render_view_host_->GetMainFrame()) {
Should we migrate this to render_frame_host_ ?

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:456: if
(IsAttached()) {
no need

https://codereview.chromium.org/744653002/diff/80001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.cc:158:
static_cast<RenderViewImpl*>(main_render_frame_->GetRenderView());
Introduce GetRenderView?

[dgozman, 2014-11-24 13:32:13]

https://codereview.chromium.org/744653002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/744653002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:285:
content::DevToolsFrontendHost::Create(devtools_bindings_->web_contents(),
If you create DevToolsFrontendHost for a WebContents instance, it should observe
render frame changes itself, and instantiate bindings in the main frame.
If you leave observing outside (as in this patch), perhaps DevToolsFrontendHost
should just enable bindings for a passed RenderFrameHost.

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
File content/browser/devtools/devtools_frontend_host_impl.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_frontend_host_impl.cc:10: #include
"content/public/browser/render_view_host.h"
nit: this is not used anymore.

[yurys, 2014-11-24 14:08:54]

https://codereview.chromium.org/744653002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/744653002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:285:
content::DevToolsFrontendHost::Create(devtools_bindings_->web_contents(),
On 2014/11/24 13:32:13, dgozman wrote:
> If you create DevToolsFrontendHost for a WebContents instance, it should
observe
> render frame changes itself, and instantiate bindings in the main frame.
> If you leave observing outside (as in this patch), perhaps
DevToolsFrontendHost
> should just enable bindings for a passed RenderFrameHost.

It will enable it for main frame only and it needs to handle IPC messages
addressed to the RFH so it needs to be a WebContentsObserver (RFH doesn't have
observers for handling messages) so I'd rather leave it as is.

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
File content/browser/devtools/devtools_frontend_host_impl.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_frontend_host_impl.cc:10: #include
"content/public/browser/render_view_host.h"
On 2014/11/24 13:32:13, dgozman wrote:
> nit: this is not used anymore.

Done.

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:340: if
(render_frame_host != render_view_host_->GetMainFrame()) {
On 2014/11/24 13:31:19, pfeldman wrote:
> Should we migrate this to render_frame_host_ ?

Let's do this in a separate change since there is some RVH specific logic in the
code.

https://codereview.chromium.org/744653002/diff/80001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:456: if
(IsAttached()) {
On 2014/11/24 13:31:19, pfeldman wrote:
> no need

Done.

https://codereview.chromium.org/744653002/diff/80001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/744653002/diff/80001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.cc:158:
static_cast<RenderViewImpl*>(main_render_frame_->GetRenderView());
On 2014/11/24 13:31:19, pfeldman wrote:
> Introduce GetRenderView?

Done.

[dgozman, 2014-11-24 14:26:08]

lgtm

[yurys, 2014-11-24 16:26:51]

@creis: please do OWNERS review, I've changed the code the way you suggested.

[Charlie Reis, 2014-11-24 17:46:27]

Great, thanks!  LGTM with one question below.

https://codereview.chromium.org/744653002/diff/120001/content/browser/devtool...
File content/browser/devtools/devtools_frontend_host_impl.cc (right):

https://codereview.chromium.org/744653002/diff/120001/content/browser/devtool...
content/browser/devtools/devtools_frontend_host_impl.cc:35: RenderFrameHost*
render_frame_host) {
Would it make sense to return early from this if render_frame_host !=
web_contents()->GetMainFrame()?

(Not sure if you're expecting any of these messages to be sent by subframes or
frames that have been swapped out.  If not, the early return would filter IPCs
down to those from the current main frame.)

[yurys, 2014-11-25 09:47:20]

https://codereview.chromium.org/744653002/diff/120001/content/browser/devtool...
File content/browser/devtools/devtools_frontend_host_impl.cc (right):

https://codereview.chromium.org/744653002/diff/120001/content/browser/devtool...
content/browser/devtools/devtools_frontend_host_impl.cc:35: RenderFrameHost*
render_frame_host) {
On 2014/11/24 17:46:26, Charlie Reis wrote:
> Would it make sense to return early from this if render_frame_host !=
> web_contents()->GetMainFrame()?
> 
Yes, that makes sense to me. Added an early return. 

> (Not sure if you're expecting any of these messages to be sent by subframes or
> frames that have been swapped out.  If not, the early return would filter IPCs
> down to those from the current main frame.)

All DevTools messages are sent by devtools_client.cc and routed through the main
frame only, so we don't expect messasges from anything other than the current
main frame.

[yurys, 2014-11-25 09:47:25]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-25 09:48:05]

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

[commit-bot: I haz the power, 2014-11-25 11:29:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-25 11:30:52]

Patchset 8 (id:??) landed as
https://crrev.com/65e008fd79d83908525452783452e69af8e7299c
Cr-Commit-Position: refs/heads/master@{#305619}