PlzNavigate: fix issue preventing navigations to WebUIs

PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[Closed in favor of r396193]

[clamy, 2016-05-20 15:11:25]

Description was changed from

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
==========

to

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
==========

[clamy, 2016-05-20 15:11:25]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, dbeam@chromium.org

[clamy, 2016-05-20 15:12:45]

@dbeam, creis: PTAL
As explained in the CL description, following
https://codereview.chromium.org/1985983002 all navigations to WebUIs stopped
working when PlzNavigate is enabled (since you always get invalid ids for
RenderFrameHosts, you can't find it in the map).

[Charlie Reis, 2016-05-20 18:50:33]

https://codereview.chromium.org/2002633002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:278: // PlzNavigate: no
navigation to a WebUI is coming from a renderer.
Hmm.  This is only a safe change for PlzNavigate if the
CheckStoragePartitionMatches code isn't needed at all, right?

It looks like dbeam's bug fix in https://codereview.chromium.org/1985983002
(i.e., actually checking against the host whitelist and not allowing all
chrome:// URLs) exposed that StoragePartition check doesn't work for
PlzNavigate.

Is there some way to find out what StoragePartition a PlzNavigate request is
being made from?  Or can we show that CheckStoragePartitionMatches is
unnecessary (per dbeam's TODO below) and remove it for everyone?  I'm a bit
hesitant to turn it off just for PlzNavigate if it might matter in some cases.

(Worst case, we can temporarily do something like this to skip the check for
PlzNavigate, but that's a potential security bug until we determine that the
check is unnecessary.)

[clamy, 2016-05-23 14:14:33]

Description was changed from

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
==========

to

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-05-23 14:21:21]

Thanks! I've looked up the code at where we choose a storage partition in
PlzNavigate, and have tried to match the check.

https://codereview.chromium.org/2002633002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:278: // PlzNavigate: no
navigation to a WebUI is coming from a renderer.
On 2016/05/20 18:50:32, Charlie Reis (slow to reply) wrote:
> Hmm.  This is only a safe change for PlzNavigate if the
> CheckStoragePartitionMatches code isn't needed at all, right?
> 
> It looks like dbeam's bug fix in https://codereview.chromium.org/1985983002
> (i.e., actually checking against the host whitelist and not allowing all
> chrome:// URLs) exposed that StoragePartition check doesn't work for
> PlzNavigate.
> 
> Is there some way to find out what StoragePartition a PlzNavigate request is
> being made from?  Or can we show that CheckStoragePartitionMatches is
> unnecessary (per dbeam's TODO below) and remove it for everyone?  I'm a bit
> hesitant to turn it off just for PlzNavigate if it might matter in some cases.
> 
> (Worst case, we can temporarily do something like this to skip the check for
> PlzNavigate, but that's a potential security bug until we determine that the
> check is unnecessary.)

There's one place in the NavigationRequest where we do get a StoragePartition to
initialize ServiceWorkers for the navigation. I've cleaned up the code to get
it, and made it available to check in 
URLRequestChromeJob::CheckStoragePartitionMatches on the UI thread.

[Charlie Reis, 2016-05-23 17:26:40]

Thanks.  Seems reasonable until we can verify that the StoragePartition check
isn't needed.

Unfortunately, looks like it's failing some tests?  Do you think it's more
useful to make this work or investigate a bit to show that the check isn't
needed?  I'm fine either way.

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

https://codereview.chromium.org/2002633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:266:
DCHECK(associated_site_instance_type_ != AssociatedSiteInstanceType::NONE);
DCHECK_NE?

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

https://codereview.chromium.org/2002633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:743: :
NavigationRequest::AssociatedSiteInstanceType::SPECULATIVE);
Just curious, what was the reason this needed to be moved inside
GetFrameHostForNavigation?  Was there a path that wasn't going through
DidCreateNavigationRequest?

[Dan Beam, 2016-05-23 19:02:58]

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
when will render_process_id be -1 here?

[Charlie Reis, 2016-05-23 19:11:17]

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/23 19:02:58, Dan Beam wrote:
> when will render_process_id be -1 here?

With PlzNavigate, the renderer process may not be created until after the
network request is made and is ready to commit.  See https://crbug.com/368813.

[Dan Beam, 2016-05-23 19:17:10]

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> On 2016/05/23 19:02:58, Dan Beam wrote:
> > when will render_process_id be -1 here?
> 
> With PlzNavigate, the renderer process may not be created until after the
> network request is made and is ready to commit.  See https://crbug.com/368813.

are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can return
true and also set render_process_id = -1?

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

[Charlie Reis, 2016-05-23 19:50:10]

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/23 19:17:09, Dan Beam wrote:
> On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> > On 2016/05/23 19:02:58, Dan Beam wrote:
> > > when will render_process_id be -1 here?
> > 
> > With PlzNavigate, the renderer process may not be created until after the
> > network request is made and is ready to commit.  See
https://crbug.com/368813.
> 
> are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can return
> true and also set render_process_id = -1?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

That's what patch set 1 implies.  I agree that seems a bit unexpected, looking
at the implementation of GetRenderFrameForRequest.  Camille, does PlzNavigate
install the URLRequestUserData on the request with -1 for the process ID and
frame routing ID?

[Dan Beam, 2016-05-23 20:41:09]

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/23 19:50:10, Charlie Reis (slow to reply) wrote:
> On 2016/05/23 19:17:09, Dan Beam wrote:
> > On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> > > On 2016/05/23 19:02:58, Dan Beam wrote:
> > > > when will render_process_id be -1 here?
> > > 
> > > With PlzNavigate, the renderer process may not be created until after the
> > > network request is made and is ready to commit.  See
> https://crbug.com/368813.
> > 
> > are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can
return
> > true and also set render_process_id = -1?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> 
> That's what patch set 1 implies.  I agree that seems a bit unexpected, looking
> at the implementation of GetRenderFrameForRequest.  Camille, does PlzNavigate
> install the URLRequestUserData on the request with -1 for the process ID and
> frame routing ID?

I removed a constant in my previous CL that might be useful here because I
thought we'd never hit a case where we have a render_process_id of -1

https://codereview.chromium.org/1985983002/diff/60001/content/browser/webui/u...

[clamy, 2016-05-24 11:18:32]

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

https://codereview.chromium.org/2002633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:743: :
NavigationRequest::AssociatedSiteInstanceType::SPECULATIVE);
On 2016/05/23 17:26:40, Charlie Reis (slow to reply) wrote:
> Just curious, what was the reason this needed to be moved inside
> GetFrameHostForNavigation?  Was there a path that wasn't going through
> DidCreateNavigationRequest?

Yes this was a TODO to move it to GetFrameHostForNavigation.
GetFrameHostForNavigation has 2 other call sites (at commit time).

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/23 20:41:09, Dan Beam wrote:
> On 2016/05/23 19:50:10, Charlie Reis (slow to reply) wrote:
> > On 2016/05/23 19:17:09, Dan Beam wrote:
> > > On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> > > > On 2016/05/23 19:02:58, Dan Beam wrote:
> > > > > when will render_process_id be -1 here?
> > > > 
> > > > With PlzNavigate, the renderer process may not be created until after
the
> > > > network request is made and is ready to commit.  See
> > https://crbug.com/368813.
> > > 
> > > are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can
> return
> > > true and also set render_process_id = -1?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> > 
> > That's what patch set 1 implies.  I agree that seems a bit unexpected,
looking
> > at the implementation of GetRenderFrameForRequest.  Camille, does
PlzNavigate
> > install the URLRequestUserData on the request with -1 for the process ID and
> > frame routing ID?
> 
> I removed a constant in my previous CL that might be useful here because I
> thought we'd never hit a case where we have a render_process_id of -1
> 
>
https://codereview.chromium.org/1985983002/diff/60001/content/browser/webui/u...
PlzNavigate does indeed creates a URLRequestUserData for the request. It is
automatically created when we try to associate the ResourceRequestInfoImpl to
the request, by checking that we have a RFH and RPH id. The function checking
that always return true, and does not check that these ids are valid. Should I
change it?

[Charlie Reis, 2016-05-24 22:14:32]

I don't know what to recommend here.

The best case scenario is that we show the storage partition check isn't needed
for anyone and we rip the whole thing out.

Barring that, we would have to get the check to work for PlzNavigate, which may
or may not be tricky given the absence of a ResourceRequestInfoImpl at the time
of the request in the WebUIWebViewBrowserTest tests.

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/24 11:18:32, clamy wrote:
> On 2016/05/23 20:41:09, Dan Beam wrote:
> > On 2016/05/23 19:50:10, Charlie Reis (slow to reply) wrote:
> > > On 2016/05/23 19:17:09, Dan Beam wrote:
> > > > On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> > > > > On 2016/05/23 19:02:58, Dan Beam wrote:
> > > > > > when will render_process_id be -1 here?
> > > > > 
> > > > > With PlzNavigate, the renderer process may not be created until after
> the
> > > > > network request is made and is ready to commit.  See
> > > https://crbug.com/368813.
> > > > 
> > > > are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can
> > return
> > > > true and also set render_process_id = -1?
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> > > 
> > > That's what patch set 1 implies.  I agree that seems a bit unexpected,
> looking
> > > at the implementation of GetRenderFrameForRequest.  Camille, does
> PlzNavigate
> > > install the URLRequestUserData on the request with -1 for the process ID
and
> > > frame routing ID?
> > 
> > I removed a constant in my previous CL that might be useful here because I
> > thought we'd never hit a case where we have a render_process_id of -1
> > 

Yeah, we could reintroduce that old path for allowing the request when the
process ID is -1 (as in patch set 1), but that was meant for the case that
"Request was not issued by renderer."  In PlzNavigate, the process ID will be -1
even when it was issued by the renderer, which is why I was hesitant to add it
back.


> >
>
https://codereview.chromium.org/1985983002/diff/60001/content/browser/webui/u...
> PlzNavigate does indeed creates a URLRequestUserData for the request. It is
> automatically created when we try to associate the ResourceRequestInfoImpl to
> the request, by checking that we have a RFH and RPH id. The function checking
> that always return true, and does not check that these ids are valid. Should I
> change it?

I don't know that code well enough to say.  It seems a bit surprising for
GetRenderFrameForRequest to return true and give back -1s, but I don't know if
that's a problem for any code.

[clamy, 2016-05-25 14:30:11]

Based on my understanding that this check is meant to prevent an unauthorized
renderer from showing a webui url, I've updated the patch set to do the check
when we're ready to commit and have selectioned a renderer for the navigation.
If we find out that the renderer can't navigate to the webui url, we just cancel
the navigation.

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

https://codereview.chromium.org/2002633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:266:
DCHECK(associated_site_instance_type_ != AssociatedSiteInstanceType::NONE);
On 2016/05/23 17:26:40, Charlie Reis wrote:
> DCHECK_NE?

Done.

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:433: } else if
(render_process_id == -1 && IsBrowserSideNavigationEnabled()) {
On 2016/05/24 22:14:32, Charlie Reis wrote:
> On 2016/05/24 11:18:32, clamy wrote:
> > On 2016/05/23 20:41:09, Dan Beam wrote:
> > > On 2016/05/23 19:50:10, Charlie Reis (slow to reply) wrote:
> > > > On 2016/05/23 19:17:09, Dan Beam wrote:
> > > > > On 2016/05/23 19:11:16, Charlie Reis (slow to reply) wrote:
> > > > > > On 2016/05/23 19:02:58, Dan Beam wrote:
> > > > > > > when will render_process_id be -1 here?
> > > > > > 
> > > > > > With PlzNavigate, the renderer process may not be created until
after
> > the
> > > > > > network request is made and is ready to commit.  See
> > > > https://crbug.com/368813.
> > > > > 
> > > > > are you saying that ResourceRequestInfo::GetRenderFrameForRequest()
can
> > > return
> > > > > true and also set render_process_id = -1?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> > > > 
> > > > That's what patch set 1 implies.  I agree that seems a bit unexpected,
> > looking
> > > > at the implementation of GetRenderFrameForRequest.  Camille, does
> > PlzNavigate
> > > > install the URLRequestUserData on the request with -1 for the process ID
> and
> > > > frame routing ID?
> > > 
> > > I removed a constant in my previous CL that might be useful here because I
> > > thought we'd never hit a case where we have a render_process_id of -1
> > > 
> 
> Yeah, we could reintroduce that old path for allowing the request when the
> process ID is -1 (as in patch set 1), but that was meant for the case that
> "Request was not issued by renderer."  In PlzNavigate, the process ID will be
-1
> even when it was issued by the renderer, which is why I was hesitant to add it
> back.
> 
> 
> > >
> >
>
https://codereview.chromium.org/1985983002/diff/60001/content/browser/webui/u...
> > PlzNavigate does indeed creates a URLRequestUserData for the request. It is
> > automatically created when we try to associate the ResourceRequestInfoImpl
to
> > the request, by checking that we have a RFH and RPH id. The function
checking
> > that always return true, and does not check that these ids are valid. Should
I
> > change it?
> 
> I don't know that code well enough to say.  It seems a bit surprising for
> GetRenderFrameForRequest to return true and give back -1s, but I don't know if
> that's a problem for any code.

I'm a bit wary of changing the behavior of
ResourceRequestInfo::GetRenderFrameForRequest() since there are othe r request
which can have ids of -1 (eg browser-initiated downloads).

My understanding is that the goal of the patch is to check using the storage
partition that a renderer is allowed to access data from a webui or not.

In PlzNavigate we have 2 use cases:
- the normal navigation request, which goes through the browser and creates a
NavigationRequest (which creates a ResourceRequest with rph id -1)
- the resource request made by the render to get the response body. We enforce
in ResourceDispatcherHost that any frame ResourceRequest made by a renderer has
to be for a blob url. So they woudln't be concerned by that check.

To handle case 1, I wrote a new version of the patch set, where when we're ready
to commit we check if the renderer we want to use has the right to display the
webui url, and cancel the navigation if not.

[Charlie Reis, 2016-05-25 21:38:02]

This would probably work if we need to, but I'm still sad that we're growing the
logic around this probably-dead code.

I filed https://crbug.com/614808 to investigate removing the whole check, and
https://codereview.chromium.org/2007223003/ seems to be passing try jobs so far.
 That seems far preferable to me.

If we end up not being able to land that, then I've left some comments below for
making this a more precise change.

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

https://codereview.chromium.org/2002633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:266:
DCHECK(associated_site_instance_type_ != AssociatedSiteInstanceType::NONE);
On 2016/05/25 14:30:11, clamy wrote:
> On 2016/05/23 17:26:40, Charlie Reis wrote:
> > DCHECK_NE?
> 
> Done.

Wasn't fixed, but I don't think we should add this function anyway if it isn't
needed here.  (See my later comment.)

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

https://codereview.chromium.org/2002633002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:270: StoragePartition*
NavigationRequest::GetStoragePartitionForNavigation() {
Is this needed anymore?  If it's just cleanup, we should do it in a separate CL,
since we would be able to revert the rest of this CL if we end up proving that
the StoragePartition check is unnecessary.

https://codereview.chromium.org/2002633002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2002633002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:277: const
ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request_);
Wasn't this null in your previous patch set during the WebUIWebViewBrowserTest
tests?  I don't see any try jobs with this patch, so I've kicked one off.

https://codereview.chromium.org/2002633002/diff/40001/content/browser/webui/w...
File content/browser/webui/web_ui_impl.h (right):

https://codereview.chromium.org/2002633002/diff/40001/content/browser/webui/w...
content/browser/webui/web_ui_impl.h:29: const GURL& url);
nit: Wrong indent, add blank line after, and please add a comment.

Please mention something about how this could be removed, per issue 614808.

[clamy, 2016-05-26 14:56:26]

Ok. https://codereview.chromium.org/2007223003/ is fixing the problem we see in
PlzNavigate, so if it lands I can close this CL.

[Charlie Reis, 2016-05-26 20:29:47]

On 2016/05/26 14:56:26, clamy wrote:
> Ok. https://codereview.chromium.org/2007223003/ is fixing the problem we see
in
> PlzNavigate, so if it lands I can close this CL.

Fix landed in r396193, so we can close this.  (If it ends up needing to be
reverted for some reason, we can revisit.)

[Charlie Reis, 2016-05-26 20:30:27]

Description was changed from

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: fix issue preventing navigations to WebUIs

This CL fixes an issue which was causing all navigations ot WebUIs to fail when
browser side navigation is enabled.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[Closed in favor of r396193]
==========