Executed HTTPS upgrade before notifying the start of the provisional load.

Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

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

Committed: https://crrev.com/2ece5a75956f9df5c0a78939a8f06d4e09b6e860
Cr-Commit-Position: refs/heads/master@{#404567}

[carlosk, 2016-06-28 13:31:55]

Description was changed from

==========
Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

BUG=618659
==========

to

==========
Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

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

[carlosk, 2016-06-28 14:44:31]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, mkwst@chromium.org

[carlosk, 2016-06-28 14:44:32]

clamy@: PTAL at the content/** files.

mkwst@: PTAL at the third_party/WebKit/** files.

This implements what we discussed during our offline meeting in MUC.

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:49:
last_committed_url_ = GURL();
These were in fact never updated given the if-check for handle_ in the beginning
of this method. The comment added to the class now reflects that.

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:46:
last_unmatched_url_ = navigation_handle->GetURL();
I added this new member to allow for more meaningful error messages for
failures. See usage in new test.

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:101: const
GURL& committed_url() { return committed_url_; }
Renamed this because it was confusing: last_... gave me the impression it would
be updated with further navigations which is not the case (it is only set once).

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:200:
CHECK(!will_start_called_);
Switched this to a CHECK because calls are useless after the 1st navigation,
what was confusing to me at first. Also updated the class comment to explain it.

https://codereview.chromium.org/2109633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2109633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1458:
m_provisionalDocumentLoader->upgradeInsecureRequest();
This was the best way I found to execute the upgrading from here. Other options
I considered were:

- Create an early FrameFetchContext: but it needs a DocumentLoader instance
(chicken and egg situation).
- Make FrameFetchContext::upgradeInsecureRequest static: would expose the whole
document vs loader checks and that didn't look good.
- Add a non-cost getter from the DocumentLoader's request: the loader already
does a lot of modifications of the request internally and the call now enclosed
in DocumentLoader::upgradeInsecureRequest looks better there than here.

[nasko, 2016-06-28 22:38:19]

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

[nasko, 2016-06-28 22:38:20]

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:49:
last_committed_url_ = GURL();
On 2016/06/28 14:44:32, carlosk wrote:
> These were in fact never updated given the if-check for handle_ in the
beginning
> of this method. The comment added to the class now reflects that.

I'm not sure I follow. DidStartNavigation will be called multiple times for
WebContents. The handle_ check ensures that it doesn't track more than one
handle at a time, but it can very well track multiple. At the
DidFinishNavigation end, the handle_ is reset, so a subsequent navigation to the
expected URL will still go through here.

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:101: const
GURL& committed_url() { return committed_url_; }
On 2016/06/28 14:44:32, carlosk wrote:
> Renamed this because it was confusing: last_... gave me the impression it
would
> be updated with further navigations which is not the case (it is only set
once).

It actually can be updated multiple times : ).

[carlosk, 2016-07-01 20:22:52]

Thanks!

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:49:
last_committed_url_ = GURL();
On 2016/06/28 22:38:20, nasko wrote:
> On 2016/06/28 14:44:32, carlosk wrote:
> > These were in fact never updated given the if-check for handle_ in the
> beginning
> > of this method. The comment added to the class now reflects that.
> 
> I'm not sure I follow. DidStartNavigation will be called multiple times for
> WebContents. The handle_ check ensures that it doesn't track more than one
> handle at a time, but it can very well track multiple. At the
> DidFinishNavigation end, the handle_ is reset, so a subsequent navigation to
the
> expected URL will still go through here.

Yes, you are right, I missed that. Reverted and made a few more changes to make
it all clearer.

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

https://codereview.chromium.org/2109633002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:101: const
GURL& committed_url() { return committed_url_; }
On 2016/06/28 22:38:20, nasko wrote:
> On 2016/06/28 14:44:32, carlosk wrote:
> > Renamed this because it was confusing: last_... gave me the impression it
> would
> > be updated with further navigations which is not the case (it is only set
> once).
> 
> It actually can be updated multiple times : ).

Reverted.

[Mike West, 2016-07-04 08:33:25]

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

https://codereview.chromium.org/2109633002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:659:
ASSERT_FALSE(iframe_secure_url.has_port());
I'm missing something; how does this work? Don't you need to keep the random
port that `embedded_test_server()` comes up with, and do some sort of
port-replacement in the document in order for it to not timeout?

https://codereview.chromium.org/2109633002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2109633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:413: if
(request.resourceRequest().httpHeaderField("Upgrade-Insecure-Requests") !=
AtomicString("1"))
1. Can you add a comment describing the rationale for checking the header's
presence?

2. This could probably be moved into `FrameFetchContext::upgradeInsecureRequest`
as an early-exit, rather than complicating the callsites.

[carlosk, 2016-07-04 09:22:59]

Thanks.

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

https://codereview.chromium.org/2109633002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:659:
ASSERT_FALSE(iframe_secure_url.has_port());
On 2016/07/04 08:33:25, Mike West wrote:
> I'm missing something; how does this work? Don't you need to keep the random
> port that `embedded_test_server()` comes up with, and do some sort of
> port-replacement in the document in order for it to not timeout?

I'm testing only if the URL received by the NavigationHandle upon navigation
start is the expected HTTPS-upgraded one. The upgrade process seems to remove
the port hence my cleaning it up here. This test works well enough for asserting
that.

I'm not testing that the frame document is actually loaded. In fact the frame
navigation is abandoned at the end of the test before it commits. So it doesn't
matter that the port information was removed.

https://codereview.chromium.org/2109633002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2109633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:413: if
(request.resourceRequest().httpHeaderField("Upgrade-Insecure-Requests") !=
AtomicString("1"))
On 2016/07/04 08:33:25, Mike West wrote:
> 1. Can you add a comment describing the rationale for checking the header's
> presence?
> 
> 2. This could probably be moved into
`FrameFetchContext::upgradeInsecureRequest`
> as an early-exit, rather than complicating the callsites.

Both done. Much better indeed.

[Mike West, 2016-07-04 09:31:43]

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

https://codereview.chromium.org/2109633002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:659:
ASSERT_FALSE(iframe_secure_url.has_port());
On 2016/07/04 at 09:22:59, carlosk wrote:
> I'm testing only if the URL received by the NavigationHandle upon navigation
start is the expected HTTPS-upgraded one. The upgrade process seems to remove
the port hence my cleaning it up here. This test works well enough for asserting
that.

The upgrade should not remove the port. That is, `http://example.test:123/`
should become `https://example.test:123/` after the upgrade (similar to what
we're doing for HSTS: if it's not a default port, we leave it alone). See
`FrameFetchContextUpgradeTest::UpgradeInsecureResourceRequests` for instance...

If you needed to do this in order for things to work, then something's wrong. :(

https://codereview.chromium.org/2109633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2109633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:679: if
(resourceRequest.httpHeaderField("Upgrade-Insecure-Requests") ==
AtomicString("1"))
Can you add a test which verifies this behavior? Could be as simple as calling
the method twice on a request and verifying that the output is sane.

[Mike West, 2016-07-04 09:32:11]

Looks good except for the port thing in the browsertest, which I still don't
really get. :)

[clamy, 2016-07-04 11:22:09]

Thanks! A few comments below.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:25: // with the
expected URL. Overlapping navigations are ignored; create new
It's not clear to me whether this comment means "You should create new instances
as needed" or "The NavigationHandleObserver creates new instances as needed".
Also I don't really get what you mean by "Overlapping navigations are ignored".

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:47:
last_unmatched_start_url_ = navigation_handle->GetURL();
Rather than putting a new member for the start urls, which doesn't really fit
with what the class is doing, how about creating a new Observer that just
records all urls it has seen (in something like a vector). Then we can check if
the url was never in, which seems a bit more robust to me.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:132: //
generally not instantiated directly but through a
Is there any case where it is actually instantiated directly in tests? If not,
the comment should reflect that.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:183: // various
NavigationThrottle related events. Waiting is only allows for the
I think it be better to rephrase the first sentence as "Install a
TestNavigationThrottle on the next navigation and allows waiting for various
NavigationThrottle related events." This way it's clearer it only works for one
navigation.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:205:
CHECK(!will_start_called_);
This is not right. It's likely to fail with PlzNavigate enabled, as it's
perfectly possible to have will_start_called _before_ we actually start to wait
for this to happen (first navigation, it gets called right away).

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:230: void
DidStartNavigation(NavigationHandle* handle) override {
If we make it work for one navigation only, we should probably not install a
throttle if will_start was called.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:646:
ProvisionalLoadStartUrlIsHttpsUpgraded) {
Nit: how about renaming it "StartWithUpgradedUrl". This way once we make it work
with PlzNavigate it still has meaning. That would also require to rewrite the
comment: "Tests that the NavigationHandle gets the url upgraded to HTTPS from
the beginning of the navigation." which is what we want to ensure.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:669:
shell()->LoadURL(start_url);
I think this can be rewritten using the TestNavigationManager from
content/test/content_browser_test_utils_internal.h. Instantiate a
TestNavigationManager for upgraded url and WaitForWillStartRequest().

Then as mentioned above, you can have another observer that just records the
urls seen, and check that the unupgraded one was never received.

https://codereview.chromium.org/2109633002/diff/60001/content/test/data/navig...
File content/test/data/navigation_handle_https_upgrade_test.html (right):

https://codereview.chromium.org/2109633002/diff/60001/content/test/data/navig...
content/test/data/navigation_handle_https_upgrade_test.html:3:
<title>NavigationHandleImpl HTTPS upgrade test</title>
How about just naming the file HTTPS upgrade (same with the name). This way it's
easily reusable in other tests.

[carlosk, 2016-07-05 09:56:43]

Patchset #5 (id:80001) has been deleted

[carlosk, 2016-07-05 09:57:24]

Thanks the mix of your suggestions the test structure changed quite some but IMO
for the better.

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

https://codereview.chromium.org/2109633002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:659:
ASSERT_FALSE(iframe_secure_url.has_port());
On 2016/07/04 09:31:43, Mike West wrote:
> On 2016/07/04 at 09:22:59, carlosk wrote:
> > I'm testing only if the URL received by the NavigationHandle upon navigation
> start is the expected HTTPS-upgraded one. The upgrade process seems to remove
> the port hence my cleaning it up here. This test works well enough for
asserting
> that.
> 
> The upgrade should not remove the port. That is, `http://example.test:123/`
> should become `https://example.test:123/` after the upgrade (similar to what
> we're doing for HSTS: if it's not a default port, we leave it alone). See
> `FrameFetchContextUpgradeTest::UpgradeInsecureResourceRequests` for
instance...
> 
> If you needed to do this in order for things to work, then something's wrong.
:(

I updated the test so that it will check the same and cross site cases where
there's different behavior in regards to the the port. If this behavior is
incorrect it should be fix in another CL.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:25: // with the
expected URL. Overlapping navigations are ignored; create new
On 2016/07/04 11:22:09, clamy wrote:
> It's not clear to me whether this comment means "You should create new
instances
> as needed" or "The NavigationHandleObserver creates new instances as needed".

I agree this was confusing so I just removed that last sentence.

> Also I don't really get what you mean by "Overlapping navigations are
ignored".

This was a leftover from my previous change where I had misunderstood the
behavior.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:47:
last_unmatched_start_url_ = navigation_handle->GetURL();
On 2016/07/04 11:22:09, clamy wrote:
> Rather than putting a new member for the start urls, which doesn't really fit
> with what the class is doing, how about creating a new Observer that just
> records all urls it has seen (in something like a vector). Then we can check
if
> the url was never in, which seems a bit more robust to me.

Done.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:132: //
generally not instantiated directly but through a
On 2016/07/04 11:22:09, clamy wrote:
> Is there any case where it is actually instantiated directly in tests? If not,
> the comment should reflect that.

Done.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:183: // various
NavigationThrottle related events. Waiting is only allows for the
On 2016/07/04 11:22:09, clamy wrote:
> I think it be better to rephrase the first sentence as "Install a
> TestNavigationThrottle on the next navigation and allows waiting for various
> NavigationThrottle related events." This way it's clearer it only works for
one
> navigation.

That would be incorrect as it continues to install them for all following
navigations and keeps on incrementing the internal counters. See my current
usage of it.

But the waiting is still really impossible for the 2nd and further navigations.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:205:
CHECK(!will_start_called_);
On 2016/07/04 11:22:09, clamy wrote:
> This is not right. It's likely to fail with PlzNavigate enabled, as it's
> perfectly possible to have will_start_called _before_ we actually start to
wait
> for this to happen (first navigation, it gets called right away).

Reverted.

Now I understand why this was done this way. It had ran correctly with
PlzNavigate enabled so I guess this is a timing issue.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:230: void
DidStartNavigation(NavigationHandle* handle) override {
On 2016/07/04 11:22:09, clamy wrote:
> If we make it work for one navigation only, we should probably not install a
> throttle if will_start was called.

I am currently relying on that behavior as it updates the internal counters.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:646:
ProvisionalLoadStartUrlIsHttpsUpgraded) {
On 2016/07/04 11:22:09, clamy wrote:
> Nit: how about renaming it "StartWithUpgradedUrl". This way once we make it
work
> with PlzNavigate it still has meaning. That would also require to rewrite the
> comment: "Tests that the NavigationHandle gets the url upgraded to HTTPS from
> the beginning of the navigation." which is what we want to ensure.

Done. I changed the structure of the test a bit but kept the concept you
suggested.

https://codereview.chromium.org/2109633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:669:
shell()->LoadURL(start_url);
On 2016/07/04 11:22:09, clamy wrote:
> I think this can be rewritten using the TestNavigationManager from
> content/test/content_browser_test_utils_internal.h. Instantiate a
> TestNavigationManager for upgraded url and WaitForWillStartRequest().
> 
> Then as mentioned above, you can have another observer that just records the
> urls seen, and check that the unupgraded one was never received.

Done.

https://codereview.chromium.org/2109633002/diff/60001/content/test/data/navig...
File content/test/data/navigation_handle_https_upgrade_test.html (right):

https://codereview.chromium.org/2109633002/diff/60001/content/test/data/navig...
content/test/data/navigation_handle_https_upgrade_test.html:3:
<title>NavigationHandleImpl HTTPS upgrade test</title>
On 2016/07/04 11:22:09, clamy wrote:
> How about just naming the file HTTPS upgrade (same with the name). This way
it's
> easily reusable in other tests.

Done.

https://codereview.chromium.org/2109633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2109633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:679: if
(resourceRequest.httpHeaderField("Upgrade-Insecure-Requests") ==
AtomicString("1"))
On 2016/07/04 09:31:43, Mike West wrote:
> Can you add a test which verifies this behavior? Could be as simple as calling
> the method twice on a request and verifying that the output is sane.

Done.

[clamy, 2016-07-05 11:36:37]

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:289: const
std::vector<GURL>& GetStartedUrls() const { return start_urls; }
s/GetStartedUrls/start_urls()

I would also suggest to change the member name to urls_ and the this method
should be called urls()

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:292:
std::vector<GURL> start_urls;
member variables need to end with an _.

[carlosk, 2016-07-05 12:04:01]

Thanks.

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:289: const
std::vector<GURL>& GetStartedUrls() const { return start_urls; }
On 2016/07/05 11:36:37, clamy wrote:
> s/GetStartedUrls/start_urls()
> 
> I would also suggest to change the member name to urls_ and the this method
> should be called urls()

Done.

https://codereview.chromium.org/2109633002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:292:
std::vector<GURL> start_urls;
On 2016/07/05 11:36:37, clamy wrote:
> member variables need to end with an _.

Done.

[Mike West, 2016-07-06 06:55:35]

//third_party/WebKit LGTM.

//content LGTM2, though I'd like you to comment the port-modification if it's
indeed necessary. It shouldn't be.

https://codereview.chromium.org/2109633002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:723:
ASSERT_FALSE(cross_site_iframe_secure_url.has_port());
Nit: If this is necessary to make the test pass, please add a `// TODO(mkwst):
Upgrades shouldn't modify the port.` and file a bug. It surprises me that this
is the case, since we have blink tests that explicitly rely on the fact that we
don't modify the port (see
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
for example).

[carlosk, 2016-07-06 08:46:21]

Thanks. There's no issue with the port.

https://codereview.chromium.org/2109633002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2109633002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:723:
ASSERT_FALSE(cross_site_iframe_secure_url.has_port());
On 2016/07/06 06:55:35, Mike West wrote:
> Nit: If this is necessary to make the test pass, please add a `// TODO(mkwst):
> Upgrades shouldn't modify the port.` and file a bug. It surprises me that this
> is the case, since we have blink tests that explicitly rely on the fact that
we
> don't modify the port (see
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> for example).

I was wrong to place the blame in the upgrade code. The URL has no port from the
start being obtained directly and fully from the HTML code which is to be
expected.

For the same site case Chrome ads the server+port from the main document because
the HTML doesn't specify one hence the difference in behavior.

So I simplified the code and removed the ASSERT.

[clamy, 2016-07-07 12:31:35]

Thanks! Lgtm.

[carlosk, 2016-07-08 09:07:17]

On 2016/07/07 12:31:35, clamy wrote:
> Thanks! Lgtm.

As I got all files LGTM-ed and nasko@ is away I'm committing this as is.

nasko@: if you have any further comments let me know.

[carlosk, 2016-07-08 09:07:25]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-07-08 09:07:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2109633002/#ps140001
(title: "Simplified cross site URL code.")

[commit-bot: I haz the power, 2016-07-08 09:07:58]

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

[commit-bot: I haz the power, 2016-07-08 14:12:02]

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

[commit-bot: I haz the power, 2016-07-08 14:12:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[carlosk, 2016-07-08 15:47:20]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2016-07-08 15:47:51]

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

[commit-bot: I haz the power, 2016-07-08 18:28:13]

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

[commit-bot: I haz the power, 2016-07-08 18:28:14]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[carlosk, 2016-07-09 17:11:34]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2016-07-09 17:11:40]

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

[commit-bot: I haz the power, 2016-07-09 19:36:15]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-09 19:38:00]

Description was changed from

==========
Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

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

to

==========
Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

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

Committed: https://crrev.com/2ece5a75956f9df5c0a78939a8f06d4e09b6e860
Cr-Commit-Position: refs/heads/master@{#404567}
==========

[commit-bot: I haz the power, 2016-07-09 19:38:02]

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