OOPIF: report both original and destination rfh in AboutToNavigateRenderFrame.

OOPIF: report both original and destination rfh in AboutToNavigateRenderFrame.

While tracking navigations, devtools needs to know if the RFH reported in the event is
the one of interest (the one being inspected). I could go through the render frame manager,
but it sounds like unnecessary indirection.

BUG=451004
TBR=atwilson (sync tests)

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

[pfeldman, 2015-01-22 13:26:17]

Patchset #1 (id:1) has been deleted

[pfeldman, 2015-01-22 13:26:45]

pfeldman@chromium.org changed reviewers:
+ creis@chromium.org, dgozman@chromium.org, nasko@chromium.org

[dgozman, 2015-01-22 13:54:09]

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

https://codereview.chromium.org/868673002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:252: content::RenderFrameHost*
render_frame_host,
Let's use |old_host| and |new_host| for consistency.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:404:
DCHECK(!rfh->GetParent());
I think we can keep this until we support child frames.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:59: // Returns
RenderViewDevToolsAgentHost attached to any of RenderFrameHost
The comment is wrong. It's only for the main frame. Since behavior changed, did
you regress something?

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:112:
RenderViewDevToolsAgentHost* agent_host = FindAgentHost(current);
pending

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:269: // TODO(creis):
Consider removing this in favor of RenderFrameHostChanged.
Remove TODO? This (or similar) notification is not going anywhere.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:525: RenderFrameHostImpl*
from_host,
old/new?

[nasko, 2015-01-22 21:20:13]

My overall concern is that we are planning to remove AboutToNavigateRenderFrame
if possible. I am not opposed to proceed with this CL in the interest of making
progress, but will be good to have an idea why this API is needed and what are
the requirements of its replacement.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:269: // TODO(creis):
Consider removing this in favor of RenderFrameHostChanged.
On 2015/01/22 13:54:09, dgozman wrote:
> Remove TODO? This (or similar) notification is not going anywhere.

I do believe that this notification will go away, so keep the TODO. I'm
currently working on cleaning up/fixing up the WebContentsObserver interfaces
for RenderFrameHosts, so it will be nice to investigate what the requirements
you have are, so I can find a good replacement for this one.

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:292: //
AboutToNavigateRenderFrame was not called for renderer-initiated
Since you already handle this case, why is AboutToNavigateRenderFrame needed?

[dgozman, 2015-01-22 21:25:57]

On 2015/01/22 21:20:13, nasko wrote:
> My overall concern is that we are planning to remove
AboutToNavigateRenderFrame
> if possible. I am not opposed to proceed with this CL in the interest of
making
> progress, but will be good to have an idea why this API is needed and what are
> the requirements of its replacement.

I believe, we basically need 3 notifications:
- One about render frame host started navigation. This one should be as early as
possible (before any networking IPCs go through), so that we can enable agent in
the renderer and catch all the information during page load. We use
AboutToNavigateRenderFrame for this purpose.
- One about navigation commit, where we know that current RenderFrameHost has
been changed to the one started navigation in the previous notification. We use
RenderFrameHostChanged for this.
- One about navigation cancel, where the navigation was aborted, and therefore
"current" RenderFrameHost was not actually changed. We use our own static call
RenderViewDevToolsAgentHost::CancelPendingNavigation for this, and we miss some
canceled navigations (e.g. download, interstitial, maybe more). It would be nice
to have exactly one call for this.

Do you think I correctly understand possible navigation scenarios? If so, could
you propose the correct WCO notifications for this?

[pfeldman, 2015-01-22 21:37:43]

I now remember that we wanted to share a doc with scenarios with you. But since
we are discussing it here, let me elaborate on the AboutToNavigateRenderFrame as
the one that raises concerns.

Consider a simple pre-oopif cross-process navigation.
Lets say that devtools is inspecting renderer1 (represented by rvh1 on browser
side) that is soon to be replaced with renderer2 (represented by rvh2).
The only thing we need to do is initialize instrumentation in the renderer2
prior to navigating it. We need to do that after renderer is initiated, but
before it is navigated.

With transition to rfh, we need the same thing for every rfh that devtools is
attached to.

[nasko, 2015-01-22 21:53:07]

