PlzNavigate: provide the FrameTreeNode ID to the network stack

PlzNavigate: provide the FrameTreeNode ID to the network stack

This CL adds the FrameTreeNode ID to the
NavigationRequestInfo that is passed to the ResourceDispatcherHost. This allows
a proper DownloadTabInfo to be created, and downloads to start with
browser-side navigation enabled.

BUG=475027
Committed: https://crrev.com/a16aa8170b607d329f45d623fa30825ad94285eb
Cr-Commit-Position: refs/heads/master@{#331351}

[clamy, 2015-04-22 13:14:32]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, davidben@chromium.org, fdegans@chromium.org,
nasko@chromium.org

[clamy, 2015-04-22 13:19:13]

@nasko, fdegans: PTAL
@carlosk, fdegans: FYI

This is a fix to have DownloadContentTests pass with PlzNavigate enabled. The
problem here is that this code:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
fails with PlzNavigate, since we don't provide it with a routing and child id it
can use to get the BrowserContext. An alternative way to fix the problem would
be to bring back the FTN lookup by general ID and add the FTN ID to the request.
Then have the DownloadRequestHandle use it when PlzNavigate is enabled to get
the BrowserContext/WebContents. Wdyt?

[nasko, 2015-04-22 14:16:20]

On 2015/04/22 13:19:13, clamy wrote:
> @nasko, fdegans: PTAL
> @carlosk, fdegans: FYI
> 
> This is a fix to have DownloadContentTests pass with PlzNavigate enabled. The
> problem here is that this code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
> fails with PlzNavigate, since we don't provide it with a routing and child id
it
> can use to get the BrowserContext. An alternative way to fix the problem would
> be to bring back the FTN lookup by general ID and add the FTN ID to the
request.
> Then have the DownloadRequestHandle use it when PlzNavigate is enabled to get
> the BrowserContext/WebContents. Wdyt?

Let's avoid using RenderViewHost. Since we are standardizing for navigation to
work with FrameTreeNode, I think it makes sense to use the FTN id only.

[davidben, 2015-04-22 15:16:11]

On 2015/04/22 14:16:20, nasko (very slow until 04-27) wrote:
> On 2015/04/22 13:19:13, clamy wrote:
> > @nasko, fdegans: PTAL
> > @carlosk, fdegans: FYI
> > 
> > This is a fix to have DownloadContentTests pass with PlzNavigate enabled.
The
> > problem here is that this code:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
> > fails with PlzNavigate, since we don't provide it with a routing and child
id
> it
> > can use to get the BrowserContext. An alternative way to fix the problem
would
> > be to bring back the FTN lookup by general ID and add the FTN ID to the
> request.
> > Then have the DownloadRequestHandle use it when PlzNavigate is enabled to
get
> > the BrowserContext/WebContents. Wdyt?
> 
> Let's avoid using RenderViewHost. Since we are standardizing for navigation to
> work with FrameTreeNode, I think it makes sense to use the FTN id only.

I haven't had any time for this, and it's unclear how much PlzNavigate should
block here (if there's always a routing ID available, maybe that's a sufficient
hack? I would have thought a freshly created WebContents would be a problem),
but I was hoping to detach the ResourceLoader stack from RDH and
child/route/request IDs altogether. The downloads code isn't the only bit that
uses routing IDs. There's also HTTP auth dialogs, WebView, some extensions code,
and possibly other craziness.

I was envisioning an end state of something like:

1. Only subresource loads initiated by renderers are owned by the RDH. All
others are owned by the browser-side components that initiate their requests. So
NavigationURLLoaderImplCore owns the navigation ResourceLoader and eventually
NavigationURLLoader becomes just the UI thread interface to ResourceLoader.

2. Downloads dispatch is removed from BufferedResourceHandler which only does
MIME sniffing. Instead the sniffed MIME type is popped back to the UI thread and
the navigation commit logic directs the body stream to the downloads subsystem
for a download. Since we're on the UI thread, we can easily associate with
WebContents and everything else.

3. For everything else that needs to pop from ResourceLoader to WebContents, the
main challenge is that there's a lot of code which acts on both renderer-owned
subresource requests and browser-owned navigation requests. It'll be too
error-prone if they all need to handle both cases. Stapling information to
browser-owned requests is actually rather easy because they all start on the UI
thread. It's the renderer-owned requests that are trouble because they start at
the renderer, and we don't trust the renderer. Right now it works only because
everything is scoped to child_id. I can see the following options:

a) Add a thread hop to every RequestResource IPC. We process those on the UI
thread, requests are, at the end of the day, owned by some UI thread object,
etc. All callbacks which may need the WebContents pop to the UI thread early
which then processes things. Very complex logic like the ResourceScheduler moves
to UI. This is conceptually very clean, doesn't involve adding duplicate
information on UI and IO, but would be yet another massive rearchitecting and
might have too high of a performance cost? Not sure.

