PlzNavigate: Fix wrong SourceLocation.

PlzNavigate: Fix wrong SourceLocation.

Before this CL, when an iframe is asked to navigate, the SourceLocation
used was the wrong one. It was always the one of the current document, even
when the navigation was initiated by another frame.

TEST=http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html

The test still does not pass however. It is due to
https://crbug.com/718942 which is not related to this issue.

BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

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

[arthursonzogni, 2017-05-24 15:07:44]

Patchset #1 (id:1) has been deleted

[arthursonzogni, 2017-05-24 15:10:42]

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

[commit-bot: I haz the power, 2017-05-24 15:11:03]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-24 16:18:05]

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

[arthursonzogni, 2017-05-24 16:18:06]

Hi Mike,

I am trying to fix http://crbug.com/690946. For this test, I found that the
SourceLocation used was the one of the current document instead of its parent.
This CL should fix the problem. Please take a look.

+CC nasko. Since I will be OOO until Monday(02-29-17), feel free to take this CL
(or checking the CQ if there was no issues) if you want it to be landed early.

[commit-bot: I haz the power, 2017-05-24 17:41:36]

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

[commit-bot: I haz the power, 2017-05-24 17:41:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-05-24 20:39:51]

Hmm, it looks like there is some test failure...
SourceLocation are used on the browser-side for CSP and mixed content. There is
problems with mixed content.
At first glance, it seems that with PlzNavigate, the error messages have a
SourceLocation and it was not expected. Maybe it is an improvement, I haven't
investigate a lot.

[alexmos, 2017-05-25 02:56:58]

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

[alexmos, 2017-05-25 02:56:58]

Nasko forwarded me a link to this CL, and I took a look at mixed content
failures since Arthur will be gone until Monday.  I think what's going on here
is that we're indeed "improving" things by including the right line number in
mixed content warnings.  In fact, these failing layout tests used to have a line
number which was removed as a compromise when estark refactored mixed content
for OOPIFs in https://codereview.chromium.org/1550723003.  However, thinking
about this more, it might not actually be safe to include that source location
here, for the same reason as issue 718942: if we've got
https://foo.com/sensitive.html, which has a cross-process mixed-content subframe
http://evil.com, we shouldn't send the full "https://foo.com/sensitive.html" URL
or the source location that initiated the navigation in foo.com to to evil.com's
renderer.  Yet IIUC this is what mixed_content_navigation_throttle currently
does for main_resource_url, and will do for source location with this CL.  I
filed https://crbug.com/726178 for this.

I guess the question is how we want to treat mixed content warnings involving
two cross-origin frames.  Today, we print the origin of the "mixed frame" if
it's remote, and the full URL if it's local - see MainResourceUrlForFrame.  One
option which would fix this issue and also simplify our lives for
--site-per-process is to just always use the mixed frame's origin, even if it's
local, which is something that estark@ also proposed in
https://codereview.chromium.org/1550723003#msg21.  I put together a draft CL to
give that a shot in https://codereview.chromium.org/2900423003 (though it might
need some more work on the non-PlzNav path).  Mike, WDYT about this whole
direction?  It does reduce the usefulness of those warnings, but to make things
a bit better we could only apply this to frame navigations, and leave alone
cases like https://foo.com itself embedding an insecure image.

[Mike West, 2017-05-26 11:47:58]

I think we should probably talk a bit about what y'all would like to achieve
with PlzNavigate. I'm totally behind the theory of not sending sensitive data
from one renderer to the other, but I'm a bit worried that we're going to neuter
the ability of developers to diagnose and fix errors if we take the strict
approach that might imply.

I'll put something on your calendar in the hopes of chatting sooner rather than
later.

[arthursonzogni, 2017-05-31 08:20:00]

Description was changed from

==========
PlzNavigate: Fix wrong SourceLocation.

Before this CL, when an iframe is asked to navigate, the SourceLocation
used was the wrong one. It was always the one of the current document, even
when the navigation was initiated by another frame.

TEST=http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html

The test still does not pass however. It is due to
https://crbug.com/718942 which is not related to this issue.

* Current behavior (without PlzNavigate):
CONSOLE ERROR: line 24: Refused to frame 'https://localhost:8443/security/[...]

* Before this CL (with PlzNavigate): (line number absent, url path absent)
CONSOLE ERROR: Refused to frame 'https://localhost:8443/

* After this CL (with PlzNavigate): (line number present, url path absent)
CONSOLE ERROR: line 24: Refused to frame 'https://localhost:8443/

BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix wrong SourceLocation.

Before this CL, when an iframe is asked to navigate, the SourceLocation
used was the wrong one. It was always the one of the current document, even
when the navigation was initiated by another frame.

TEST=http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html

The test still does not pass however. It is due to
https://crbug.com/718942 which is not related to this issue.

BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

[arthursonzogni, 2017-05-31 08:22:03]

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

[commit-bot: I haz the power, 2017-05-31 08:22:24]

Dry run: 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-05-31 10:51:39]

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

[commit-bot: I haz the power, 2017-05-31 10:51:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-05-31 11:35:39]

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

[arthursonzogni, 2017-05-31 11:35:56]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-31 11:36:00]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-31 11:36:02]

Patchset #2 (id:60001) has been deleted

[arthursonzogni, 2017-05-31 13:11:15]

I updated the FlagExpectations/enable-browser-side-navigation file for now.
The added mixed content tests are failing because of https://crbug.com/726178.
We realize that now because, before this CL, the SourceLocation provided was the
wrong one.

Next step will be (if possible) to route directly console messages to the
devtools process, such that there won't be URLs leaks anymore.

