PlzNavigate: sanitize the referrer in NavigationRequest

PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2764363002
Cr-Commit-Position: refs/heads/master@{#458806}
Committed: https://chromium.googlesource.com/chromium/src/+/a6d343f911171278376b4509f7da9d90592b03ee

[clamy, 2017-03-22 16:55:55]

Description was changed from

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
==========

to

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2017-03-22 16:56:01]

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

[commit-bot: I haz the power, 2017-03-22 16:56:16]

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

[clamy, 2017-03-22 16:56:34]

Description was changed from

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2017-03-22 16:56:34]

clamy@chromium.org changed reviewers:
+ jam@chromium.org, nasko@chromium.org

[clamy, 2017-03-22 16:57:12]

@nasko, jam: PTAL
This is the fix for the Referrer issue we're seeing on Canary.

[nasko, 2017-03-22 17:11:12]

LGTM with some nits.

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:311:
CHECK(root->navigation_request());
nit: ASSERT_TRUE

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:320: CHECK(entry);
nit: ASSERT_TRUE

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:321:
EXPECT_EQ(kInsecureUrl, entry->GetURL());
nit: You could just call WebContents::GetLastCommittedURL(), without having
through NavigationController and want to save the above two lines of code.

[clamy, 2017-03-22 17:18:58]

Thanks!

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:311:
CHECK(root->navigation_request());
On 2017/03/22 17:11:12, nasko (out) wrote:
> nit: ASSERT_TRUE

Done.

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:320: CHECK(entry);
On 2017/03/22 17:11:12, nasko (out) wrote:
> nit: ASSERT_TRUE

Done.

https://codereview.chromium.org/2764363002/diff/1/content/browser/browser_sid...
content/browser/browser_side_navigation_browsertest.cc:321:
EXPECT_EQ(kInsecureUrl, entry->GetURL());
On 2017/03/22 17:11:12, nasko (out) wrote:
> nit: You could just call WebContents::GetLastCommittedURL(), without having
> through NavigationController and want to save the above two lines of code.

Done.

[clamy, 2017-03-22 17:19:04]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-03-22 17:19:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2764363002/#ps20001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-03-22 17:19:24]

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, 2017-03-22 18:20:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490203144085070,
"parent_rev": "1203b185b7da6a3e2e879c72ad0c340e36b34120", "commit_rev":
"a6d343f911171278376b4509f7da9d90592b03ee"}

[commit-bot: I haz the power, 2017-03-22 18:21:25]

Description was changed from

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: sanitize the referrer in NavigationRequest

This CL ensures that the referrer in CommonNavigationParams is properly
sanitized for navigation requests when PlzNavigate is enabled.

BUG=659089
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2764363002
Cr-Commit-Position: refs/heads/master@{#458806}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6d343f911171278376b4509f7da...
==========

[commit-bot: I haz the power, 2017-03-22 18:21:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a6d343f911171278376b4509f7da...

[wychen, 2017-03-22 21:18:05]

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

[wychen, 2017-03-22 21:18:05]

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

https://codereview.chromium.org/2764363002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:373:
Referrer::SanitizeForRequest(common_params_.url,
Do we need to sanitize it here?

[jam, 2017-03-23 23:35:17]

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

https://codereview.chromium.org/2764363002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:373:
Referrer::SanitizeForRequest(common_params_.url,
On 2017/03/22 21:18:05, wychen wrote:
> Do we need to sanitize it here?

This is done in the constructor.

[wychen, 2017-03-23 23:48:03]

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

https://codereview.chromium.org/2764363002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:373:
Referrer::SanitizeForRequest(common_params_.url,
On 2017/03/23 23:35:17, jam wrote:
> On 2017/03/22 21:18:05, wychen wrote:
> > Do we need to sanitize it here?
> 
> This is done in the constructor.

This is exactly what I thought. So we can directly use common_params_.referrer,
rather than calling SanitizeForRequest() again here, right?