b) Add to ResourceRequestInfo a GetWebContentsCallback() which returns a
callback which can give you a WebContents but only after you pop it to the UI
thread. Advantage is that we don't need to track anything extra for
renderer-owned requests. Disadvantage is we still need to add some kind of map
on FTN ID or WebContents ID or something for the browser-owned ones. It also
doesn't work for extensions and WebView which need to bucket on the IO thread,
so we'd still need to rearchitect those.

c) Add a higher-level set of IDs on the IO thread and ditch the existing ones.
The most immediate one would probably be a tab_id to identify a WebContents.
WebView and extensions effectively need this anyway. FTN id may also be
reasonable, though we then need to expose FTN out of content. The advantage is
that we don't need to move logic from the IO thread to the UI thread.
Disadvantage is we still have all this IO thread code with partial information.
We also need a way to staple the correct tab IDs to renderer-owned requests,
which means we need to maintain an IO-thread-side map from (child, frame) ->
(high-level IDs).

c is probably the easiest to get to, but it does require the most machinery that
we may want to go back and flatten out later. 1, 2, and 3 should be more-or-less
orthogonal. Though they all depend on clearing through everything which depends
on the child ID, request ID, etc, on ResourceRequestInfo one way or another.

[clamy, 2015-04-22 15:33:59]

I see. In the meantime, I have a new version of the patch where we pass down the
FTN id to te RDH. This seems to work with half of the download tests (the ones
about resume still fail).

[nasko, 2015-04-27 14:24:12]

Thanks David for the thorough write up. I tend to agree with most of it, but I
don't think we can block PlzNavigate on that work. Unless you feel strongly
otherwise, I'd suggest we proceed with this CL.

