Added the RenderView routing_id to the DidCommitProvisionalLoad message.

Added the RenderView routing_id to the DidCommitProvisionalLoad message.

This fixes an issue introduced by https://codereview.chromium.org/135723003/
where the navigation messages are no longer associated with the
RenderView and as a result the resource loader was not resetting state
for new navigations.

BUG=391741
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284985

[Pat Meenan, 2014-07-07 20:01:17]

nasko@chromium.org, creis@chromium.org: PTAL

When the navigation notification was moved from RenderView to RenderFrame (
https://codereview.chromium.org/135723003/ ) it broke some logic in the resource
loader where non-critical resources were delayed until after the body tag was
parsed.  It wasn't resetting state because it was depending on the routing_id of
the navigation message matching the resource loader messages.

Hopefully this isn't too ugly of a hack but I just added the RenderView
routing_id to the IPC params for the message so the resource loader could
continue to use the existing logic.

[nasko, 2014-07-08 07:17:31]

This seems like the wrong approach to take and will leave us in the same
situation of possible bugs in the future.

Since we are moving away from RenderViewHost and will only use RenderFrameHost
objects, it makes more sense to audit the ResourceScheduler and see where we
have RVH ids being used and replace those with RFH ids. If there are still IPCs
being routed through RVH that are impacting this, we should move them to RFH.

[Pat Meenan, 2014-07-08 12:20:48]

On 2014/07/08 07:17:31, nasko wrote:
> This seems like the wrong approach to take and will leave us in the same
> situation of possible bugs in the future.
> 
> Since we are moving away from RenderViewHost and will only use RenderFrameHost
> objects, it makes more sense to audit the ResourceScheduler and see where we
> have RVH ids being used and replace those with RFH ids. If there are still
IPCs
> being routed through RVH that are impacting this, we should move them to RFH.

I don't think it's going to be that simple.  The ViewHostMsg_WillInsertBody
would
probably be easy enough to move to RFH but the actual resource loading is also
tied to the routing ID of the view and as best as I can tell, the ID is plumbed
all the way back into WebKit.  Untangling the ResourceHostMsg_RequestResource,
ResourceHostMsg_SyncLoad, ResourceHostMsg_DidChangePriority and all of the
data flow for resource loading looks like it will be rather epic.

I could easily be wrong but it poking through code search it looks like the
routing_id for the resource loading IPC messages goes back to the WebKit bridge
here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It could be sheer luck that the requestorID() from the blink side matches the
RVH routing_id and maybe it was never supposed to work (or I followed the code
for the fetch path incorrectly).

[nasko, 2014-07-08 12:27:44]

On 2014/07/08 12:20:48, Pat Meenan wrote:
> On 2014/07/08 07:17:31, nasko wrote:
> > This seems like the wrong approach to take and will leave us in the same
> > situation of possible bugs in the future.
> > 
> > Since we are moving away from RenderViewHost and will only use
RenderFrameHost
> > objects, it makes more sense to audit the ResourceScheduler and see where we
> > have RVH ids being used and replace those with RFH ids. If there are still
> IPCs
> > being routed through RVH that are impacting this, we should move them to
RFH.
> 
> I don't think it's going to be that simple.  The ViewHostMsg_WillInsertBody
> would
> probably be easy enough to move to RFH but the actual resource loading is also
> tied to the routing ID of the view and as best as I can tell, the ID is
plumbed
> all the way back into WebKit.  Untangling the ResourceHostMsg_RequestResource,
> ResourceHostMsg_SyncLoad, ResourceHostMsg_DidChangePriority and all of the
> data flow for resource loading looks like it will be rather epic.
> 
> I could easily be wrong but it poking through code search it looks like the
> routing_id for the resource loading IPC messages goes back to the WebKit
bridge
> here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Isn't that controlled by content? Poking around codesearch a bit, it looks like
we manually set the RenderView routing id here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r....
If this changes to the routing id of the RenderFrame things might just start to
work. 

> It could be sheer luck that the requestorID() from the blink side matches the
> RVH routing_id and maybe it was never supposed to work (or I followed the code
> for the fetch path incorrectly).

Why would it be sheer luck? Blink is completely oblivious to routing ids, which
are controlled by content/. As long as we use the expected routing id when we
generate the request, it should just work. I could be missing something as well,
as I haven't dug around this area much.

[Pat Meenan, 2014-07-08 13:06:32]

On 2014/07/08 12:27:44, nasko wrote:
> On 2014/07/08 12:20:48, Pat Meenan wrote:
> > On 2014/07/08 07:17:31, nasko wrote:
> > > This seems like the wrong approach to take and will leave us in the same
> > > situation of possible bugs in the future.
> > > 
> > > Since we are moving away from RenderViewHost and will only use
> RenderFrameHost
> > > objects, it makes more sense to audit the ResourceScheduler and see where
we
> > > have RVH ids being used and replace those with RFH ids. If there are still
> > IPCs
> > > being routed through RVH that are impacting this, we should move them to
> RFH.
> > 
> > I don't think it's going to be that simple.  The ViewHostMsg_WillInsertBody
> > would
> > probably be easy enough to move to RFH but the actual resource loading is
also
> > tied to the routing ID of the view and as best as I can tell, the ID is
> plumbed
> > all the way back into WebKit.  Untangling the
ResourceHostMsg_RequestResource,
> > ResourceHostMsg_SyncLoad, ResourceHostMsg_DidChangePriority and all of the
> > data flow for resource loading looks like it will be rather epic.
> > 
> > I could easily be wrong but it poking through code search it looks like the
> > routing_id for the resource loading IPC messages goes back to the WebKit
> bridge
> > here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Isn't that controlled by content? Poking around codesearch a bit, it looks
like
> we manually set the RenderView routing id here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r....
> If this changes to the routing id of the RenderFrame things might just start
to
> work. 
> 
> > It could be sheer luck that the requestorID() from the blink side matches
the
> > RVH routing_id and maybe it was never supposed to work (or I followed the
code
> > for the fetch path incorrectly).
> 
> Why would it be sheer luck? Blink is completely oblivious to routing ids,
which
> are controlled by content/. As long as we use the expected routing id when we
> generate the request, it should just work. I could be missing something as
well,
> as I haven't dug around this area much.

Looks like that does take care of a bunch of the net events but as the comment
warns, the download manager breaks because it is expecting to be able to use the
routing ID to be able to get a RVH.  I'll see if I can track down where that
happens.  Not knowing what else might be looking for a RVH based on the
routing_id on the requests has me pretty nervous about breaking stuff though.

[Pat Meenan, 2014-07-08 14:22:48]

On 2014/07/08 13:06:32, Pat Meenan wrote:
> On 2014/07/08 12:27:44, nasko wrote:
> > On 2014/07/08 12:20:48, Pat Meenan wrote:
> > > On 2014/07/08 07:17:31, nasko wrote:
> > > > This seems like the wrong approach to take and will leave us in the same
> > > > situation of possible bugs in the future.
> > > > 
> > > > Since we are moving away from RenderViewHost and will only use
> > RenderFrameHost
> > > > objects, it makes more sense to audit the ResourceScheduler and see
where
> we
> > > > have RVH ids being used and replace those with RFH ids. If there are
still
> > > IPCs
> > > > being routed through RVH that are impacting this, we should move them to
> > RFH.
> > > 
> > > I don't think it's going to be that simple.  The
ViewHostMsg_WillInsertBody
> > > would
> > > probably be easy enough to move to RFH but the actual resource loading is
> also
> > > tied to the routing ID of the view and as best as I can tell, the ID is
> > plumbed
> > > all the way back into WebKit.  Untangling the
> ResourceHostMsg_RequestResource,
> > > ResourceHostMsg_SyncLoad, ResourceHostMsg_DidChangePriority and all of the
> > > data flow for resource loading looks like it will be rather epic.
> > > 
> > > I could easily be wrong but it poking through code search it looks like
the
> > > routing_id for the resource loading IPC messages goes back to the WebKit
> > bridge
> > > here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Isn't that controlled by content? Poking around codesearch a bit, it looks
> like
> > we manually set the RenderView routing id here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r....
> > If this changes to the routing id of the RenderFrame things might just start
> to
> > work. 
> > 
> > > It could be sheer luck that the requestorID() from the blink side matches
> the
> > > RVH routing_id and maybe it was never supposed to work (or I followed the
> code
> > > for the fetch path incorrectly).
> > 
> > Why would it be sheer luck? Blink is completely oblivious to routing ids,
> which
> > are controlled by content/. As long as we use the expected routing id when
we
> > generate the request, it should just work. I could be missing something as
> well,
> > as I haven't dug around this area much.
> 
> Looks like that does take care of a bunch of the net events but as the comment
> warns, the download manager breaks because it is expecting to be able to use
the
> routing ID to be able to get a RVH.  I'll see if I can track down where that
> happens.  Not knowing what else might be looking for a RVH based on the
> routing_id on the requests has me pretty nervous about breaking stuff though.

So far it looks like the download manager, file save manager, speech
recognition, media stream UI proxy and possibly web contents all expect to be
able to look up a RVH from the routing_id attached to the loader requests.  I
assume that unwinding all of that is going to be part of the effort to remove
RVH but it feels like an awful lot to bite off to fix the perf regression that
came from the first round of changes.

[nasko, 2014-07-08 14:40:29]

On 2014/07/08 14:22:48, Pat Meenan wrote:
> On 2014/07/08 13:06:32, Pat Meenan wrote:
> > On 2014/07/08 12:27:44, nasko wrote:
> > > On 2014/07/08 12:20:48, Pat Meenan wrote:
> > > > On 2014/07/08 07:17:31, nasko wrote:
> > > > > This seems like the wrong approach to take and will leave us in the
same
> > > > > situation of possible bugs in the future.
> > > > > 
> > > > > Since we are moving away from RenderViewHost and will only use
> > > RenderFrameHost
> > > > > objects, it makes more sense to audit the ResourceScheduler and see
> where
> > we
> > > > > have RVH ids being used and replace those with RFH ids. If there are
> still
> > > > IPCs
> > > > > being routed through RVH that are impacting this, we should move them
to
> > > RFH.
> > > > 
> > > > I don't think it's going to be that simple.  The
> ViewHostMsg_WillInsertBody
> > > > would
> > > > probably be easy enough to move to RFH but the actual resource loading
is
> > also
> > > > tied to the routing ID of the view and as best as I can tell, the ID is
> > > plumbed
> > > > all the way back into WebKit.  Untangling the
> > ResourceHostMsg_RequestResource,
> > > > ResourceHostMsg_SyncLoad, ResourceHostMsg_DidChangePriority and all of
the
> > > > data flow for resource loading looks like it will be rather epic.
> > > > 
> > > > I could easily be wrong but it poking through code search it looks like
> the
> > > > routing_id for the resource loading IPC messages goes back to the WebKit
> > > bridge
> > > > here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Isn't that controlled by content? Poking around codesearch a bit, it looks
> > like
> > > we manually set the RenderView routing id here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r....
> > > If this changes to the routing id of the RenderFrame things might just
start
> > to
> > > work. 
> > > 
> > > > It could be sheer luck that the requestorID() from the blink side
matches
> > the
> > > > RVH routing_id and maybe it was never supposed to work (or I followed
the
> > code
> > > > for the fetch path incorrectly).
> > > 
> > > Why would it be sheer luck? Blink is completely oblivious to routing ids,
> > which
> > > are controlled by content/. As long as we use the expected routing id when
> we
> > > generate the request, it should just work. I could be missing something as
> > well,
> > > as I haven't dug around this area much.
> > 
> > Looks like that does take care of a bunch of the net events but as the
comment
> > warns, the download manager breaks because it is expecting to be able to use
> the
> > routing ID to be able to get a RVH.  I'll see if I can track down where that
> > happens.  Not knowing what else might be looking for a RVH based on the
> > routing_id on the requests has me pretty nervous about breaking stuff
though.
> 
> So far it looks like the download manager, file save manager, speech
> recognition, media stream UI proxy and possibly web contents all expect to be
> able to look up a RVH from the routing_id attached to the loader requests.  I
> assume that unwinding all of that is going to be part of the effort to remove
> RVH but it feels like an awful lot to bite off to fix the perf regression that
> came from the first round of changes.

Please file a bug for migrating all the places you've found and include the
details. With that many components needing to move, I think it is fair to use
your proposed workaround, but do put comments around it indicating why we are
doing it and that it should be removed at some point (also including the bug
number).
Thanks for looking into this!

[Pat Meenan, 2014-07-08 15:22:00]

Bug filed: http://crbug.com/392171

I added comments around each of the places where the RVH routing_id was
referenced explaining the dependencies and linking to the bug.

[Pat Meenan, 2014-07-21 12:35:47]

brettw@chromium.org: I just noticed that cries was OOTO, would you mind taking a
look?

[nasko, 2014-07-21 13:11:24]

Noticed a possible comment error. Otherwise LGTM.

https://codereview.chromium.org/368953006/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/368953006/diff/20001/content/browser/loader/r...
content/browser/loader/resource_scheduler_filter.cc:51: // those dependencies
are unwound or moved to RenderViewHost we can move
Same as other comment.

https://codereview.chromium.org/368953006/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/368953006/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:159: // those dependencies are unwound or moved
to RenderViewHost we can move
Did you mean RenderFrameHost here and the line below?

https://codereview.chromium.org/368953006/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/368953006/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2960: // those dependencies are unwound or
moved to RenderViewHost we can move
Same as other comment.

[Pat Meenan, 2014-07-21 14:25:03]

Thanks, fixed the comment.  Just need OWNER LGTM (or feedback) from brettw on
content/browser/loader/resource_scheduler_filter.cc and
content/renderer/render_frame_impl.cc

[brettw, 2014-07-21 20:46:56]

Is there a bug open for unwinding this? I also feel like there should be at
least two references to this bug in your code, once in your OnNavigate call you
already have a comment in, and once in the other main place where it requires
render view IDs rather than frame IDs.

[brettw, 2014-07-21 20:47:42]

John is more involved with this site isolation stuff, I'd prefer he look at it.

[jam, 2014-07-22 01:06:18]

lgtm with comment

https://codereview.chromium.org/368953006/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/368953006/diff/40001/content/browser/loader/r...
content/browser/loader/resource_scheduler_filter.cc:49: // dependencies on being
able to look up the view based on the ID stored
"downstream dependencies" is vague. list exactly what needs it.

[Pat Meenan, 2014-07-22 19:09:33]

jam@, the dependencies that I could track down are in the bug that is referenced
at the end of the comment.  Should I make that clearer or do you want them
called out specifically in the comment?  There are probably more that I didn't
find which is kind of why I left the linkage loose.

nasko@, do you happen to have a bug that is tracking the overall migration away
from RenderViewHost that I can reference?  I'm not really involved in or
familiar with it, just trying to fix the regression in the resource scheduler
that was introduced.

[jam, 2014-07-22 20:05:01]

On 2014/07/22 19:09:33, Pat Meenan wrote:
> jam@, the dependencies that I could track down are in the bug that is
referenced
> at the end of the comment.  Should I make that clearer or do you want them
> called out specifically in the comment?  There are probably more that I didn't
> find which is kind of why I left the linkage loose.

It's good to list what classes are still needing this. That way when someone
looks at the comment, they can figure out if that render view id can be removed
if it's not used anymore.

> 
> nasko@, do you happen to have a bug that is tracking the overall migration
away
> from RenderViewHost that I can reference?  I'm not really involved in or
> familiar with it, just trying to fix the regression in the resource scheduler
> that was introduced.

[nasko, 2014-07-23 06:48:08]

On 2014/07/22 20:05:01, jam wrote:
> On 2014/07/22 19:09:33, Pat Meenan wrote:
> > jam@, the dependencies that I could track down are in the bug that is
> referenced
> > at the end of the comment.  Should I make that clearer or do you want them
> > called out specifically in the comment?  There are probably more that I
didn't
> > find which is kind of why I left the linkage loose.
> 
> It's good to list what classes are still needing this. That way when someone
> looks at the comment, they can figure out if that render view id can be
removed
> if it's not used anymore.
> 
> > 
> > nasko@, do you happen to have a bug that is tracking the overall migration
> away
> > from RenderViewHost that I can reference?  I'm not really involved in or
> > familiar with it, just trying to fix the regression in the resource
scheduler
> > that was introduced.

crbug.com/304341 tracks moving code from RVH to RFH.

[Pat Meenan, 2014-07-23 13:08:28]

The CQ bit was checked by pmeenan@chromium.org

[commit-bot: I haz the power, 2014-07-23 13:09:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmeenan@chromium.org/368953006/80001

[commit-bot: I haz the power, 2014-07-23 17:48:15]

Change committed as 284985