> - One about render frame host started navigation. This one should be as early
as
> possible (before any networking IPCs go through), so that we can enable agent
in
> the renderer and catch all the information during page load. We use
> AboutToNavigateRenderFrame for this purpose.

I think you might want to go for RenderFrameCreated. This is fired off right
after we send the IPC to the renderer to create a RenderFrame in the renderer
process or immediately after the browser process knows a RenderFrame has been
created in the renderer process.

> - One about navigation commit, where we know that current RenderFrameHost has
> been changed to the one started navigation in the previous notification. We
use
> RenderFrameHostChanged for this.

This is correct.

> - One about navigation cancel, where the navigation was aborted, and therefore
> "current" RenderFrameHost was not actually changed. We use our own static call
> RenderViewDevToolsAgentHost::CancelPendingNavigation for this, and we miss
some
> canceled navigations (e.g. download, interstitial, maybe more). It would be
nice
> to have exactly one call for this.

Doesn't DidFailProvisionalLoadForFrame accomplish this for you? It should. If it
doesn't, let's fix it.

On 2015/01/22 21:37:43, pfeldman wrote:
> I now remember that we wanted to share a doc with scenarios with you. But
since
> we are discussing it here, let me elaborate on the AboutToNavigateRenderFrame
as
> the one that raises concerns.
> 
> Consider a simple pre-oopif cross-process navigation.
> Lets say that devtools is inspecting renderer1 (represented by rvh1 on browser
> side) that is soon to be replaced with renderer2 (represented by rvh2).
> The only thing we need to do is initialize instrumentation in the renderer2
> prior to navigating it. We need to do that after renderer is initiated, but
> before it is navigated.

As I mentioned above, the event you should really be listening on is
RenderFrameCreated. I am in the process of ensuring that
RenderFrameCreated/RenderFrameDeleted work reliably and correspond to
RenderFrame object's lifetime in the renderer process.

> With transition to rfh, we need the same thing for every rfh that devtools is
> attached to.

There are 4 events from WCO which are of interest and I'm trying to streamline
right now:
- RenderFrameCreated/RenderFrameDeleted - these are tied to the objects in the
renderer process. They should balance and are fired off as early as possible.
These should be used to know when the renderer side is alive and can
successfully accept IPCs. When a process is killed or crashes, the Deleted event
will be triggered and if reload happens, the Create event will trigger again for
the same process/routing id pair.
- RenderFrameHostChanged/FrameDeleted - these are tied to the browser site
objects. RenderFrameHostChanged will fire when a new RFH will do navigation -
that is when it is newly created or a cross-site navigation commits. The
FrameDeleted will fire when the frame is removed from the page or the
WebContents is closed.

Granted, what I described above isn't working quite yet. It will be very useful
to know if this will satisfy your requirements. This should be irrespective of
whether there are oopifs or not.

Let's proceed with this CL for now and revisit this once I've landed all the
work to make the above work as intended. Does that sound good?

[pfeldman, 2015-01-22 21:58:58]

> Granted, what I described above isn't working quite yet. It will be very
useful
> to know if this will satisfy your requirements. This should be irrespective of
> whether there are oopifs or not.

I think the only missing bit is figuring out which existing RFH the one being
reported in RenderFrameCreated corresponds to. We need to pick the state from
the old_rfh and seed it in the new_rfh, so we need to correlate the two.

> 
> Let's proceed with this CL for now and revisit this once I've landed all the
> work to make the above work as intended. Does that sound good?

Yes, the use of the API is localized in this single class, we can follow up and
make it work with the new versions of the APIs as they arrive.

[dgozman, 2015-01-22 22:04:13]

On 2015/01/22 21:53:07, nasko wrote:
> > - One about render frame host started navigation. This one should be as
early
> as
> > possible (before any networking IPCs go through), so that we can enable
agent
> in
> > the renderer and catch all the information during page load. We use
> > AboutToNavigateRenderFrame for this purpose.
> 
> I think you might want to go for RenderFrameCreated. This is fired off right
> after we send the IPC to the renderer to create a RenderFrame in the renderer
> process or immediately after the browser process knows a RenderFrame has been
> created in the renderer process.

Note that we need to know the previous RenderFrameHost (if any), which will be
replaced by this freshly created host - in the case of cross-process navigation
in the frame.