The CL description needs to be updated, as it doesn't use RVH anymore.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
content/browser/download/download_request_handle.cc:54: ->GetAsWebContents();
This method isn't very good and we should avoid using it. We have
WebContents::FromRenderFrameHost, so I think similarly we can add
WebContentsImpl::FromFrameTreeNode (or FTN ID), which will help here.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:183: DidStartLoading(true);
Does that and the DidStopLoading code fix issues on their own? I wonder if they
belong logically in a separate CL.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl(
This reformat seems less readable : (

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1942:
info.is_main_frame, info.parent_is_main_frame,
This is even more confusing, some params are on their own, some are bunched up
together. I'd defer this to David, but to me this is better kept with the
original formatting.

[clamy, 2015-04-27 14:34:44]

Thanks! I agree that in the long term this would be a nice architecture to have,
but would also like not to block the advancement of PlzNavigate on it if you
agree with it.

Nasko, you just reviewed the test patch set I was uploading before I got through
it to look at formatting issues and else :). This latest patchset uses either
the FTN id or the routing id/child id to get the DownloadManager (because we
cannot have just one or the other see my comment in
content/browser/download/download_request_handle.cc). With that all download
browser tests pass with PlzNavigate enabled.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
content/browser/download/download_request_handle.cc:57: 
If we use the FTN id, we still have to rely on the rvh/routing id in a certain
number of cases. In particular, some downloads are created through
RDH::BeginDownload, which is part of the public RDH interface. This function
takes as a parameter the child id and route id and creates a ResourceRequest
based on that. Since we cannot have the FTN id exposed in a public interface in
content for now, some of the DownloadRequestHandle will have the FTN id info
right, others will have the child id/route id right.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:183: DidStartLoading(true);
On 2015/04/27 14:24:11, nasko (very slow until 04-27) wrote:
> Does that and the DidStopLoading code fix issues on their own? I wonder if
they
> belong logically in a separate CL.

No this was a test patch that I uploaded and was reviewing before sending it
out. I have to add this locally for the DidStart/StopLoading to get signaled and
the tests to work (and normally remove it when sending patches out but forgot
here). Hopefully Fabrice's CL will be ready soon.

[davidben, 2015-04-27 14:55:18]

On 2015/04/27 14:34:44, clamy wrote:
> Thanks! I agree that in the long term this would be a nice architecture to
have,
> but would also like not to block the advancement of PlzNavigate on it if you
> agree with it.

Haven't yet looked through the CL---have a bit of a backlog from being sick last
week, but I agree that it makes sense to avoid blocking if possible. I'm unclear
on whether the RVH/RFH version would have worked because of the initial load,
but using FTN sounds reasonable to me. Though whether or not the RDH cleanup
blocks PlzNavigate, I imagine PlzNavigate will have to block on getting WebView,
extensions, and network auth dialogs working. I suspect that'll effectively
block on #3. (#1 and #2 should be pretty separable.)

[clamy, 2015-04-27 16:19:12]

Thanks! Yes it's quite likely that we will have some more issues with the loader
code tying things to the child id. Even though, I'd like to proceed with this
patch (keeping in mind that if we end up having to do some larger changes in the
loader code, the download code could benefit from it).

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/downloa...
content/browser/download/download_request_handle.cc:54: ->GetAsWebContents();
On 2015/04/27 14:24:11, nasko (very slow until 04-27) wrote:
> This method isn't very good and we should avoid using it. We have
> WebContents::FromRenderFrameHost, so I think similarly we can add
> WebContentsImpl::FromFrameTreeNode (or FTN ID), which will help here.

I created a WebContentsImpl::FromFrameTreeNode method that returns a
WebContentsImpl* using WebContents::FromRenderFrameHost.

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl(
On 2015/04/27 14:24:11, nasko (very slow until 04-27) wrote:
> This reformat seems less readable : (

Done. (git cl format is not always optimal..)

https://codereview.chromium.org/1079163008/diff/50001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1942:
info.is_main_frame, info.parent_is_main_frame,
On 2015/04/27 14:24:11, nasko (very slow until 04-27) wrote:
> This is even more confusing, some params are on their own, some are bunched up
> together. I'd defer this to David, but to me this is better kept with the
> original formatting.

Done (git cl format again).

[clamy, 2015-05-19 11:50:38]

@davidben: friendly ping now that I'm back from vacation :)

[davidben, 2015-05-19 19:40:46]

Oh, sorry, didn't realize you were blocking on me. I probably won't get to
looking at this under tomorrow since I'm on network bug triage today.

[davidben, 2015-05-19 19:41:05]

On 2015/05/19 19:40:46, David Benjamin wrote:
> Oh, sorry, didn't realize you were blocking on me. I probably won't get to
> looking at this under tomorrow since I'm on network bug triage today.

s/under tomorrow/until tomorrow/

[carlosk, 2015-05-19 20:20:49]

No, I don't think we're blocked by that right now. We're going over all
content browser tests right now so we might get there at some point.

What we wanted was just to have an idea of how to tackle it when that time
arrives.
On May 19, 2015 21:41, <davidben@chromium.org> wrote:

> On 2015/05/19 19:40:46, David Benjamin wrote:
>
>> Oh, sorry, didn't realize you were blocking on me. I probably won't get to
>> looking at this under tomorrow since I'm on network bug triage today.
>>
>
> s/under tomorrow/until tomorrow/
>
> https://codereview.chromium.org/1079163008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[carlosk, 2015-05-19 20:22:17]

Duh, sorry, ignore my last email. I mixed up the email threads.
On May 19, 2015 22:20, "Carlos Knippschild" <carlosk@chromium.org> wrote:

> No, I don't think we're blocked by that right now. We're going over all
> content browser tests right now so we might get there at some point.
>
> What we wanted was just to have an idea of how to tackle it when that time
> arrives.
> On May 19, 2015 21:41, <davidben@chromium.org> wrote:
>
>> On 2015/05/19 19:40:46, David Benjamin wrote:
>>
>>> Oh, sorry, didn't realize you were blocking on me. I probably won't get
>>> to
>>> looking at this under tomorrow since I'm on network bug triage today.
>>>
>>
>> s/under tomorrow/until tomorrow/
>>
>> https://codereview.chromium.org/1079163008/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2015-05-20 21:45:39]

lgtm. I'm rather sad downloads needs to know about this, but that can be
resolved later.

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
content/browser/download/download_request_handle.cc:46: // WebContents.
Could you add a

// TODO(davidben): This logic should be abstracted within the ResourceLoader
stack. https://crbug.com/376003

Since we'll need to do this request -> WebContents hop for other things too like
HTTP auth. I suspect we'll want to refactor this somewhat later.

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
content/browser/download/download_request_handle.cc:66: // DownloadManager.
Ditto with the TODO.

[clamy, 2015-05-22 16:01:29]

Thanks! @nasko: PTAL

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
content/browser/download/download_request_handle.cc:46: // WebContents.
On 2015/05/20 21:45:38, David Benjamin wrote:
> Could you add a
> 
> // TODO(davidben): This logic should be abstracted within the ResourceLoader
> stack. https://crbug.com/376003
> 
> Since we'll need to do this request -> WebContents hop for other things too
like
> HTTP auth. I suspect we'll want to refactor this somewhat later.

Done.

https://codereview.chromium.org/1079163008/diff/70001/content/browser/downloa...
content/browser/download/download_request_handle.cc:66: // DownloadManager.
On 2015/05/20 21:45:38, David Benjamin wrote:
> Ditto with the TODO.

Done.

[nasko, 2015-05-22 16:48:07]

Mostly nits. The important thing to note is that FrameTreeNode ID changed from
int64 to int recently, so this CL needs to be updated to reflect that. Once
that's fixed, LGTM

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.cc:61: return NULL;
nit: might as well change NULL to nullptr as we are touching these methods.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.cc:75: if (frame_tree_node) {
nit: Why not follow the style of this method by checking for null and returning
immediately instead of nesting if statements?

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
File content/browser/download/download_request_handle.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.h:64: int64
frame_tree_node_id);
FTN id is now int, not in64. It changed recently.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.h:88: int64
frame_tree_node_id_;
int

https://codereview.chromium.org/1079163008/diff/110001/content/browser/frame_...
File content/browser/frame_host/navigation_request_info.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/frame_...
content/browser/frame_host/navigation_request_info.h:44: const int64
frame_tree_node_id;
int

https://codereview.chromium.org/1079163008/diff/110001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:46: int64
frame_tree_node_id,
int

[clamy, 2015-05-26 10:43:11]

Thanks!

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.cc:61: return NULL;
On 2015/05/22 16:48:06, nasko wrote:
> nit: might as well change NULL to nullptr as we are touching these methods.

Done.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.cc:75: if (frame_tree_node) {
On 2015/05/22 16:48:06, nasko wrote:
> nit: Why not follow the style of this method by checking for null and
returning
> immediately instead of nesting if statements?

Because when we resume a download we don't have access to the FTN id, but to the
RVH routing id. Therefore, we want the second part to execute in that case.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
File content/browser/download/download_request_handle.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.h:64: int64
frame_tree_node_id);
On 2015/05/22 16:48:07, nasko wrote:
> FTN id is now int, not in64. It changed recently.

Done.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/downlo...
content/browser/download/download_request_handle.h:88: int64
frame_tree_node_id_;
On 2015/05/22 16:48:06, nasko wrote:
> int

Done.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/frame_...
File content/browser/frame_host/navigation_request_info.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/frame_...
content/browser/frame_host/navigation_request_info.h:44: const int64
frame_tree_node_id;
On 2015/05/22 16:48:07, nasko wrote:
> int

Done.

https://codereview.chromium.org/1079163008/diff/110001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1079163008/diff/110001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:46: int64
frame_tree_node_id,
On 2015/05/22 16:48:07, nasko wrote:
> int

Done.

[clamy, 2015-05-26 10:43:18]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-05-26 10:43:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1079163008/#ps130001
(title: "Rebase + addressed Nasko's comments")

[commit-bot: I haz the power, 2015-05-26 10:43:28]

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

[commit-bot: I haz the power, 2015-05-26 13:07:33]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-05-26 13:08:15]

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

[Randy Smith (Not in Mondays), 2015-05-29 15:23:20]

If possible, I'd like some download owner (someone in
content/browser/download/OWNERS, but asanka & I are the primary folks) review
stuff going into the download code, both because it can be tricky code and
because we'd like to know about changes in it :-}.  Knowing about this CL would
have saved me some time in the work I'm doing on http://crbug.com/482049.  

Not a big deal, just a heads up for next time (to both reviewers and committer).