PlzNavigate: support upgrade-insecure-navigation.

PlzNavigate: support upgrade-insecure-navigation.

With enable-browser-side-navigation, ResourceRequests were not upgraded
in case of a navigation.

Request upgrade is now made few line above, just before being
intercepted by shouldContinueForNavigationPolicy.

BUG=576270
Committed: https://crrev.com/9160af0215bceea0e0c97ddc8170228b84bc4eb6
Cr-Commit-Position: refs/heads/master@{#417949}

[arthursonzogni, 2016-08-26 14:58:59]

arthursonzogni@chromium.org changed reviewers:
+ mkwst@chromium.org

[arthursonzogni, 2016-08-26 14:58:59]

Hi Mike!

I am trying to support upgrade-insecure-request with browser-side-navigation
(aka PlzNavigate).

I have written a little quickfix that makes the last remaining tests pass.
Though, there is things I need to ask you.

My patch consists in upgrading the navigation request a little bit before in
FrameLoader::startLoad() (in fact, just before the request is intercepted by
PlzNavigate), but there we don't have a DocumentLoader yet (neither a
FrameFetchContext).

So I transformed FrameFetchContext:upgradeInsecureRequest into a function and
used it with appropriate(I think ?) arguments. But I do not intend to keep this
like this.
How do you think I can achieve the same thing but in a better way?

I see you are working on moving the feature on the browser process:
https://codereview.chromium.org/2053693002/
Maybe after this, there would be a more elegant way of doing what I want.

Thanks!

Note:
http/tests/security/upgrade-insecure-requests/https-header-top-level.html
needs also the CL(2269653002) for passing.

[Nate Chapin, 2016-08-26 16:04:34]

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

[Nate Chapin, 2016-08-26 16:04:35]

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:671: void
FrameFetchContext::upgradeInsecureRequest(ResourceRequest& resourceRequest,
Document* document, FrameLoader* frameLoader)
Take a FrameLoader& instead?

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1455:
m_provisionalDocumentLoader->upgradeInsecureRequest();
Looks like DocumentLoader::upgradeInsecureRequest() is no longer necessary.

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1428:
FrameFetchContext::upgradeInsecureRequest(request, nullptr, this);
Calls to the FetchContext outside of core/fetch tend to be pretty rare. I'm
wondering if this is a signal that upgradeInsecureRequest() should just live on
FrameLoader.

[arthursonzogni, 2016-08-26 16:38:34]

arthursonzogni@chromium.org changed reviewers:
- japhet@chromium.org

[arthursonzogni, 2016-08-26 16:38:35]

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:671: void
FrameFetchContext::upgradeInsecureRequest(ResourceRequest& resourceRequest,
Document* document, FrameLoader* frameLoader)
On 2016/08/26 16:04:34, Nate Chapin wrote:
> Take a FrameLoader& instead?

Yes we can. It is for saying it couldn't be null ?

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1455:
m_provisionalDocumentLoader->upgradeInsecureRequest();
On 2016/08/26 16:04:34, Nate Chapin wrote:
> Looks like DocumentLoader::upgradeInsecureRequest() is no longer necessary.

Yes!

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1428:
FrameFetchContext::upgradeInsecureRequest(request, nullptr, this);
On 2016/08/26 16:04:34, Nate Chapin wrote:
> Calls to the FetchContext outside of core/fetch tend to be pretty rare. I'm
> wondering if this is a signal that upgradeInsecureRequest() should just live
on
> FrameLoader.

Yes, I was wondering where I could put this function.
I keep in in FrameFetchContext and would to know what could be the best thing to
do then.
I had no intention to keep it here :)

This method needs to know which policy to apply and of course the request to be
updated. But maybe it is easier to find from which object to take these
information when you are in a FrameFetchContext.

[Nate Chapin, 2016-08-26 17:57:47]

On 2016/08/26 16:38:35, arthursonzogni wrote:
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):
> 
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:671: void
> FrameFetchContext::upgradeInsecureRequest(ResourceRequest& resourceRequest,
> Document* document, FrameLoader* frameLoader)
> On 2016/08/26 16:04:34, Nate Chapin wrote:
> > Take a FrameLoader& instead?
> 
> Yes we can. It is for saying it couldn't be null ?

Yeah, and a lot of other uses of FrameLoader are referenced-based, because
LocalFrame::loader() returns a reference.