> > - One about navigation cancel, where the navigation was aborted, and
therefore
> > "current" RenderFrameHost was not actually changed. We use our own static
call
> > RenderViewDevToolsAgentHost::CancelPendingNavigation for this, and we miss
> some
> > canceled navigations (e.g. download, interstitial, maybe more). It would be
> nice
> > to have exactly one call for this.
> 
> Doesn't DidFailProvisionalLoadForFrame accomplish this for you? It should. If
it
> doesn't, let's fix it.

So, this means that DidFailProvisionalLoadForFrame will happen exactly once for
the frame which was RenderFrameCreated, but was not changed to in
RenderFrameHostChanged?

> Granted, what I described above isn't working quite yet. It will be very
useful
> to know if this will satisfy your requirements. This should be irrespective of
> whether there are oopifs or not.
> 
> Let's proceed with this CL for now and revisit this once I've landed all the
> work to make the above work as intended. Does that sound good?
sgtm

[pfeldman, 2015-01-22 22:10:05]

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

https://codereview.chromium.org/868673002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:252: content::RenderFrameHost*
render_frame_host,
On 2015/01/22 13:54:08, dgozman wrote:
> Let's use |old_host| and |new_host| for consistency.

Done.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:404:
DCHECK(!rfh->GetParent());
On 2015/01/22 13:54:09, dgozman wrote:
> I think we can keep this until we support child frames.

Ack. But it is only called for GetMainFrame and I plan to follow up with the
experimental ability to inspect RFH shortly.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:59: // Returns
RenderViewDevToolsAgentHost attached to any of RenderFrameHost
On 2015/01/22 13:54:09, dgozman wrote:
> The comment is wrong. It's only for the main frame. Since behavior changed,
did
> you regress something?

Done.

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:112:
RenderViewDevToolsAgentHost* agent_host = FindAgentHost(current);
On 2015/01/22 13:54:09, dgozman wrote:
> pending

Done, but we'll have to carefully manage the state here as planned.

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:269: // TODO(creis):
Consider removing this in favor of RenderFrameHostChanged.
Leave the TODO for now, as agreed.

https://codereview.chromium.org/868673002/diff/20001/content/browser/devtools...
content/browser/devtools/render_view_devtools_agent_host.cc:292: //
AboutToNavigateRenderFrame was not called for renderer-initiated
On 2015/01/22 21:20:12, nasko wrote:
> Since you already handle this case, why is AboutToNavigateRenderFrame needed?

It is too late, we've already failed to seed the right state in the renderer.
This one is a fallback basically and I'd need to look at the bug it was fixing
to find out the call stack.

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

https://codereview.chromium.org/868673002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:525: RenderFrameHostImpl*
from_host,
On 2015/01/22 13:54:09, dgozman wrote:
> old/new?

Done.

[dgozman, 2015-01-22 22:12:46]

lgtm

[pfeldman, 2015-01-22 22:14:02]

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

[pfeldman, 2015-01-22 22:14:03]

+kalman@ for chrome/browser/extensions/ owner stamp
nasko@ for content/ stamp.

[nasko, 2015-01-22 22:20:50]

We really need to take a hard look at how tracking of navigation works, not only
because of out-of-process iframes, but mostly because of PlzNavigate, which
moves all navigation requests to be initiated in the browser process and the RFH
which the navigation will be associated with wouldn't be known until commit
time.

Since this is a far bigger issue to solve, let's go with what you have for now.
LGTM

[not at google - send to devlin, 2015-01-22 22:40:00]

lgtm

[pfeldman, 2015-01-23 12:30:53]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-01-23 12:38:20]

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

[commit-bot: I haz the power, 2015-01-23 12:44:05]

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

[commit-bot: I haz the power, 2015-01-23 12:44:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pfeldman, 2015-01-23 15:51:27]

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

[pfeldman, 2015-01-23 16:15:57]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-01-23 16:16:29]

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

[commit-bot: I haz the power, 2015-01-23 16:17:07]

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

[commit-bot: I haz the power, 2015-01-23 16:17:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pfeldman, 2015-01-23 16:29:42]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-01-23 16:30:30]

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

[commit-bot: I haz the power, 2015-01-23 16:58:50]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-01-23 17:00:01]

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