[commit-bot: I haz the power, 2017-05-31 13:46:08]

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

[commit-bot: I haz the power, 2017-05-31 13:46:09]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-06-01 15:36:12]

lgtm

[arthursonzogni, 2017-06-01 15:41:45]

On 2017/06/01 15:36:12, Mike West wrote:
> lgtm

Thanks Mike and Alex!

[arthursonzogni, 2017-06-01 15:41:49]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-06-01 15:42:33]

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-06-01 17:54:50]

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

[commit-bot: I haz the power, 2017-06-01 17:54:51]

Failed to apply patch for
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:20
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation'
with conflicts.
  U
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

Patch:      
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
Index:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
diff --git
a/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
b/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
index
791d316b04a739ba5a35ac84f94d6a0534c73fae..c7acda23f6c68a58bf8994c07998dd8997780e6d
100644
---
a/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
+++
b/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
@@ -5,11 +5,6 @@
 crbug.com/551000 http/tests/inspector/network/x-frame-options-deny.html [
Failure ]
 crbug.com/551000
virtual/mojo-loading/http/tests/inspector/network/x-frame-options-deny.html [
Failure ]
 
-# https://crbug.com/690946 PlzNavigate: Missing line number in console messages
when the 'frame-src' CSP is infringed.
-# SourceLocation is not set when the navigation is initiated outside of a
script.
-crbug.com/690946
http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html [
Failure ]
-crbug.com/690946
virtual/mojo-loading/http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html
[ Failure ]
-
 # These tests are flaky.
 Bug(none) http/tests/security/popup-allowed-by-sandbox-can-navigate.html [
Failure ]
 Bug(none)
virtual/mojo-loading/http/tests/security/popup-allowed-by-sandbox-can-navigate.html
[ Failure ]
@@ -20,11 +15,38 @@ Bug(none)
virtual/mojo-loading/http/tests/security/popup-allowed-by-sandbox-can-
 Bug(718942) http/tests/misc/onload-detach-during-csp-frame-src-none.html [
Failure ]
 Bug(718942)
http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html
[ Failure ]
 Bug(718942)
http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html
[ Failure ]
+Bug(718942)
http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html [
Failure ]
 Bug(718942)
virtual/mojo-loading/http/tests/misc/onload-detach-during-csp-frame-src-none.html
[ Failure ]
 Bug(718942)
virtual/mojo-loading/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html
[ Failure ]
 Bug(718942)
virtual/mojo-loading/http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html
[ Failure ]
+Bug(718942)
virtual/mojo-loading/http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html
[ Failure ]
 Bug(718942)
virtual/off-main-thread-fetch/http/tests/misc/onload-detach-during-csp-frame-src-none.html
[ Failure ]
 
+# Console error message source location is present with PlzNavigate. It means
+# that URLs are potentially disclosed across cross-origin renderers.
+# See https://crbug.com/726178.
+# One solution would be to send console messages from the browser to the
+# devtool process without the round-trip in the renderer.
+# See https://crbug.com/721329
+Bug(726178)
http/tests/security/mixedContent/insecure-frame-in-data-iframe-in-main-frame-blocked.html
[ Failure ]
+Bug(726178) http/tests/security/mixedContent/insecure-iframe-in-iframe.html [
Failure ]
+Bug(726178)
http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html [
Failure ]
+Bug(726178)
http/tests/security/mixedContent/insecure-iframe-in-main-frame-blocked.html [
Failure ]
+Bug(726178) http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
[ Failure ]
+Bug(726178)
http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html [
Failure ]
+Bug(726178)
http/tests/security/mixedContent/redirect-http-to-https-iframe-in-main-frame.html
[ Failure ]
+Bug(726178)
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/insecure-frame-in-data-iframe-in-main-frame-blocked.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-iframe.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-main-frame-blocked.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/redirect-http-to-https-iframe-in-main-frame.html
[ Failure ]
+Bug(726178)
virtual/mojo-loading/http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
[ Failure ]
+
+
+
 # PlzNavigate: A few line are missing in InspectorTest.dumpServiceWorkersView()
 Bug(725818) http/tests/inspector/service-workers/service-workers-redundant.html
[ Failure ]
 Bug(725818)
virtual/off-main-thread-fetch/http/tests/inspector/service-workers/service-workers-redundant.html
[ Failure ]

[arthursonzogni, 2017-06-02 08:31:22]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-06-02 08:31:23]

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/2901223003/#ps100001
(title: "Rebase + Fix conflicts.")

[commit-bot: I haz the power, 2017-06-02 08:31:29]

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-06-02 10:40:30]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496392282452180,
"parent_rev": "017ba8c602a378a6f508cf642ef81ed46eea5dbe", "commit_rev":
"ccfe0b5a32bc0ed36a95eb1da8e7f4b03268a661"}

[commit-bot: I haz the power, 2017-06-02 10:40:43]

Description was changed from

==========
PlzNavigate: Fix wrong SourceLocation.

Before this CL, when an iframe is asked to navigate, the SourceLocation
used was the wrong one. It was always the one of the current document, even
when the navigation was initiated by another frame.

TEST=http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html

The test still does not pass however. It is due to
https://crbug.com/718942 which is not related to this issue.

BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix wrong SourceLocation.

Before this CL, when an iframe is asked to navigate, the SourceLocation
used was the wrong one. It was always the one of the current document, even
when the navigation was initiated by another frame.

TEST=http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html

The test still does not pass however. It is due to
https://crbug.com/718942 which is not related to this issue.

BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

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

[commit-bot: I haz the power, 2017-06-02 10:40:44]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ccfe0b5a32bc0ed36a95eb1da8e7...