> 
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):
> 
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:1455:
> m_provisionalDocumentLoader->upgradeInsecureRequest();
> On 2016/08/26 16:04:34, Nate Chapin wrote:
> > Looks like DocumentLoader::upgradeInsecureRequest() is no longer necessary.
> 
> Yes!
> 
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:1428:
> FrameFetchContext::upgradeInsecureRequest(request, nullptr, this);
> On 2016/08/26 16:04:34, Nate Chapin wrote:
> > Calls to the FetchContext outside of core/fetch tend to be pretty rare. I'm
> > wondering if this is a signal that upgradeInsecureRequest() should just live
> on
> > FrameLoader.
> 
> Yes, I was wondering where I could put this function.
> I keep in in FrameFetchContext and would to know what could be the best thing
to
> do then.
> I had no intention to keep it here :)
> 
> This method needs to know which policy to apply and of course the request to
be
> updated. But maybe it is easier to find from which object to take these
> information when you are in a FrameFetchContext.

I think putting it in FrameLoader makes sense, but you may want to wait for
mkwst's opinion as a sanity check. You still would need to pass the Document* in
to ensure that FrameLoader didn't use the wrong Document, but I think it would
end up being a little bit better organized overall.

[arthursonzogni, 2016-08-29 08:37:22]

Thanks for your comments!

I see that mkwst is working on moving a lot of upgrade-insecure-request stuff
recently, I think I will wait to know where is the place he would like to put
this function.

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:125: LocalFrame*
frame() const; // Can be null
Because in the definition, there is an "ASSERT(frame)", the function is not
expected to return null. 
Maybe this was the case in the past and the comment "Can be null" was not
removed. Or maybe this is in case the definition could change in the future and
returns null.
Do you think I can I assume frame() will never return null ? Then maybe we could
remove the comment.

[Mike West, 2016-08-31 13:22:50]

Sorry for the delay; I've been OOO for a little while.

I think Nate's comments make sense. I'm not entirely sure what moving the
functionality to FrameLoader would look like, but if you think it makes sense,
I'm willing to give it a try. The general direction looks reasonable.

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2284683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1428:
FrameFetchContext::upgradeInsecureRequest(request, nullptr, this);
On 2016/08/26 at 16:38:34, arthursonzogni wrote:
> On 2016/08/26 16:04:34, Nate Chapin wrote:
> > Calls to the FetchContext outside of core/fetch tend to be pretty rare. I'm
> > wondering if this is a signal that upgradeInsecureRequest() should just live
on
> > FrameLoader.
> 
> Yes, I was wondering where I could put this function.
> I keep in in FrameFetchContext and would to know what could be the best thing
to do then.
> I had no intention to keep it here :)
> 
> This method needs to know which policy to apply and of course the request to
be updated. But maybe it is easier to find from which object to take these
information when you are in a FrameFetchContext.

It's probably a signal that FrameFetchContext isn't a good fit for things that
modify navigations, potentially across renderer processes.

[arthursonzogni, 2016-08-31 14:55:32]

Hi Mike,

I apply what Nate suggested. I think it looks better.

[arthursonzogni, 2016-08-31 14:56:36]

Patchset #2 (id:20001) has been deleted

[Mike West, 2016-09-12 07:24:03]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 07:24:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-09-12 07:26:25]

LGTM (again, sorry for the delay... it should never take this long to get a
reasonable review).

[commit-bot: I haz the power, 2016-09-12 10:52:17]

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

[commit-bot: I haz the power, 2016-09-12 10:52:18]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-09-12 16:01:42]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2016-09-12 16:01:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-09-12 16:02:21]

On 2016/09/12 07:26:25, Mike West wrote:
> LGTM (again, sorry for the delay... it should never take this long to get a
> reasonable review).

Thanks!

[commit-bot: I haz the power, 2016-09-12 16:06:20]

Committed patchset #2 (id:30001)

[commit-bot: I haz the power, 2016-09-12 16:08:40]

Description was changed from

==========
PlzNavigate: support upgrade-insecure-navigation.

With enable-browser-side-navigation, ResourceRequests were not upgraded
in case of a navigation.

Request upgrade is now made few line above, just before being
intercepted by shouldContinueForNavigationPolicy.

BUG=576270
==========

to

==========
PlzNavigate: support upgrade-insecure-navigation.

With enable-browser-side-navigation, ResourceRequests were not upgraded
in case of a navigation.

Request upgrade is now made few line above, just before being
intercepted by shouldContinueForNavigationPolicy.

BUG=576270

Committed: https://crrev.com/9160af0215bceea0e0c97ddc8170228b84bc4eb6
Cr-Commit-Position: refs/heads/master@{#417949}
==========

[commit-bot: I haz the power, 2016-09-12 16:08:41]

Patchset 2 (id:??) landed as
https://crrev.com/9160af0215bceea0e0c97ddc8170228b84bc4eb6
Cr-Commit-Position: refs/heads/master@{#417949}