Issue 1905033002: PlzNavigate: Move navigation-level mixed content checks to the browser.

carlosk

Description was changed from ========== Move navigation-level mixed-content checks to the browser. This is the ...

4 years, 8 months ago (2016-04-21 16:16:59 UTC) #1

carlosk

Description was changed from ========== Move navigation-level mixed-content checks to the browser. This is the ...

4 years, 8 months ago (2016-04-21 16:19:26 UTC) #2

carlosk

Description was changed from ========== WIP: Move navigation-level mixed-content checks to the browser. This is ...

4 years, 8 months ago (2016-04-21 16:20:56 UTC) #3

carlosk

Description was changed from ========== WIP: Move navigation-level mixed-content checks to the browser. This is ...

4 years, 6 months ago (2016-06-06 12:36:38 UTC) #7

carlosk

Description was changed from ========== WIP: Move navigation-level mixed content checks to the browser. This ...

4 years, 6 months ago (2016-06-06 12:37:13 UTC) #8

carlosk

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

4 years, 5 months ago (2016-07-18 14:37:10 UTC) #9

carlosk

mkwst@, nasko@, clamy@: PTAL. Pay special attention to my comments in this patch set with ...

4 years, 5 months ago (2016-07-18 14:37:14 UTC) #10

carlosk

Description was changed from ========== WIP: Move navigation-level mixed content checks to the browser. This ...

4 years, 5 months ago (2016-07-18 14:37:32 UTC) #11

carlosk

Description was changed from ========== Move navigation-level mixed content checks to the browser. This is ...

4 years, 5 months ago (2016-07-18 14:37:42 UTC) #12

carlosk

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

4 years, 5 months ago (2016-07-18 14:49:15 UTC) #13

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-18 14:49:46 UTC) #14

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-18 14:49:48 UTC) #15

clamy

Thanks! I can't comment on the mixed content-specifics, but one remark below. On the subject ...

4 years, 5 months ago (2016-07-18 15:20:38 UTC) #16

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-18 16:58:05 UTC) #17

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/246528)

4 years, 5 months ago (2016-07-18 16:58:06 UTC) #18

carlosk

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

4 years, 5 months ago (2016-07-19 13:04:33 UTC) #19

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-19 13:05:01 UTC) #20

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-19 13:05:03 UTC) #21

carlosk

Thanks clamy@. For the record this was the layout test that failed with the previous ...

4 years, 5 months ago (2016-07-19 13:31:01 UTC) #22

carlosk

On the reportingStatus issue I mentioned in a comment: The trybots on https://crrev.com/2160583003 just finished ...

4 years, 5 months ago (2016-07-19 14:07:07 UTC) #23

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-19 14:32:35 UTC) #24

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 5 months ago (2016-07-19 14:32:37 UTC) #25

Mike West

This looks like it's going in the right direction. I added a few comments on ...

4 years, 5 months ago (2016-07-19 14:39:32 UTC) #26

Mike West

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc File content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc#newcode49 content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:49: } I think we should have more extensive tests ...

4 years, 5 months ago (2016-07-19 14:41:39 UTC) #27

carlosk

Thanks! But mkwst@, did you have a chance to read my comments/questions from the original ...

4 years, 5 months ago (2016-07-19 16:32:16 UTC) #28

Thanks!

But mkwst@, did you have a chance to read my comments/questions from the
original request for reviews (patch set #20)?

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:34: // renderer
code? See http://crbug.com/627502.
On 2016/07/19 14:39:31, Mike West wrote:
> Super very important nit: http_s_.

Done. But why? Fear of MITM attacks?

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:41:
url.SchemeIs(kChromeUIScheme);
On 2016/07/19 14:39:32, Mike West wrote:
> As you note, this isn't pretty. It also doesn't seem to include
> `chrome-extension` or `chrome-extension-resource`, which are registered in
> `//extensions/renderer/dispatcher.cc` (which, due to the excitement of
layering,
> you don't even have access to here). I think we're going to need to be a bit
> more clever about how we define this list in an extensible way. Perhaps
setting
> up a registry in //content directly that passes through to
//third_party/WebKit
> is the right short-term thing to do, but I don't think you're going to be able
> to hard-code an exhaustive list without layering violations.

Acknowledged. Then we indeed need to solve http://crbug.com/627502 before
landing this.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:49: //
HasPotentiallySecureScheme applies here. See http://crbug.com/627502.
On 2016/07/19 14:39:31, Mike West wrote:
> S.

Done.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:57:
url.SchemeIs("chrome-native");
On 2016/07/19 14:39:32, Mike West wrote:
> Ditto. I'm not an OWNER of this code, but reaching up into the //chrome layer
> for a string seems pretty ugly.

Agreed.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:66: // (in
origin_util.cc). See http://crbug.com/629059.
On 2016/07/19 14:39:32, Mike West wrote:
> S.

Done.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:89: }
On 2016/07/19 14:39:32, Mike West wrote:
> Should be gone by the time you land this (if I can get the relevant tests
> working).

Acknowledged.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:104: //
http://crbug.com/627502.
On 2016/07/19 14:39:31, Mike West wrote:
> S.

Done.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:153:
tree_node->current_frame_host()->AddMessageToConsole(messageLevel, message);
On 2016/07/19 14:39:32, Mike West wrote:
> s/tree_node/mixed_content_node/ (or `tree_node->parent()`?), right? Because we
> might never commit this navigation in the current node.

That makes sense but MixedContentChecker disagrees:

183: bool MixedContentChecker::shouldBlockFetch(LocalFrame* frame, ...
263:         logToConsoleAboutFetch(frame, ...

125: void MixedContentChecker::logToConsoleAboutFetch(LocalFrame* frame, ...
275:     frame->document()->addConsoleMessage(...

WDYT?

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:181:
node->current_frame_host()->render_view_host()->GetWebkitPreferences();
On 2016/07/19 14:39:32, Mike West wrote:
> The mixed content prefs aren't renderer-specific, but CLI-based, aren't they?

Some come from CLI, some from the renderer/Blink and currently a couple are
controlled field trials.

They seem to be renderer specific but the fact that they are stored at the
RenderViewHost level makes it so that they can be different for different
RenderViews, even if they live in the same renderer.

> Perhaps you could pass them in when you create the throttle, rather than
reading
> them for each request?

I could store them but note that a new instance of this class is created for
each request anyway.

But maybe one thing I can do -- in a small separate patch -- is to add a cont
version of this getter that returns a reference to it instead of a copy. I did
find a few other calls that would benefit from this too. WDYT?

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:231:
"ContentSettings.MixedScript.RanMixedScript", origin_url);
On 2016/07/19 14:39:31, Mike West wrote:
> I wonder if anyone is looking at these metrics...

Don't we all?

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:280: //
obtaining the "origin of the security context"? http://crbug.com/623486
On 2016/07/19 14:39:32, Mike West wrote:
> S.

Done.

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/1905033002/diff/480001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:49: }
On 2016/07/19 14:41:39, Mike West wrote:
> I think we should have more extensive tests here that verify the result of the
> various throttle bits. The layout tests conform the broad behavior, but unit
> tests are super-helpful for verification in the small.

Noted!

But layout tests are not be enough anymore. Given the settings are now stored in
Chrome-land and that no Chrome code exist in layout tests we will need to add
new mixed content browser tests to cover those.

https://codereview.chromium.org/1905033002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:185: //
Frame-level loads are checked by the browser. No need to check them again here.
On 2016/07/19 14:39:32, Mike West wrote:
> Can you add an assertion that we're not accidentally allowing mixed content?
> Something like the following might work?
> 
> ```
> if (frameType != WebURLRequest::FrameTypeNone) {
>   ASSERT_EQ(nullptr,
> inWhichFrameIsContentMixed(effectiveFrameForFrameType(frame, frameType),
> frameType, url));
>   return false;
> }
> ```

Right now I can't because in some cases -- renderer initiated navigations --
this call happens before the request arrives at the browser and that assert
would be triggered.

With PlzNavigate the situation might be different though.

https://codereview.chromium.org/1905033002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1905033002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.h:56: //
https://w3c.github.io/webappsec-mixed-content/
On 2016/07/19 14:39:32, Mike West wrote:
> Can you file a bug to move whatever else we can up and out of Blink? This
seems
> like it's going to be a maintenance burden, and it would be a good idea to
start
> planning for it now.

Sub resource loads are more complicated to handle in the browser. From a quick
talk to clamy@ that at least adds one extra IPC round trip to the browser per
resource request. Alternatively it would require adding the so much avoided
request-to-tree-node mapping to the IO thread.

This is something that should be evaluated anyway so I filed:
https://bugs.chromium.org/p/chromium/issues/detail?id=629528

carlosk

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

4 years, 5 months ago (2016-07-25 18:29:57 UTC) #29

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-25 18:30:34 UTC) #30

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-25 18:30:38 UTC) #31

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-25 19:32:34 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/250409)

4 years, 5 months ago (2016-07-25 19:32:36 UTC) #33

carlosk

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

4 years, 4 months ago (2016-07-26 12:34:07 UTC) #34

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 12:34:19 UTC) #35

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 4 months ago (2016-07-26 12:34:21 UTC) #36

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 12:36:29 UTC) #37

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/41444) ios-simulator on ...

4 years, 4 months ago (2016-07-26 12:36:30 UTC) #38

carlosk

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

4 years, 4 months ago (2016-07-26 13:45:58 UTC) #39

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 13:46:05 UTC) #40

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 4 months ago (2016-07-26 13:46:09 UTC) #41

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 15:12:17 UTC) #42

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-26 15:12:18 UTC) #43

carlosk

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

4 years, 4 months ago (2016-08-18 22:52:13 UTC) #44

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 22:53:28 UTC) #45

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 22:53:29 UTC) #46

commit-bot: I haz the power

Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying ...

4 years, 4 months ago (2016-08-18 22:53:30 UTC) #47

carlosk

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

4 years, 4 months ago (2016-08-18 23:12:19 UTC) #48

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 23:13:24 UTC) #49

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 23:13:25 UTC) #50

commit-bot: I haz the power

Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying ...

4 years, 4 months ago (2016-08-18 23:13:26 UTC) #51

carlosk

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

4 years, 4 months ago (2016-08-18 23:23:54 UTC) #52

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 23:25:00 UTC) #53

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-18 23:25:02 UTC) #54

commit-bot: I haz the power

Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying ...

4 years, 4 months ago (2016-08-18 23:25:03 UTC) #55

carlosk

Description was changed from ========== Move navigation-level mixed content checks to the browser. This is ...

4 years, 4 months ago (2016-08-18 23:46:35 UTC) #56

carlosk

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

4 years, 4 months ago (2016-08-19 00:25:21 UTC) #58

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-19 00:25:54 UTC) #59

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-19 04:00:49 UTC) #60

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-19 04:00:50 UTC) #61

carlosk

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

4 years, 1 month ago (2016-11-16 19:29:06 UTC) #62

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-16 19:30:44 UTC) #63

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-16 19:35:26 UTC) #64

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/106975) mac_chromium_compile_dbg_ng on ...

4 years, 1 month ago (2016-11-16 19:35:28 UTC) #65

carlosk

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

4 years, 1 month ago (2016-11-17 01:58:21 UTC) #66

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-17 01:59:36 UTC) #67

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-17 02:36:43 UTC) #68

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/337256)

4 years, 1 month ago (2016-11-17 02:36:44 UTC) #69

carlosk

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

4 years, 1 month ago (2016-11-23 02:02:56 UTC) #70

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-23 02:03:31 UTC) #71

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-23 02:06:47 UTC) #72

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/311352) mac_chromium_rel_ng on ...

4 years, 1 month ago (2016-11-23 02:06:48 UTC) #73

carlosk

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

4 years ago (2016-11-23 17:58:03 UTC) #74

commit-bot: I haz the power

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

4 years ago (2016-11-23 17:58:43 UTC) #75

commit-bot: I haz the power

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

4 years ago (2016-11-23 18:48:13 UTC) #76

commit-bot: I haz the power

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

4 years ago (2016-11-23 18:48:15 UTC) #77

carlosk

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

4 years ago (2016-11-23 22:07:00 UTC) #78

commit-bot: I haz the power

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

4 years ago (2016-11-23 22:08:03 UTC) #79

commit-bot: I haz the power

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

4 years ago (2016-11-24 00:10:03 UTC) #80

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) ...

4 years ago (2016-11-24 00:10:05 UTC) #81

carlosk

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

4 years ago (2016-12-14 03:34:50 UTC) #82

commit-bot: I haz the power

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

4 years ago (2016-12-14 03:35:13 UTC) #83

commit-bot: I haz the power

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

4 years ago (2016-12-14 06:55:41 UTC) #84

carlosk

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

4 years ago (2016-12-14 23:16:19 UTC) #86

commit-bot: I haz the power

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

4 years ago (2016-12-14 23:17:09 UTC) #87

commit-bot: I haz the power

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

4 years ago (2016-12-15 01:29:35 UTC) #88

carlosk

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

4 years ago (2016-12-21 01:26:13 UTC) #90

commit-bot: I haz the power

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

4 years ago (2016-12-21 01:26:49 UTC) #91

commit-bot: I haz the power

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

4 years ago (2016-12-21 03:42:00 UTC) #92

jam

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode128 content/browser/frame_host/mixed_content_navigation_throttle.cc:128: return blink::WebMixedContent::requestContextName( similarly here, can we just send an ...

3 years, 11 months ago (2016-12-28 20:41:00 UTC) #95

carlosk

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

3 years, 11 months ago (2016-12-30 03:31:10 UTC) #96

commit-bot: I haz the power

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

3 years, 11 months ago (2016-12-30 03:31:19 UTC) #97

commit-bot: I haz the power

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

3 years, 11 months ago (2016-12-30 05:34:25 UTC) #98

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2016-12-30 05:34:26 UTC) #99

carlosk

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

3 years, 11 months ago (2017-01-05 06:06:38 UTC) #100

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-05 06:07:02 UTC) #101

carlosk

Thanks for the comments. The annoying part about those calls is that this code was ...

3 years, 11 months ago (2017-01-05 06:10:08 UTC) #102

Thanks for the comments.

The annoying part about those calls is that this code was moved into
WebMixedContent in Blink's public folder specifically to be accessible from the
new mixed content code in the browser. This landed in master long ago when
WebMixedContent was introduced. Will it be necessary to revert that?

Note that my reply to one of the comments below might affect this matter too.

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:128: return
blink::WebMixedContent::requestContextName(
On 2016/12/28 20:41:00, jam wrote:
> similarly here, can we just send an IPC to the renderer to log this? It could
> then call that wekbit method.

Done.

I split FrameMsg_MixedContentUpdate IPC in two: one for Blink feature reporting
and one for when mixed content is found. As logging only happens when mixed
content is found the handling of the latter on the renderer now also does the
logging.

Beyond avoiding the Blink call from content/browser this was also an improvement
because the new IPCs are simpler and clearer, and because the log function is
not duplicated anymore.

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:137: return
blink::WebMixedContent::contextTypeFromRequestContext(
On 2016/12/28 20:41:00, jam wrote:
> we only include enums/POD from webkit in the browser process (see comment in
> content/browser/DEPS)
> 
> Can we send the mixed context blocking type from the renderer along with the
> other request data in the IPC? This would handle all the request types from
the
> renderer, other than the browser-initiated navigations with plznavigate
> (WebURLRequest::RequestContextLocation I believe). For
> WebURLRequest::RequestContextLocation we can just hard code that it's
blockable.

This proved a tad more complicated than I assumed. I'll try to explain it below
otherwise a VC might be in order.

The values of WebMixedContent::ContextType is derived from
WebURLRequest::RequestContext but it needs to be "fixed" for the plugin case
based on the setting for "strictMixedContentCheckingForPlugin", which is stored
at the render-view level.

I initially considered adding the new member to ResourceRequest and deriving its
value in WebURLLoaderImpl::Context::Start where
ResourceRequest::fetch_request_context_type is set but it doesn't seem like I
have access to the render-view there (it lives in content/child).

So I'm considering these alternatives:

a) Move the implementation of WebMixedContent::contextTypeFromRequestContext
into content/common (or content/public/common) and make the former call into
that. This would allow the browser to call it too.

b) Derive the WebMixedContent::ContextType value without the
strict-plugin-checking information in WebURLLoaderImpl::Context::Start and then
patch it when the mixed content check happens on the browser.

IMO a) seems better: clearer and keeps the "conversion" logic in one single
place. However I'm not sure of the best/correct way to make Blink call into a
global function defined in content/ (or maybe a static class function).

Comments on these suggestions as well as new ideas are both welcome.

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-05 06:24:34 UTC) #103

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/259267)

3 years, 11 months ago (2017-01-05 06:24:35 UTC) #104

jam

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode137 content/browser/frame_host/mixed_content_navigation_throttle.cc:137: return blink::WebMixedContent::contextTypeFromRequestContext( On 2017/01/05 06:10:08, carlosk wrote: > On ...

3 years, 11 months ago (2017-01-05 17:21:53 UTC) #105

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:137: return
blink::WebMixedContent::contextTypeFromRequestContext(
On 2017/01/05 06:10:08, carlosk wrote:
> On 2016/12/28 20:41:00, jam wrote:
> > we only include enums/POD from webkit in the browser process (see comment in
> > content/browser/DEPS)
> > 
> > Can we send the mixed context blocking type from the renderer along with the
> > other request data in the IPC? This would handle all the request types from
> the
> > renderer, other than the browser-initiated navigations with plznavigate
> > (WebURLRequest::RequestContextLocation I believe). For
> > WebURLRequest::RequestContextLocation we can just hard code that it's
> blockable.
> 
> This proved a tad more complicated than I assumed. I'll try to explain it
below
> otherwise a VC might be in order.
> 
> The values of WebMixedContent::ContextType is derived from
> WebURLRequest::RequestContext but it needs to be "fixed" for the plugin case
> based on the setting for "strictMixedContentCheckingForPlugin", which is
stored
> at the render-view level.
> 
> I initially considered adding the new member to ResourceRequest and deriving
its
> value in WebURLLoaderImpl::Context::Start where
> ResourceRequest::fetch_request_context_type is set but it doesn't seem like I
> have access to the render-view there (it lives in content/child).

The plugin setting is available in the renderer via WebPreferences.
RenderFrameImpl::willSendRequest can store that boolean on
content::RequestExtraData which can then be read by WebURLLoaderImpl. WDYT?
> 
> So I'm considering these alternatives:
> 
> a) Move the implementation of WebMixedContent::contextTypeFromRequestContext
> into content/common (or content/public/common) and make the former call into
> that. This would allow the browser to call it too.

But this is called by blink right? Blink can't call into content, since that's a
layering violation (i.e. it would be a circular dependency for content to call
blink and vice versa).
> 
> b) Derive the WebMixedContent::ContextType value without the
> strict-plugin-checking information in WebURLLoaderImpl::Context::Start and
then
> patch it when the mixed content check happens on the browser.
> 
> IMO a) seems better: clearer and keeps the "conversion" logic in one single
> place. However I'm not sure of the best/correct way to make Blink call into a
> global function defined in content/ (or maybe a static class function).
> 
> Comments on these suggestions as well as new ideas are both welcome.

carlosk

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

3 years, 11 months ago (2017-01-06 03:58:50 UTC) #106

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 03:59:02 UTC) #107

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 04:01:16 UTC) #108

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/131204) ios-device-xcode-clang on ...

3 years, 11 months ago (2017-01-06 04:01:19 UTC) #109

carlosk

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

3 years, 11 months ago (2017-01-06 04:14:45 UTC) #110

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 04:15:05 UTC) #111

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 04:17:23 UTC) #112

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/131210) ios-device-xcode-clang on ...

3 years, 11 months ago (2017-01-06 04:17:25 UTC) #113

jam

https://codereview.chromium.org/1905033002/diff/880001/third_party/WebKit/public/platform/WebMixedContent.h File third_party/WebKit/public/platform/WebMixedContent.h (right): https://codereview.chromium.org/1905033002/diff/880001/third_party/WebKit/public/platform/WebMixedContent.h#newcode48 third_party/WebKit/public/platform/WebMixedContent.h:48: BLINK_PLATFORM_EXPORT static ContextType contextTypeFromRequestContext( we should separate this static ...

3 years, 11 months ago (2017-01-06 04:49:40 UTC) #115

jam

https://codereview.chromium.org/1905033002/diff/880001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/880001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode93 content/browser/frame_host/mixed_content_navigation_throttle.cc:93: if (HasLocalScheme(url) || net::IsLocalhost(url.HostNoBrackets())) for clarity, it would help ...

3 years, 11 months ago (2017-01-06 21:20:13 UTC) #116

carlosk

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

3 years, 11 months ago (2017-01-06 21:32:35 UTC) #117

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 21:32:54 UTC) #118

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 23:11:30 UTC) #119

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-06 23:11:32 UTC) #120

carlosk

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

3 years, 11 months ago (2017-01-07 01:48:44 UTC) #121

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-07 01:48:58 UTC) #122

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-07 02:01:51 UTC) #123

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/189495)

3 years, 11 months ago (2017-01-07 02:01:53 UTC) #124

carlosk

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

3 years, 11 months ago (2017-01-07 18:51:44 UTC) #125

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-07 18:51:51 UTC) #126

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-07 21:24:46 UTC) #128

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-07 21:24:48 UTC) #129

carlosk

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

3 years, 11 months ago (2017-01-08 02:25:36 UTC) #130

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-08 02:25:43 UTC) #132

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-08 04:00:07 UTC) #133

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-08 04:00:08 UTC) #134

carlosk

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

3 years, 11 months ago (2017-01-09 19:40:52 UTC) #136

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-09 19:41:23 UTC) #137

jam

one last round of minor comments after looking at the latest patchset. https://codereview.chromium.org/1905033002/diff/970001/chrome/browser/content_settings/mixed_content_settings.h File chrome/browser/content_settings/mixed_content_settings.h ...

3 years, 11 months ago (2017-01-09 21:15:47 UTC) #138

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-09 23:37:19 UTC) #139

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-09 23:37:21 UTC) #140

carlosk

Thanks. Just wanted to publish my replies to the previous comments before moving on with ...

3 years, 11 months ago (2017-01-10 02:10:44 UTC) #141

Thanks. Just wanted to publish my replies to the previous comments before moving
on with to the last batch.

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/780001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:137: return
blink::WebMixedContent::contextTypeFromRequestContext(
On 2017/01/05 17:21:53, jam wrote:
> On 2017/01/05 06:10:08, carlosk wrote:
> > On 2016/12/28 20:41:00, jam wrote:
> > > we only include enums/POD from webkit in the browser process (see comment
in
> > > content/browser/DEPS)
> > > 
> > > Can we send the mixed context blocking type from the renderer along with
the
> > > other request data in the IPC? This would handle all the request types
from
> > the
> > > renderer, other than the browser-initiated navigations with plznavigate
> > > (WebURLRequest::RequestContextLocation I believe). For
> > > WebURLRequest::RequestContextLocation we can just hard code that it's
> > blockable.
> > 
> > This proved a tad more complicated than I assumed. I'll try to explain it
> below
> > otherwise a VC might be in order.
> > 
> > The values of WebMixedContent::ContextType is derived from
> > WebURLRequest::RequestContext but it needs to be "fixed" for the plugin case
> > based on the setting for "strictMixedContentCheckingForPlugin", which is
> stored
> > at the render-view level.
> > 
> > I initially considered adding the new member to ResourceRequest and deriving
> its
> > value in WebURLLoaderImpl::Context::Start where
> > ResourceRequest::fetch_request_context_type is set but it doesn't seem like
I
> > have access to the render-view there (it lives in content/child).
> 
> The plugin setting is available in the renderer via WebPreferences.
> RenderFrameImpl::willSendRequest can store that boolean on
> content::RequestExtraData which can then be read by WebURLLoaderImpl. WDYT?
> > 
> > So I'm considering these alternatives:
> > 
> > a) Move the implementation of WebMixedContent::contextTypeFromRequestContext
> > into content/common (or content/public/common) and make the former call into
> > that. This would allow the browser to call it too.
> 
> But this is called by blink right? Blink can't call into content, since that's
a
> layering violation (i.e. it would be a circular dependency for content to call
> blink and vice versa).
> > 
> > b) Derive the WebMixedContent::ContextType value without the
> > strict-plugin-checking information in WebURLLoaderImpl::Context::Start and
> then
> > patch it when the mixed content check happens on the browser.
> > 
> > IMO a) seems better: clearer and keeps the "conversion" logic in one single
> > place. However I'm not sure of the best/correct way to make Blink call into
a
> > global function defined in content/ (or maybe a static class function).
> > 
> > Comments on these suggestions as well as new ideas are both welcome.
> 

Done. The content::RequestExtraData idea worked.

https://codereview.chromium.org/1905033002/diff/880001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/880001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:93: if
(HasLocalScheme(url) || net::IsLocalhost(url.HostNoBrackets()))
On 2017/01/06 21:20:13, jam wrote:
> for clarity, it would help to split the HasLocalScheme into IsUnique() and
> IsLocal()
> 
> then IsLocal() would be easy to compare to SecurityOrigin::isLocal(). When I
try
> to compare them I see differences, i.e.
> SecurityOrigin::isLocal checks "file" (hardcoded from SchemeRegistry.cpp)
while
> here doesn't and also there's kContentScheme from
> chrome_content_renderer_client.cc and aw_content_renderer_client.cc. 

Done.

https://codereview.chromium.org/1905033002/diff/880001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.cc:121: //
RenderThreadImpl::RegisterSchemes.
On 2017/01/06 21:20:13, jam wrote:
> nit: also mention that extensions/renderer/dispatcher.cc also adds
> chrome-extension?

Done. I reorganized all hard coded scheme code and updated comments with file
references and other information.

https://codereview.chromium.org/1905033002/diff/880001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMixedContent.h (right):

https://codereview.chromium.org/1905033002/diff/880001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMixedContent.h:48: BLINK_PLATFORM_EXPORT
static ContextType contextTypeFromRequestContext(
On 2017/01/06 04:49:40, jam wrote:
> we should separate this static method into a different header than the enum,
so
> that the browser only includes the header with enum and no one accidentally
> changes it to call the method from the browser.

Done.

carlosk

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

3 years, 11 months ago (2017-01-10 06:40:40 UTC) #142

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-10 06:40:57 UTC) #143

carlosk

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

3 years, 11 months ago (2017-01-10 07:10:16 UTC) #144

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-10 07:10:34 UTC) #145

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-10 08:40:59 UTC) #147

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-10 08:41:01 UTC) #148

carlosk

I Forgot to hit send on my replies yesterday... :/ https://codereview.chromium.org/1905033002/diff/970001/chrome/browser/content_settings/mixed_content_settings.h File chrome/browser/content_settings/mixed_content_settings.h (right): https://codereview.chromium.org/1905033002/diff/970001/chrome/browser/content_settings/mixed_content_settings.h#newcode13 ...

3 years, 11 months ago (2017-01-10 19:13:11 UTC) #149

I Forgot to hit send on my replies yesterday... :/

https://codereview.chromium.org/1905033002/diff/970001/chrome/browser/content...
File chrome/browser/content_settings/mixed_content_settings.h (right):

https://codereview.chromium.org/1905033002/diff/970001/chrome/browser/content...
chrome/browser/content_settings/mixed_content_settings.h:13: class
NavigationHandle;
On 2017/01/09 21:15:46, jam wrote:
> nit: no need to forward declare a parameter to an overridden method

Done.

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
File content/browser/frame_host/DEPS (right):

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
content/browser/frame_host/DEPS:25: "mixed_content_navigation_throttle\.(cc|h)":
[
On 2017/01/09 21:15:46, jam wrote:
> nit: not needed anymore

Done.

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.h:51:
MixedContentFormsSubmitted = 441,
On 2017/01/09 21:15:46, jam wrote:
> It's unfortunate to copy these enums between webkit and content. I guess we're
> guaranteed that the values in blink never change?

The inertia for those values ever changing is very high. They are the mapping
from the enum in blink::UseCounter to the respective histogram entries tracking
feature usage. So, practically speaking, yes.

> One possible way to avoid this duplication is to create this enum in
> webkit/public (perhaps in WebMixedContentContextType.h ?) and use these values
> in content/browser and send them in the IPC. Then content/renderer can call a
> method in WebMixedContent.h that takes this enum and translates it into the
> internal webkit enum?
>
> also nit: use upper case per chromium style guide
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

IIUC this wouldn't avoid the duplication but it would make it possible to 1)
keep these values in Blink-land, 2) allow the conversion code to be Blink
internal and 3) even allow for static-checking them. But I'd still have to have
this selected copy of values from blink::UseCounter so that they can be in a
public Blink location.

re nit: It would as well help me avoid having to convert casing of all these
values. ;)

Did I understand you correctly?

https://codereview.chromium.org/1905033002/diff/970001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1905033002/diff/970001/content/common/DEPS#ne...
content/common/DEPS:26: "+third_party/WebKit/public/platform/WebMixedContent.h",
On 2017/01/09 21:15:46, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/1905033002/diff/970001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/common/frame_m...
content/common/frame_messages.h:50: #include
"third_party/WebKit/public/platform/WebMixedContent.h"
On 2017/01/09 21:15:46, jam wrote:
> nit: shouldn't be needed because the header that declares the struct should be
> including the file

Done.

https://codereview.chromium.org/1905033002/diff/970001/content/common/frame_m...
content/common/frame_messages.h:947:
IPC_MESSAGE_ROUTED5(FrameMsg_MixedContentFoundByTheBrowser,
On 2017/01/09 21:15:46, jam wrote:
> nit: ByTheBrowser is redundant since this is an IPC coming from the browser

Done.

https://codereview.chromium.org/1905033002/diff/970001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/common/navigat...
content/common/navigation_params.h:29: enum class WebMixedContentContextType;
On 2017/01/09 21:15:46, jam wrote:
> nit: why not just include the header here?

Done. Agreed... Here and in all other places where I did this.

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
content/public/browser/content_browser_client.h:770: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/09 21:15:46, jam wrote:
> this method should be on WebContentsDelegate. ContentBrowserClient is
generally
> used for:
> 1) embedder calls when there's no more specific delegate interface like WCD
> 2) web contents delegate methods that are common for all WebContents' used in
> the browser. in this case, this method only makes sense for tabs that the user
> sees (and the implementation depends on a class that is only added for browser
> tabs). so then it can be implemented on chrome's Browser class only.

Understood and thanks for this explanation.

But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I
know I need to have it implemented for:

* The default implementation, replacing ContentBrowserClient's implementation.
->>> Would be in WebContentsDelegate itself.

* User facing WebContents in Chrome, replacing ChromeContentBrowserClient's
implementation.
->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
substitute?

* Supporting layout tests within content_shell, replacing
LayoutTestContentBrowserClient's implementation.
->>> As I can't find any WCD subclass under content/shell/browser/layout_test/ I
think I'd have to create a new one to hold that implementation.

* And now that I am thinking about this, it seems active mixed content should
also work for user facing WebContents in content_shell itself, and not be always
blocked. That would be as if I had overridden it in ShellContentBrowserClient.
->>> It seems content::Shell could implement this? As I couldn't find evidence
of settings or flags under content/shell/ I'm assuming they don't exist and I'd
simply return allowed_per_settings.

And shouldn't PassiveInsecureContentFound be moved too? It seems what you
described also applies to it.

Final sanity check: are you sure mixed content checks only applies to user
visible WebContents? I see WCD subclasses for things like extensions, headless,
prerender and more, and they make me wonder...

https://codereview.chromium.org/1905033002/diff/970001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1905033002/diff/970001/net/url_request/url_re...
net/url_request/url_request.h:594: const GURL& GetDeferredRedirectUrl();
On 2017/01/09 21:15:46, jam wrote:
> nit: i believe these net changes aren't needed anymore? I don't see callers

Yes, I spun this off, changed it there and forgot to remove it here. Good catch!

jam

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_host/mixed_content_navigation_throttle.h File content/browser/frame_host/mixed_content_navigation_throttle.h (right): https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_host/mixed_content_navigation_throttle.h#newcode51 content/browser/frame_host/mixed_content_navigation_throttle.h:51: MixedContentFormsSubmitted = 441, On 2017/01/10 19:13:11, carlosk wrote: > ...

3 years, 11 months ago (2017-01-10 19:28:37 UTC) #150

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.h:51:
MixedContentFormsSubmitted = 441,
On 2017/01/10 19:13:11, carlosk wrote:
> On 2017/01/09 21:15:46, jam wrote:
> > It's unfortunate to copy these enums between webkit and content. I guess
we're
> > guaranteed that the values in blink never change?
> 
> The inertia for those values ever changing is very high. They are the mapping
> from the enum in blink::UseCounter to the respective histogram entries
tracking
> feature usage. So, practically speaking, yes.
> 
> > One possible way to avoid this duplication is to create this enum in
> > webkit/public (perhaps in WebMixedContentContextType.h ?) and use these
values
> > in content/browser and send them in the IPC. Then content/renderer can call
a
> > method in WebMixedContent.h that takes this enum and translates it into the
> > internal webkit enum?
> >
> > also nit: use upper case per chromium style guide
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> 
> IIUC this wouldn't avoid the duplication but it would make it possible to 1)
> keep these values in Blink-land, 2) allow the conversion code to be Blink
> internal and 3) even allow for static-checking them. But I'd still have to
have
> this selected copy of values from blink::UseCounter so that they can be in a
> public Blink location.

just to be clear, I had meant that the content defined enum wouldn't match the
same values, but there would be code that maps from one to the other.

but given that these values aren't changing, I'm fine with this for now.

> 
> re nit: It would as well help me avoid having to convert casing of all these
> values. ;)
> 
> Did I understand you correctly?

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
content/public/browser/content_browser_client.h:770: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/10 19:13:11, carlosk wrote:
> On 2017/01/09 21:15:46, jam wrote:
> > this method should be on WebContentsDelegate. ContentBrowserClient is
> generally
> > used for:
> > 1) embedder calls when there's no more specific delegate interface like WCD
> > 2) web contents delegate methods that are common for all WebContents' used
in
> > the browser. in this case, this method only makes sense for tabs that the
user
> > sees (and the implementation depends on a class that is only added for
browser
> > tabs). so then it can be implemented on chrome's Browser class only.
> 
> Understood and thanks for this explanation.
> 
> But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I
> know I need to have it implemented for:
> 
> * The default implementation, replacing ContentBrowserClient's implementation.
> ->>> Would be in WebContentsDelegate itself.
> 
> * User facing WebContents in Chrome, replacing ChromeContentBrowserClient's
> implementation.
> ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
> substitute?

right. browser is the one that's currently adding the MixedContentSettings tab
helper which the current implementation depends on.

> 
> * Supporting layout tests within content_shell, replacing
> LayoutTestContentBrowserClient's implementation.
> ->>> As I can't find any WCD subclass under content/shell/browser/layout_test/
I
> think I'd have to create a new one to hold that implementation.

content/shell/browser/shell.h is the WebContentsDelegate for content_shell (note
a WCD is always required)

> 
> * And now that I am thinking about this, it seems active mixed content should
> also work for user facing WebContents in content_shell itself, and not be
always
> blocked. That would be as if I had overridden it in ShellContentBrowserClient.
> ->>> It seems content::Shell could implement this? As I couldn't find evidence
> of settings or flags under content/shell/ I'm assuming they don't exist and
I'd
> simply return allowed_per_settings.
> 
> 
> And shouldn't PassiveInsecureContentFound be moved too? It seems what you
> described also applies to it.

yep SG

> 
> 
> Final sanity check: are you sure mixed content checks only applies to user
> visible WebContents? I see WCD subclasses for things like extensions,
headless,
> prerender and more, and they make me wonder...

these don't get tabhelpers, i.e. the MixedContentSettings  class that you added
wouldn't run. so ChromeContentBrowserClient::ShouldAllowRunningInsecureContent
will crash with a null allowed_per_settings


you bring up a good point though. What's the current behavior for extensions,
prerender etc? we should maintain that behavior. if the renderer is doing it for
all types of RenderFrames, which it appears to, then we can keep this method on
ContentBrowserClient. But then you'll need to get the tab helper added for
extensions, prerender etc...

carlosk

Thanks. Now all is clear and I will work on this for the next patch. ...

3 years, 11 months ago (2017-01-10 20:03:59 UTC) #151

Thanks. Now all is clear and I will work on this for the next patch.

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.h:51:
MixedContentFormsSubmitted = 441,
On 2017/01/10 19:28:37, jam wrote:
> On 2017/01/10 19:13:11, carlosk wrote:
> > On 2017/01/09 21:15:46, jam wrote:
> > > It's unfortunate to copy these enums between webkit and content. I guess
> we're
> > > guaranteed that the values in blink never change?
> > 
> > The inertia for those values ever changing is very high. They are the
mapping
> > from the enum in blink::UseCounter to the respective histogram entries
> tracking
> > feature usage. So, practically speaking, yes.
> > 
> > > One possible way to avoid this duplication is to create this enum in
> > > webkit/public (perhaps in WebMixedContentContextType.h ?) and use these
> values
> > > in content/browser and send them in the IPC. Then content/renderer can
call
> a
> > > method in WebMixedContent.h that takes this enum and translates it into
the
> > > internal webkit enum?
> > >
> > > also nit: use upper case per chromium style guide
> > >
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > 
> > IIUC this wouldn't avoid the duplication but it would make it possible to 1)
> > keep these values in Blink-land, 2) allow the conversion code to be Blink
> > internal and 3) even allow for static-checking them. But I'd still have to
> have
> > this selected copy of values from blink::UseCounter so that they can be in a
> > public Blink location.
> 
> just to be clear, I had meant that the content defined enum wouldn't match the
> same values, but there would be code that maps from one to the other.
> 
> but given that these values aren't changing, I'm fine with this for now.
> 
> > 
> > re nit: It would as well help me avoid having to convert casing of all these
> > values. ;)
> > 
> > Did I understand you correctly?
> 

Acknowledged. Will keep all as is then.

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
content/public/browser/content_browser_client.h:770: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/10 19:28:37, jam wrote:
> On 2017/01/10 19:13:11, carlosk wrote:
> > On 2017/01/09 21:15:46, jam wrote:
> > > this method should be on WebContentsDelegate. ContentBrowserClient is
> > generally
> > > used for:
> > > 1) embedder calls when there's no more specific delegate interface like
WCD
> > > 2) web contents delegate methods that are common for all WebContents' used
> in
> > > the browser. in this case, this method only makes sense for tabs that the
> user
> > > sees (and the implementation depends on a class that is only added for
> browser
> > > tabs). so then it can be implemented on chrome's Browser class only.
> > 
> > Understood and thanks for this explanation.
> > 
> > But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I
> > know I need to have it implemented for:
> > 
> > * The default implementation, replacing ContentBrowserClient's
implementation.
> > ->>> Would be in WebContentsDelegate itself.
> > 
> > * User facing WebContents in Chrome, replacing ChromeContentBrowserClient's
> > implementation.
> > ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
> > substitute?
> 
> right. browser is the one that's currently adding the MixedContentSettings tab
> helper which the current implementation depends on.
> 
> > 
> > * Supporting layout tests within content_shell, replacing
> > LayoutTestContentBrowserClient's implementation.
> > ->>> As I can't find any WCD subclass under
content/shell/browser/layout_test/
> I
> > think I'd have to create a new one to hold that implementation.
> 
> content/shell/browser/shell.h is the WebContentsDelegate for content_shell
(note
> a WCD is always required)

There is logic that is specific to layout tests that shouldn't apply to
content_shell running by itself (see LayoutTestContentBrowserClient's
implementation). As test support is built in (and not in a separate target) it
might be possible to consolidate both in Shell.

> > * And now that I am thinking about this, it seems active mixed content
should
> > also work for user facing WebContents in content_shell itself, and not be
> always
> > blocked. That would be as if I had overridden it in
ShellContentBrowserClient.
> > ->>> It seems content::Shell could implement this? As I couldn't find
evidence
> > of settings or flags under content/shell/ I'm assuming they don't exist and
> I'd
> > simply return allowed_per_settings.
> > 
> > 
> > And shouldn't PassiveInsecureContentFound be moved too? It seems what you
> > described also applies to it.
> 
> yep SG

I will move both then.

> > 
> > 
> > Final sanity check: are you sure mixed content checks only applies to user
> > visible WebContents? I see WCD subclasses for things like extensions,
> headless,
> > prerender and more, and they make me wonder...
> 
> these don't get tabhelpers, i.e. the MixedContentSettings  class that you
added
> wouldn't run. so ChromeContentBrowserClient::ShouldAllowRunningInsecureContent
> will crash with a null allowed_per_settings
> 
> 
> you bring up a good point though. What's the current behavior for extensions,
> prerender etc? we should maintain that behavior. if the renderer is doing it
for
> all types of RenderFrames, which it appears to, then we can keep this method
on
> ContentBrowserClient. But then you'll need to get the tab helper added for
> extensions, prerender etc...

Then I'll keep the current behavior by changing the default return value I I am
currently using: instead of always returning false I'll return
allowed_per_settings' value. This will solve the content_shell case I mentioned
above and will also match the renderer behavior (see
FrameLoaderClientImpl::allowRunningInsecureContent)

jam

On 2017/01/10 20:03:59, carlosk wrote: > Thanks. Now all is clear and I will work ...

3 years, 11 months ago (2017-01-10 20:22:12 UTC) #152

On 2017/01/10 20:03:59, carlosk wrote:
> Thanks. Now all is clear and I will work on this for the next patch.
> 
>
https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
> File content/browser/frame_host/mixed_content_navigation_throttle.h (right):
> 
>
https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
> content/browser/frame_host/mixed_content_navigation_throttle.h:51:
> MixedContentFormsSubmitted = 441,
> On 2017/01/10 19:28:37, jam wrote:
> > On 2017/01/10 19:13:11, carlosk wrote:
> > > On 2017/01/09 21:15:46, jam wrote:
> > > > It's unfortunate to copy these enums between webkit and content. I guess
> > we're
> > > > guaranteed that the values in blink never change?
> > > 
> > > The inertia for those values ever changing is very high. They are the
> mapping
> > > from the enum in blink::UseCounter to the respective histogram entries
> > tracking
> > > feature usage. So, practically speaking, yes.
> > > 
> > > > One possible way to avoid this duplication is to create this enum in
> > > > webkit/public (perhaps in WebMixedContentContextType.h ?) and use these
> > values
> > > > in content/browser and send them in the IPC. Then content/renderer can
> call
> > a
> > > > method in WebMixedContent.h that takes this enum and translates it into
> the
> > > > internal webkit enum?
> > > >
> > > > also nit: use upper case per chromium style guide
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > > 
> > > IIUC this wouldn't avoid the duplication but it would make it possible to
1)
> > > keep these values in Blink-land, 2) allow the conversion code to be Blink
> > > internal and 3) even allow for static-checking them. But I'd still have to
> > have
> > > this selected copy of values from blink::UseCounter so that they can be in
a
> > > public Blink location.
> > 
> > just to be clear, I had meant that the content defined enum wouldn't match
the
> > same values, but there would be code that maps from one to the other.
> > 
> > but given that these values aren't changing, I'm fine with this for now.
> > 
> > > 
> > > re nit: It would as well help me avoid having to convert casing of all
these
> > > values. ;)
> > > 
> > > Did I understand you correctly?
> > 
> 
> Acknowledged. Will keep all as is then.
> 
>
https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
> content/public/browser/content_browser_client.h:770: virtual bool
> ShouldAllowRunningInsecureContent(bool allowed_per_settings,
> On 2017/01/10 19:28:37, jam wrote:
> > On 2017/01/10 19:13:11, carlosk wrote:
> > > On 2017/01/09 21:15:46, jam wrote:
> > > > this method should be on WebContentsDelegate. ContentBrowserClient is
> > > generally
> > > > used for:
> > > > 1) embedder calls when there's no more specific delegate interface like
> WCD
> > > > 2) web contents delegate methods that are common for all WebContents'
used
> > in
> > > > the browser. in this case, this method only makes sense for tabs that
the
> > user
> > > > sees (and the implementation depends on a class that is only added for
> > browser
> > > > tabs). so then it can be implemented on chrome's Browser class only.
> > > 
> > > Understood and thanks for this explanation.
> > > 
> > > But now I'm also kinda lost in the sea of 72 existing subclasses of WCD...
I
> > > know I need to have it implemented for:
> > > 
> > > * The default implementation, replacing ContentBrowserClient's
> implementation.
> > > ->>> Would be in WebContentsDelegate itself.
> > > 
> > > * User facing WebContents in Chrome, replacing
ChromeContentBrowserClient's
> > > implementation.
> > > ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
> > > substitute?
> > 
> > right. browser is the one that's currently adding the MixedContentSettings
tab
> > helper which the current implementation depends on.
> > 
> > > 
> > > * Supporting layout tests within content_shell, replacing
> > > LayoutTestContentBrowserClient's implementation.
> > > ->>> As I can't find any WCD subclass under
> content/shell/browser/layout_test/
> > I
> > > think I'd have to create a new one to hold that implementation.
> > 
> > content/shell/browser/shell.h is the WebContentsDelegate for content_shell
> (note
> > a WCD is always required)
> 
> There is logic that is specific to layout tests that shouldn't apply to
> content_shell running by itself (see LayoutTestContentBrowserClient's
> implementation). As test support is built in (and not in a separate target) it
> might be possible to consolidate both in Shell.
> 
> > > * And now that I am thinking about this, it seems active mixed content
> should
> > > also work for user facing WebContents in content_shell itself, and not be
> > always
> > > blocked. That would be as if I had overridden it in
> ShellContentBrowserClient.
> > > ->>> It seems content::Shell could implement this? As I couldn't find
> evidence
> > > of settings or flags under content/shell/ I'm assuming they don't exist
and
> > I'd
> > > simply return allowed_per_settings.
> > > 
> > > 
> > > And shouldn't PassiveInsecureContentFound be moved too? It seems what you
> > > described also applies to it.
> > 
> > yep SG
> 
> I will move both then.
> 
> > > 
> > > 
> > > Final sanity check: are you sure mixed content checks only applies to user
> > > visible WebContents? I see WCD subclasses for things like extensions,
> > headless,
> > > prerender and more, and they make me wonder...
> > 
> > these don't get tabhelpers, i.e. the MixedContentSettings  class that you
> added
> > wouldn't run. so
ChromeContentBrowserClient::ShouldAllowRunningInsecureContent
> > will crash with a null allowed_per_settings
> > 
> > 
> > you bring up a good point though. What's the current behavior for
extensions,
> > prerender etc? we should maintain that behavior. if the renderer is doing it
> for
> > all types of RenderFrames, which it appears to, then we can keep this method
> on
> > ContentBrowserClient. But then you'll need to get the tab helper added for
> > extensions, prerender etc...
> 
> Then I'll keep the current behavior by changing the default return value I I
am
> currently using: instead of always returning false I'll return
> allowed_per_settings' value. This will solve the content_shell case I
mentioned
> above and will also match the renderer behavior (see
> FrameLoaderClientImpl::allowRunningInsecureContent)

returning allowed_per_settings sgtm, I just traced through the code and I agree
this would match the renderer behavior.

So just to be clear, this will move to WebContentsDelegate then?

carlosk

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

3 years, 11 months ago (2017-01-11 02:14:39 UTC) #153

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-11 02:15:07 UTC) #154

carlosk

Thanks. All open concerns were addressed and I really like the current state. I think ...

3 years, 11 months ago (2017-01-11 03:32:21 UTC) #155

Thanks. All open concerns were addressed and I really like the current state. I
think it's almost time for the final round of reviews to get this little monster
of a CL finally landed.

So, jam@, PTAL. After I get your LGTM I'll poke the other reviewers.

On my side I'm doing a final pass on comments and other minor stuff.


On 2017/01/10 20:22:12, jam wrote:
> returning allowed_per_settings sgtm, I just traced through the code and I
agree
> this would match the renderer behavior.
> 
> So just to be clear, this will move to WebContentsDelegate then?

Yes. This was done in this last patch set.

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...
content/browser/frame_host/mixed_content_navigation_throttle.h:51:
MixedContentFormsSubmitted = 441,
On 2017/01/10 20:03:59, carlosk wrote:
> On 2017/01/10 19:28:37, jam wrote:
> > On 2017/01/10 19:13:11, carlosk wrote:
> > > On 2017/01/09 21:15:46, jam wrote:
> > > > It's unfortunate to copy these enums between webkit and content. I guess
> > we're
> > > > guaranteed that the values in blink never change?
> > > 
> > > The inertia for those values ever changing is very high. They are the
> mapping
> > > from the enum in blink::UseCounter to the respective histogram entries
> > tracking
> > > feature usage. So, practically speaking, yes.
> > > 
> > > > One possible way to avoid this duplication is to create this enum in
> > > > webkit/public (perhaps in WebMixedContentContextType.h ?) and use these
> > values
> > > > in content/browser and send them in the IPC. Then content/renderer can
> call
> > a
> > > > method in WebMixedContent.h that takes this enum and translates it into
> the
> > > > internal webkit enum?
> > > >
> > > > also nit: use upper case per chromium style guide
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > > 
> > > IIUC this wouldn't avoid the duplication but it would make it possible to
1)
> > > keep these values in Blink-land, 2) allow the conversion code to be Blink
> > > internal and 3) even allow for static-checking them. But I'd still have to
> > have
> > > this selected copy of values from blink::UseCounter so that they can be in
a
> > > public Blink location.
> > 
> > just to be clear, I had meant that the content defined enum wouldn't match
the
> > same values, but there would be code that maps from one to the other.
> > 
> > but given that these values aren't changing, I'm fine with this for now.
> > 
> > > 
> > > re nit: It would as well help me avoid having to convert casing of all
these
> > > values. ;)
> > > 
> > > Did I understand you correctly?
> > 
> 
> Acknowledged. Will keep all as is then.

In fact I still had to re-case the enum values so... Done.

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1905033002/diff/970001/content/public/browser...
content/public/browser/content_browser_client.h:770: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/10 20:03:59, carlosk wrote:
> On 2017/01/10 19:28:37, jam wrote:
> > On 2017/01/10 19:13:11, carlosk wrote:
> > > On 2017/01/09 21:15:46, jam wrote:
> > > > this method should be on WebContentsDelegate. ContentBrowserClient is
> > > generally
> > > > used for:
> > > > 1) embedder calls when there's no more specific delegate interface like
> WCD
> > > > 2) web contents delegate methods that are common for all WebContents'
used
> > in
> > > > the browser. in this case, this method only makes sense for tabs that
the
> > user
> > > > sees (and the implementation depends on a class that is only added for
> > browser
> > > > tabs). so then it can be implemented on chrome's Browser class only.
> > > 
> > > Understood and thanks for this explanation.
> > > 
> > > But now I'm also kinda lost in the sea of 72 existing subclasses of WCD...
I
> > > know I need to have it implemented for:
> > > 
> > > * The default implementation, replacing ContentBrowserClient's
> implementation.
> > > ->>> Would be in WebContentsDelegate itself.
> > > 
> > > * User facing WebContents in Chrome, replacing
ChromeContentBrowserClient's
> > > implementation.
> > > ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
> > > substitute?
> > 
> > right. browser is the one that's currently adding the MixedContentSettings
tab
> > helper which the current implementation depends on.
> > 
> > > 
> > > * Supporting layout tests within content_shell, replacing
> > > LayoutTestContentBrowserClient's implementation.
> > > ->>> As I can't find any WCD subclass under
> content/shell/browser/layout_test/
> > I
> > > think I'd have to create a new one to hold that implementation.
> > 
> > content/shell/browser/shell.h is the WebContentsDelegate for content_shell
> (note
> > a WCD is always required)
> 
> There is logic that is specific to layout tests that shouldn't apply to
> content_shell running by itself (see LayoutTestContentBrowserClient's
> implementation). As test support is built in (and not in a separate target) it
> might be possible to consolidate both in Shell.
> 
> > > * And now that I am thinking about this, it seems active mixed content
> should
> > > also work for user facing WebContents in content_shell itself, and not be
> > always
> > > blocked. That would be as if I had overridden it in
> ShellContentBrowserClient.
> > > ->>> It seems content::Shell could implement this? As I couldn't find
> evidence
> > > of settings or flags under content/shell/ I'm assuming they don't exist
and
> > I'd
> > > simply return allowed_per_settings.
> > > 
> > > 
> > > And shouldn't PassiveInsecureContentFound be moved too? It seems what you
> > > described also applies to it.
> > 
> > yep SG
> 
> I will move both then.
> 
> > > 
> > > 
> > > Final sanity check: are you sure mixed content checks only applies to user
> > > visible WebContents? I see WCD subclasses for things like extensions,
> > headless,
> > > prerender and more, and they make me wonder...
> > 
> > these don't get tabhelpers, i.e. the MixedContentSettings  class that you
> added
> > wouldn't run. so
ChromeContentBrowserClient::ShouldAllowRunningInsecureContent
> > will crash with a null allowed_per_settings
> > 
> > 
> > you bring up a good point though. What's the current behavior for
extensions,
> > prerender etc? we should maintain that behavior. if the renderer is doing it
> for
> > all types of RenderFrames, which it appears to, then we can keep this method
> on
> > ContentBrowserClient. But then you'll need to get the tab helper added for
> > extensions, prerender etc...
> 
> Then I'll keep the current behavior by changing the default return value I I
am
> currently using: instead of always returning false I'll return
> allowed_per_settings' value. This will solve the content_shell case I
mentioned
> above and will also match the renderer behavior (see
> FrameLoaderClientImpl::allowRunningInsecureContent)

Done.

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-11 04:03:47 UTC) #156

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-11 04:03:50 UTC) #157

jam

Thanks for bearing through. This looks ready to land as well to me. I just ...

3 years, 11 months ago (2017-01-11 16:38:06 UTC) #158

carlosk

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

3 years, 11 months ago (2017-01-11 22:09:59 UTC) #159

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-11 22:11:44 UTC) #160

carlosk

Thanks again jam@. Time to start the final round of reviews. mkwst@: PTAL at all ...

3 years, 11 months ago (2017-01-11 22:15:03 UTC) #161

Thanks again jam@. Time to start the final round of reviews.

mkwst@: PTAL at all mixed content and WebKit related changes.

nasko@: PTAL at content and IPC changes.

jam@: I'm hoping your ongoing review will cover all of chrome/** changes?

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site
instance.
On 2017/01/11 16:38:06, jam wrote:
> I don't understand why site instance is involved here. i.e. why aren't we just
> resetting this value on every navigation to a different page (i.e. adding ||
> !IsSamePage() to above check). The reason I say this is that presumably a user
> bubble action is just for the page, and its effects go away when going to a
> different page?

That's not how the mixed content settings are reset on the renderer. I somewhat
described this in a section [1] in my initial design doc for this change (Google
internal). There's a link to the code that does this but in short these settings
are reset on the renderer only when the main RenderFrame is replaced.

So it seems to me that a reasonable translation to browser/browser is that cross
site navigations cause the reset hence the monitoring of SiteInstance changes.

You can test it yourself with test sites for mixed content, like for instance
[2]. Navigate to that site, allow mixed content scripts to run and the do this:

* Case 1: Click the "Test" button by the password field, which will cause a same
site navigation, then hit back. Active mixed content is still allowed.
* Case 2: Navigate to wikipedia.org from the omnibar and then hit back. Active
mixed content is not allowed anymore.

I did ask about this many times while working on this and never got a clear
answer. I don't think anyone has a definitive understanding or opinion on how
this is supposed to work.

[1]
https://docs.google.com/document/d/1bNZG5ZN9flm1_-03sShI4466heqpBTVSnjc-HJtvw...

[2] https://www.bennish.net/mixed-content.html

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:257: bool
strictMode =
On 2017/01/11 16:38:06, jam wrote:
> nit: strict_mode

Done.

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:268:
WebContentsDelegate* web_contents_delegate =
On 2017/01/11 16:38:06, jam wrote:
> frame_host can't include web_contents for layering reasons (see comment at the
> top of the DEPS file).
> 
> This should go through RenderFrameHostDelegate. So add the two new methods to
> that interface, implement them in WebContentsImpl, and then call out to WCD
> there

Done.

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:334: // - The
root nor parent frames' origins are secure.
On 2017/01/11 16:38:06, jam wrote:
> is this supposed to read "or"?

No but I rephrased it a bit. Apparently nor should generally come after a
neither. :)

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/1905033002/diff/1030001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:49:
GURL originUrl(test.origin);
On 2017/01/11 16:38:06, jam wrote:
> nit: here and below use chromium style for variable naming

Done.

https://codereview.chromium.org/1905033002/diff/1030001/content/renderer/rend...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1030001/content/renderer/rend...
content/renderer/render_frame_impl.cc:8: #include <set>
On 2017/01/11 16:38:06, jam wrote:
> no need, it's in the header by definition

Done.

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-12 00:21:21 UTC) #162

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-12 00:21:23 UTC) #163

jam

lgtm https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/content_settings/mixed_content_settings.cc File chrome/browser/content_settings/mixed_content_settings.cc (right): https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/content_settings/mixed_content_settings.cc#newcode52 chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site instance. On 2017/01/11 ...

3 years, 11 months ago (2017-01-12 01:39:00 UTC) #164

lgtm

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site
instance.
On 2017/01/11 22:15:02, carlosk wrote:
> On 2017/01/11 16:38:06, jam wrote:
> > I don't understand why site instance is involved here. i.e. why aren't we
just
> > resetting this value on every navigation to a different page (i.e. adding ||
> > !IsSamePage() to above check). The reason I say this is that presumably a
user
> > bubble action is just for the page, and its effects go away when going to a
> > different page?
> 
> That's not how the mixed content settings are reset on the renderer. I
somewhat
> described this in a section [1] in my initial design doc for this change
(Google
> internal). There's a link to the code that does this but in short these
settings
> are reset on the renderer only when the main RenderFrame is replaced.
> 
> So it seems to me that a reasonable translation to browser/browser is that
cross
> site navigations cause the reset hence the monitoring of SiteInstance changes.
> 
> You can test it yourself with test sites for mixed content, like for instance
> [2]. Navigate to that site, allow mixed content scripts to run and the do
this:
> 
> * Case 1: Click the "Test" button by the password field, which will cause a
same
> site navigation, then hit back. Active mixed content is still allowed.
> * Case 2: Navigate to http://wikipedia.org from the omnibar and then hit back.
Active
> mixed content is not allowed anymore.
> 
> I did ask about this many times while working on this and never got a clear
> answer. I don't think anyone has a definitive understanding or opinion on how
> this is supposed to work.
> 
> [1]
>
https://docs.google.com/document/d/1bNZG5ZN9flm1_-03sShI4466heqpBTVSnjc-HJtvw...
> 
> [2] https://www.bennish.net/mixed-content.html

Ah, thanks for the explanation. I had assumed it works like other bubbles but
now I see it's different.

ok your change makes sense then. Since this is tricky, please add a comment to
say this matches the renderer behavior which doesn't reset it on navigation

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/render_frame_host_delegate.cc:101: return false;
nit: why not return allowed_per_settings?

carlosk

Just replying to the comments. Changes are minimal so I'm holding on uploading so to ...

3 years, 11 months ago (2017-01-12 17:44:02 UTC) #165

Just replying to the comments. Changes are minimal so I'm holding on uploading
so to not disturb ongoing reviews with a new patch set.

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site
instance.
On 2017/01/12 01:39:00, jam wrote:
> On 2017/01/11 22:15:02, carlosk wrote:
> > On 2017/01/11 16:38:06, jam wrote:
> > > I don't understand why site instance is involved here. i.e. why aren't we
> just
> > > resetting this value on every navigation to a different page (i.e. adding
||
> > > !IsSamePage() to above check). The reason I say this is that presumably a
> user
> > > bubble action is just for the page, and its effects go away when going to
a
> > > different page?
> > 
> > That's not how the mixed content settings are reset on the renderer. I
> somewhat
> > described this in a section [1] in my initial design doc for this change
> (Google
> > internal). There's a link to the code that does this but in short these
> settings
> > are reset on the renderer only when the main RenderFrame is replaced.
> > 
> > So it seems to me that a reasonable translation to browser/browser is that
> cross
> > site navigations cause the reset hence the monitoring of SiteInstance
changes.
> > 
> > You can test it yourself with test sites for mixed content, like for
instance
> > [2]. Navigate to that site, allow mixed content scripts to run and the do
> this:
> > 
> > * Case 1: Click the "Test" button by the password field, which will cause a
> same
> > site navigation, then hit back. Active mixed content is still allowed.
> > * Case 2: Navigate to http://wikipedia.org from the omnibar and then hit
back.
> Active
> > mixed content is not allowed anymore.
> > 
> > I did ask about this many times while working on this and never got a clear
> > answer. I don't think anyone has a definitive understanding or opinion on
how
> > this is supposed to work.
> > 
> > [1]
> >
>
https://docs.google.com/document/d/1bNZG5ZN9flm1_-03sShI4466heqpBTVSnjc-HJtvw...
> > 
> > [2] https://www.bennish.net/mixed-content.html
> 
> Ah, thanks for the explanation. I had assumed it works like other bubbles but
> now I see it's different.
> 
> ok your change makes sense then. Since this is tricky, please add a comment to
> say this matches the renderer behavior which doesn't reset it on navigation

That was indeed missing. I added a detailed explanation here that will be in my
next patch set.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/render_frame_host_delegate.cc:101: return false;
On 2017/01/12 01:39:00, jam wrote:
> nit: why not return allowed_per_settings?

My reasoning was:
B) These methods were not required from them before anyway, so there's no
current users (beyond the "right" one).
A) For the 2 existing sub-classes of RenderFrameHostDelegate beyond
WebContentsImpl, calling any mixed content method on them looks like an error.

I even considered using NOTREACHED. IIUC these are default implemented only to
avoid the need for subclasses to create an empty override and not because they
are expected to be called.

WDYT?

jam

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/render_frame_host_delegate.cc File content/browser/frame_host/render_frame_host_delegate.cc (right): https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/render_frame_host_delegate.cc#newcode101 content/browser/frame_host/render_frame_host_delegate.cc:101: return false; On 2017/01/12 17:44:02, carlosk wrote: > On ...

3 years, 11 months ago (2017-01-12 18:24:08 UTC) #166

nasko

Sending a round of comments, but need to do a more thorough reading of mixed_content_navigation_throttle.cc ...

3 years, 11 months ago (2017-01-12 18:32:38 UTC) #167

Mike West

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/mixed_content_navigation_throttle.h File content/browser/frame_host/mixed_content_navigation_throttle.h (right): https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/mixed_content_navigation_throttle.h#newcode67 content/browser/frame_host/mixed_content_navigation_throttle.h:67: }; I know we talked about this a while ...

3 years, 11 months ago (2017-01-13 13:20:00 UTC) #168

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:67: };
I know we talked about this a while ago, but looking at the code, I think I'm
convinced that it does more harm than good to try to jam these metrics into the
existing use counter. Especially given that this only triggers in redirect
cases, I don't think the value of the information we're gathering here justifies
the complexity.

The only metric here I'm actively evaluating is
`MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME`, which I don't think this
patch actually affects (as we're gathering it after a response comes in).

I'd suggest removing `UseCounterFeature`, as well as `mixed_content_features_`
and its usage throughout.

I do want to keep the CSP reporting, as that's essential to helping developers
migrate from HTTP to HTTPS, but that doesn't rely on the usecounter code at all.

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:301: return false;
Does this change the timing of MIX and HSTS? That is, we currently check mixed
content first, then apply HSTS. It seems like this would change that, as we'd
send the request, upgrade it via HSTS, and only then kick into the throttle.
While that's the right thing to do in the long run, we shouldn't do it without
also changing the platform as per https://wicg.github.io/hsts-priming/.

Instead of returning false, why not just let the checks run through? Blink will
handle the initial request, and the browser will handle redirects. Seems like
the best of both worlds.

carlosk

Thanks. Latest patch set addresses most comments. I will integrate the new shared scheme registries ...

3 years, 11 months ago (2017-01-21 02:54:59 UTC) #169

Thanks.

Latest patch set addresses most comments. I will integrate the new shared scheme
registries [1] that jam@ landed in the next one.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=627502

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:19:
insecure_content_site_instance_(nullptr),
On 2017/01/12 18:32:37, nasko wrote:
> nit: You could initialize these in the header file these days with the new C++
> features allowed.

Done.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:24: // level as Blink
does.
On 2017/01/12 18:32:37, nasko wrote:
> Hmm, I think we now track openers at the frame level even browser side. Do we
> want to have frame granularity here? If not, then the comment might be more
> misleading than helpful the way it is now.

We want to keep frame granularity at tab-level as it is today.

I tried clarifying the comment. WDYT?

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:48: if
(!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
On 2017/01/12 18:32:37, nasko wrote:
> What happens if the commit was for an error page?

In PlzNavigate, the RenderFrame that would render the error page would be the
same as if the navigation had succeeded: either the current one if it was a
same-site navigation or a new one if it was cross-site. I confirmed this with
clamy@. So letting this execute in the error page case should be the right thing
to do.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site
instance.
On 2017/01/12 18:32:37, nasko wrote:
> nit: s/site instance/SiteInstance/

Done. This comment also grew considerably to explain why the settings are reset
on cross-site navigations.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.h:16: class
MixedContentSettings
On 2017/01/12 18:32:37, nasko wrote:
> Shouldn't this class name include "Observer", since it is one? Or TabHelper,
> since it is installed through TabHelpers.

I added the TabHelper suffix as I think it clarifies the expected behavior. But
FYI many of the other instances registered by TabHelpers are not especially
named.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.h:24: // navigates to a
different SiteInstance.
On 2017/01/12 18:32:37, nasko wrote:
> Why does the SiteInstance matter?

It does not so I removed it.

It is now better explained as an implementation detail in the .cc file.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.h:27: bool
insecure_content_allowed_running() {
On 2017/01/12 18:32:37, nasko wrote:
> nit: is_running_insecure_content_allowed

Done.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.h:40: // on cross-site
main frame navigations.
On 2017/01/12 18:32:37, nasko wrote:
> Why do they stay the same for same-site navigations? What's the difference?

See:
https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...

Do you think I should also not explain it here, keeping it as an implementation
detail (already explained in the .cc)?

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:222: // level of
the connection. For instance: does it use an outdated protocol?
On 2017/01/12 18:32:37, nasko wrote:
> nit: s/:/,/

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:224: //
MixedContentChecker::handleCertificateError.
On 2017/01/12 18:32:37, nasko wrote:
> Should this be fixed before we consider mixed content checking browser side
> complete? Maybe put a crbug link here.

I am honestly unsure about this. This is checked mainly when the
"DidReceiveResponse" chain of calls happen on the renderer which I think happen
normally even with PlzNavigate enabled. For PlzNavigate* the reason for moving
mixed content checking to the browser is to avoid extra IPC-round trips to the
renderer and *I think* that doesn't apply for this method.

WDYT? Should we involve clamy@ in this matter?

I clarified the comment a little and added a reference to the main issue of this
very CL. I also updated that issue's description to explain this.

(*): Mind that OOPIF has different reasons to want to move mixed content checks
to the browser so it *could* be a requirement from their side.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:287: bool
shouldAskEmbedder =
On 2017/01/12 18:32:37, nasko wrote:
> nit: should_ask_embedder, this is not Blink code anymore. :)

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:351: // Note:
This code below should behave the same way as as the two calls to
On 2017/01/12 18:32:37, nasko wrote:
> nit: s/as as/as/

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:376:
RenderFrameHost* rfh = handle_impl->frame_tree_node()->current_frame_host();
On 2017/01/12 18:32:37, nasko wrote:
> Is the current RFH the correct place to always report these?

It shouldn't really matter what frame this is reported to as on the renderer
these values are stored at the page level (UseCounter is a member of the Page
instance). See this UseCounter::count [1] implementation.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Use...

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:397: // Note: as
there's no mixed content checks for sub resources on the browser
On 2017/01/12 18:32:37, nasko wrote:
> nit: sub-resources or subresources, "browser side"

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/01/12 18:32:37, nasko wrote:
> nit: We are now 2017 ;).

Done for all added files.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:25: //
Responsible for browser-process-side mixed-content security. It is only
On 2017/01/12 18:32:37, nasko wrote:
> nit: security checks.

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:39: 
On 2017/01/12 18:32:37, nasko wrote:
> nit: No need for empty spaces, can cluster all three with a "//
> NavigationThrottle " comment on top. This is a pattern used in a lot of files
to
> combine together all methods being overriden from a base class.

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:49:
MIXED_CONTENT_FORMS_SUBMITTED = 441,
On 2017/01/12 18:32:37, nasko wrote:
> Can we use static_assert to ensure we don't accidentally make a breaking
change?

See:
https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:67: };
On 2017/01/13 13:20:00, Mike West (sloooooow) wrote:
> I know we talked about this a while ago, but looking at the code, I think I'm
> convinced that it does more harm than good to try to jam these metrics into
the
> existing use counter. Especially given that this only triggers in redirect
> cases, I don't think the value of the information we're gathering here
justifies
> the complexity.
>
> The only metric here I'm actively evaluating is
> `MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME`, which I don't think this
> patch actually affects (as we're gathering it after a response comes in).
> 
> I'd suggest removing `UseCounterFeature`, as well as `mixed_content_features_`
> and its usage throughout.

Why do you think this triggers only on redirects? There is potential for feature
values to be recorded for any navigation that is checked by this class, even if
mixed content is not found. For instance see InWhichFrameIsContentMixed: it is
executed before the decision on the existence of mixed content and might already
have recorded feature usage to mixed_content_features_.

In light of this information, are you still in favor of removing?

And just to make the decision easier, this is the list of features that might
actually be reported:

MIXED_CONTENT_BLOCKABLE
MIXED_CONTENT_BLOCKABLE_ALLOWED
MIXED_CONTENT_IN_NON_HTTPS_FRAME_THAT_RESTRICTS_MIXED_CONTENT
MIXED_CONTENT_IN_SECURE_FRAME_THAT_DOES_NOT_RESTRICT_MIXED_CONTENT
MIXED_CONTENT_INTERNAL
MIXED_CONTENT_PREFETCH
MIXED_CONTENT_PRESENT

And if this should be finally kept then maybe I should trim down that enum to
only include these?

> I do want to keep the CSP reporting, as that's essential to helping developers
> migrate from HTTP to HTTPS, but that doesn't rely on the usecounter code at
all.

OK, this should be correctly done with the whole mixedContentFound(ByTheBrowser)
IPC and call chain.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:80: // Records
basic mixed content "feature" usage when any kind of mixed is
On 2017/01/12 18:32:37, nasko wrote:
> nit: s/mixed/mixed content/

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:87: static bool
CONTENT_EXPORT IsMixedContentForTesting(const GURL& origin_url,
On 2017/01/12 18:32:37, nasko wrote:
> Why is this method CONTENT_EXPORT and private at the same time?

Because its unit test friend-ed in line 45 needs the export to be able to call
this method.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/navigation_handle_impl.cc:793: new
MixedContentNavigationThrottle(this));
On 2017/01/12 18:32:37, nasko wrote:
> Does the ordering of when we add this matter? Would DevTools need to know and
be
> able to log the navigation before it is potentially blocked by the mixed
content
> checker?

I don't know the answer for this.

As this is a security check it seemed appropriate to move it up the chain but
that's just me guessing. I created a new never-to-be-committed CL [1] where I
moved this registration to be the last one and no trybots ever complained about
it.

I also noticed the established pattern here of concealing the creation decision
from each throttle within their classes by having a CreateThrottleForNavigation
method called from here. I updated my code to do the same.

Another thing I'm usure about is if I should register directly to |throttles_|
or through |throttles_to_register| as both cases exist. I chose the latter
because 2 out of 3 cases do that...? :O

[1] https://codereview.chromium.org/2630043002

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/render_frame_host_delegate.h:276: // Notifies that
mixed-content was displayed or ran within the container
On 2017/01/12 18:32:38, nasko wrote:
> You mean within the RenderFrameHost that is reporting this, right? Will there
be
> a case where one RFH reports about mixed content in another RFH?

This report only matters at the WebContents level and that's what I meant to
explain. Rephrased by that last part as it doesn't really matter here.

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
content/public/browser/web_contents_delegate.h:567: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/12 18:32:38, nasko wrote:
> nit: Shouldn't this be "allower_per_prefs"? When I read settings, I thought
> immediately content settings, which are maintained by the chrome/ layer and
got
> confused. Tracing the code, the value comes from WebPreferences.

I'm just following the pattern used in MixedContentsChecked for the mirror API
[1]. I didn't understand your explanation.

[1]
https://cs.chromium.org/chromium/src/chrome/renderer/content_settings_observe...

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
content/public/browser/web_contents_delegate.h:570: WebContents* web_contents);
On 2017/01/12 18:32:38, nasko wrote:
> Let's keep WebContents as the first parameter to be consistent with the rest
of
> the delegate methods and since it is the context in which this call is made.

Done.

https://codereview.chromium.org/1905033002/diff/1070001/content/renderer/rend...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/renderer/rend...
content/renderer/render_frame_impl.cc:1584: OnMixedContentFoundByTheBrowser)
On 2017/01/12 18:32:38, nasko wrote:
> Also on the naming, we usually just prefix the IPC name itself with On, so it
> will be also consistent with that.

Acknowledged.

https://codereview.chromium.org/1905033002/diff/1070001/content/renderer/rend...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/renderer/rend...
content/renderer/render_frame_impl.h:889: void
OnMixedContentFoundByTheBrowser(const GURL& main_resource_url,
On 2017/01/12 18:32:38, nasko wrote:
> Let's remove the "ByTheBrowser" as it doesn't really matter who found it, it
> matters that it is found and reported. It might not be the browser process in
> the future : ).

Done both for these browser side methods and for their renderer counterparts.

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:301: return false;
On 2017/01/13 13:20:00, Mike West (sloooooow) wrote:
> Does this change the timing of MIX and HSTS? That is, we currently check mixed
> content first, then apply HSTS. It seems like this would change that, as we'd
> send the request, upgrade it via HSTS, and only then kick into the throttle.
> While that's the right thing to do in the long run, we shouldn't do it without
> also changing the platform as per https://wicg.github.io/hsts-priming/.
> 
> Instead of returning false, why not just let the checks run through? Blink
will
> handle the initial request, and the browser will handle redirects. Seems like
> the best of both worlds.

I wasn't aware of HSTS until you mentioned it here so I don't know how these
changes would affect it. Are there tests I can run to verify that?

But your proposed solution of allowing checks to happen renderer-side as well
causes problems while running layout tests: in all cases where mixed content is
found but allowed the warning message end up being logged twice, once for each
detection in browser and renderer. Specific mixed content failures:

Regressions: Unexpected text-only failures (6)
  http/tests/security/mixedContent/insecure-iframe-in-iframe.html [ Failure ]
  http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html [
Failure ]
  http/tests/security/mixedContent/insecure-iframe-in-main-frame.html [ Failure
]

virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-iframe.html
[ Failure ]

virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html
[ Failure ]

virtual/mojo-loading/http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
[ Failure ]

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.h:105: // Receive a
information about mixed content that was found by the browser.
On 2017/01/12 18:32:38, nasko wrote:
> nit: No need for "a" before information.

Done.

carlosk

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

3 years, 11 months ago (2017-01-21 02:59:11 UTC) #170

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-21 02:59:30 UTC) #171

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-21 05:04:34 UTC) #172

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-21 05:04:35 UTC) #173

carlosk

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

3 years, 11 months ago (2017-01-21 06:40:51 UTC) #174

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-21 06:41:08 UTC) #175

carlosk

On 2017/01/21 02:54:59, carlosk wrote: > Thanks. > > Latest patch set addresses most comments. ...

3 years, 11 months ago (2017-01-21 06:52:36 UTC) #176

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-21 08:24:50 UTC) #177

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-21 08:24:52 UTC) #178

nasko

Thanks for the updates. I've also gone through the mixed content implementation, so expect lots ...

3 years, 11 months ago (2017-01-23 22:32:38 UTC) #179

Thanks for the updates. I've also gone through the mixed content implementation,
so expect lots of questions since I'm not familiar with it as much as mkwst@ is.
I'll defer to him on that area, but wanted to ensure I'm not missing something
as I read it.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:48: if
(!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
On 2017/01/21 02:54:58, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > What happens if the commit was for an error page?
> 
> In PlzNavigate, the RenderFrame that would render the error page would be the
> same as if the navigation had succeeded: either the current one if it was a
> same-site navigation or a new one if it was cross-site. I confirmed this with
> clamy@. So letting this execute in the error page case should be the right
thing
> to do.

Do we have a test that ensures that is the case and we don't regress it?

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:52: // to a new site
instance.
On 2017/01/21 02:54:58, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > nit: s/site instance/SiteInstance/
> 
> Done. This comment also grew considerably to explain why the settings are
reset
> on cross-site navigations.

Thanks! That makes it a lot more clear why.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.h:40: // on cross-site
main frame navigations.
On 2017/01/21 02:54:58, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > Why do they stay the same for same-site navigations? What's the difference?
> 
> See:
>
https://codereview.chromium.org/1905033002/diff/1030001/chrome/browser/conten...
> 
> Do you think I should also not explain it here, keeping it as an
implementation
> detail (already explained in the .cc)? 

Thanks for the pointer. Also the description you've expanded on in the .cc file
is good, so no need to duplicate here.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/navigation_handle_impl.cc:793: new
MixedContentNavigationThrottle(this));
On 2017/01/21 02:54:59, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > Does the ordering of when we add this matter? Would DevTools need to know
and
> be
> > able to log the navigation before it is potentially blocked by the mixed
> content
> > checker?
> 
> I don't know the answer for this.
> 
> As this is a security check it seemed appropriate to move it up the chain but
> that's just me guessing. I created a new never-to-be-committed CL [1] where I
> moved this registration to be the last one and no trybots ever complained
about
> it.

Let's keep it first then, until we hear something breaking.

> I also noticed the established pattern here of concealing the creation
decision
> from each throttle within their classes by having a
CreateThrottleForNavigation
> method called from here. I updated my code to do the same.

Cool, thanks! That's definitely nice.

> Another thing I'm usure about is if I should register directly to |throttles_|
> or through |throttles_to_register| as both cases exist. I chose the latter
> because 2 out of 3 cases do that...? :O

Yeah, what you did is good. We should follow up and make them all consistent.

> [1] https://codereview.chromium.org/2630043002

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
content/public/browser/web_contents_delegate.h:567: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/21 02:54:59, carlosk wrote:
> On 2017/01/12 18:32:38, nasko wrote:
> > nit: Shouldn't this be "allower_per_prefs"? When I read settings, I thought
> > immediately content settings, which are maintained by the chrome/ layer and
> got
> > confused. Tracing the code, the value comes from WebPreferences.
> 
> I'm just following the pattern used in MixedContentsChecked for the mirror API
> [1]. I didn't understand your explanation.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/renderer/content_settings_observe...

My comment meant to say that the source of the input is WebPreferences and not
ContentSettings, so naming the variable to indicate where it comes from makes it
easier to understand.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.cc
(right):

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.cc:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
nit: no "(c)".

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.cc:19: if
(tab->HasOpener()) {
nit: if (!tab->HasOpener()) return; will yield a bit easier to read code as it
won't be as indented.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.h
(right):

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
nit: No "(c)" needed.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:15: //
Controls mixed content related settings for the associated, working as the
nit: associated what?

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:71: bool
SecurityOriginIsSecure(const GURL& url) {
nit: IsOriginSecure? We don't have "SecurityOrigin" concept in browser code,
which makes reading the name of this method a bit awkward.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:74:
HasPotentiallySecureScheme(*url.inner_url())) ||
Let's use a different code pattern for extracting the data from Filesystem and
Blob URLs, so we are consistent across the codebase. Once we have one of these,
we can construct an url::Origin from the URL and check its scheme. An example
can be seen in ExtensionNavigationThrottle::WillStartRequest.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:90: // to
same-origin contexts, so they are not blocked either.
nit: no need for " either" as the rest of the comment from the Blink code is not
here and the "either" doesn't make much sense.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:95: //
SecurityOrigin::isPotentiallyTrustworthy without duplicating much of the
It is a bit sad to have this code and
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_re....
Any chance we can combine the two somehow? Possibly in a separate CL?

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:100: if
(IsUniqueScheme(url) || HasLocalScheme(url) ||
This doesn't match what isPotentiallyTrustworthy does, as it checks the
uniqueness of the origin, which is not what this code does. You could very well
have an http(s) based document that is in a unique origin, through iframe
sandboxing. Would this work correctly in those cases?

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:107:
net::IsLocalHostname(url.HostNoBrackets(), nullptr)) {
Why IsLocalHostname vs IsLocalhost above?

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:216: if
(!ShouldTreatURLSchemeAsCORSEnabled(handle_impl->GetURL())) {
nit: no need of {} for one line if statements.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:19:
const bool expectation;
nit: What is the meaning of "expectation"? Maybe rename it more explicitly
"expected_mixed_content" or something like that.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:31:
{"https://example.com/foo", "blob:http://example.com/foo", false},
nit: I know this is how the code works, but this just feels wrong :(.

carlosk

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

3 years, 11 months ago (2017-01-25 06:37:12 UTC) #180

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 06:37:23 UTC) #181

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 06:39:15 UTC) #182

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/141903) ios-device-xcode-clang on ...

3 years, 11 months ago (2017-01-25 06:39:17 UTC) #183

Mike West

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/mixed_content_navigation_throttle.h File content/browser/frame_host/mixed_content_navigation_throttle.h (right): https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame_host/mixed_content_navigation_throttle.h#newcode67 content/browser/frame_host/mixed_content_navigation_throttle.h:67: }; On 2017/01/21 at 02:54:59, carlosk wrote: > On ...

3 years, 11 months ago (2017-01-25 11:37:30 UTC) #184

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:67: };
On 2017/01/21 at 02:54:59, carlosk wrote:
> On 2017/01/13 13:20:00, Mike West (sloooooow) wrote:
> > I know we talked about this a while ago, but looking at the code, I think
I'm
> > convinced that it does more harm than good to try to jam these metrics into
the
> > existing use counter. Especially given that this only triggers in redirect
> > cases, I don't think the value of the information we're gathering here
justifies
> > the complexity.
> >
> > The only metric here I'm actively evaluating is
> > `MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME`, which I don't think
this
> > patch actually affects (as we're gathering it after a response comes in).
> > 
> > I'd suggest removing `UseCounterFeature`, as well as
`mixed_content_features_`
> > and its usage throughout.
> 
> Why do you think this triggers only on redirects?

Because I think you should remove the early-exit for frame navigations that
we're talking about in `MixedContentChecker.cpp` in order to make sure that we
don't change the ordering of MIX and HSTS, which has the side-effect of ensuring
that we'd only hit this throttle for redirects (or cases in which the user
explicitly allowed mixed content, which is vanishingly small). :)

> > I do want to keep the CSP reporting, as that's essential to helping
developers
> > migrate from HTTP to HTTPS, but that doesn't rely on the usecounter code at
all.
> 
> OK, this should be correctly done with the whole
mixedContentFound(ByTheBrowser) IPC and call chain.

Yup!

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/1070001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:301: return false;
On 2017/01/21 at 02:54:59, carlosk wrote:
> On 2017/01/13 13:20:00, Mike West (sloooooow) wrote:
> > Does this change the timing of MIX and HSTS? That is, we currently check
mixed
> > content first, then apply HSTS. It seems like this would change that, as
we'd
> > send the request, upgrade it via HSTS, and only then kick into the throttle.
> > While that's the right thing to do in the long run, we shouldn't do it
without
> > also changing the platform as per https://wicg.github.io/hsts-priming/.
> > 
> > Instead of returning false, why not just let the checks run through? Blink
will
> > handle the initial request, and the browser will handle redirects. Seems
like
> > the best of both worlds.
> 
> I wasn't aware of HSTS until you mentioned it here so I don't know how these
changes would affect it. Are there tests I can run to verify that?

I would hope that we have tests, but I wouldn't be at all surprised to find that
we don't. :) As a manual test, add `<iframe src="http://youtube.com/"></iframe>`
to `https://example.com/`. It shouldn't load. As a less-manual test, we'd
probably need to teach Blink's layout test engine how to register specific
`*.test` subdomains as HSTSized.

> But your proposed solution of allowing checks to happen renderer-side as well
causes problems while running layout tests: in all cases where mixed content is
found but allowed the warning message end up being logged twice, once for each
detection in browser and renderer.

That surprises me, as we should be blocking the load before it hits the browser
if we consider it mixed content. Ah, looking at the tests, they all include
`testRunner.overridePreference("WebKitAllowRunningInsecureContent", true);`,
which explains things. Reworking those tests to treat frames as blockable
content (which is the behavior we ship) should resolve the double-warning.

It wouldn't resolve doubled error messages for folks who click through the
mixed-content shield, but since we're planning to remove that shield at some
point in the somewhat near future, I'm not terribly concerned about that case.

WDYT?

carlosk

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

3 years, 11 months ago (2017-01-25 21:18:31 UTC) #186

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 21:19:21 UTC) #187

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 21:35:01 UTC) #188

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/309965) mac_chromium_rel_ng on ...

3 years, 11 months ago (2017-01-25 21:35:05 UTC) #189

carlosk

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

3 years, 11 months ago (2017-01-25 21:59:52 UTC) #190

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 22:00:47 UTC) #192

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-25 23:15:49 UTC) #193

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/376184)

3 years, 11 months ago (2017-01-25 23:15:51 UTC) #194

carlosk

I'm still investigating nasko@'s suggestions in regards to the use of url::Origin, consolidating IsPotentiallyTrustworthyOrigin and ...

3 years, 10 months ago (2017-01-28 02:09:44 UTC) #195

I'm still investigating nasko@'s suggestions in regards to the use of
url::Origin, consolidating IsPotentiallyTrustworthyOrigin and the Origin
uniqueness checks, what is taking longer.

But I thought a lot about mkwst@'s suggestion and I wanted to address it before
anything else because I think it may require significant changes to this patch. 

On 2017/01/25 11:37:30, Mike West (sloooooow) wrote:
> > Why do you think this triggers only on redirects?
> 
> Because I think you should remove the early-exit for frame navigations that
> we're talking about in `MixedContentChecker.cpp` in order to make sure that we
> don't change the ordering of MIX and HSTS, which has the side-effect of
ensuring
> that we'd only hit this throttle for redirects (or cases in which the user
> explicitly allowed mixed content, which is vanishingly small). :)

The main problem I see with this solution is that we'll have to appropriately
handle the combinations of "where does the request started" with "is the request
allowed or blocked":

a) Request starts on the browser then goes to the renderer (PlzNavigate):
	1) Request is blocked on the browser; renderer never checks mixed content.
	2) Request is allowed by the browser; renderer executes the check too.
b) Request starts on the renderer then goes to the browser (most/all (?) cases
today):
	1) Request is blocked on the renderer; browser never checks mixed content.
	2) Request is allowed by the renderer; browser executes the check too.

So in cases a1 and b1 where only one side is actually run, all reporting --
encompassing any sort of signals sent, not only UseCounter -- must happen at
that side. This means that both sides must must be able to do the reporting.
Moreover, for a2 and b2 where both sides run the code, one of them will have to
run "silent", doing no reporting at all.

To be able to support this we have to be able to properly detect in which final
combination we are, both in the browser and renderer. For the
where-did-the-navigation-start variable the
NavigationHandle::IsRendererInitiated is a good signal but I'm unsure if that's
enough. I don't know if there is an equivalent flag on the renderer side. And to
support the will-this-be-blocked variable we'd have to holding reporting until
after we know if that request will be allowed or not.

Beyond my uncertainties about this being even feasible, it seems this would make
the code change a lot and become much more complicated.

Finally, as it seems like the long-term plan is to move mixed content checks to
the browser, isn't it more natural to also HSTS working there? That would
certainly simplify this situation.

carlosk

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

3 years, 10 months ago (2017-01-28 02:19:24 UTC) #197

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-28 02:19:58 UTC) #198

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-28 02:22:10 UTC) #199

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/144128) ios-device-xcode-clang on ...

3 years, 10 months ago (2017-01-28 02:22:12 UTC) #200

carlosk

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

3 years, 10 months ago (2017-01-28 02:35:02 UTC) #202

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-28 02:35:28 UTC) #203

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-28 02:38:44 UTC) #204

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/352789) linux_chromium_tsan_rel_ng on ...

3 years, 10 months ago (2017-01-28 02:38:45 UTC) #205

carlosk

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

3 years, 10 months ago (2017-01-30 21:34:51 UTC) #206

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-30 21:35:31 UTC) #207

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-30 23:56:59 UTC) #210

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-30 23:57:02 UTC) #211

carlosk

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

3 years, 10 months ago (2017-01-31 02:37:22 UTC) #213

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 02:37:57 UTC) #214

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 04:40:45 UTC) #215

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 04:40:47 UTC) #216

carlosk

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

3 years, 10 months ago (2017-02-08 02:56:24 UTC) #219

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 02:57:01 UTC) #220

carlosk

On 2017/01/28 02:09:44, carlosk wrote: > I'm still investigating nasko@'s suggestions in regards to the ...

3 years, 10 months ago (2017-02-08 02:59:03 UTC) #221

On 2017/01/28 02:09:44, carlosk wrote:
> I'm still investigating nasko@'s suggestions in regards to the use of
> url::Origin, consolidating IsPotentiallyTrustworthyOrigin and the Origin
> uniqueness checks, what is taking longer.

All of these points seemed to have been solved. Yay!
But I have to say I’m no expert in either of them and I am relying exclusively
on what our trybots can detect. So PTAL at these changes and let me know if you
see any issues.

> But I thought a lot about mkwst@'s suggestion and I wanted to address it
before
> anything else because I think it may require significant changes to this
patch. 
> 
> On 2017/01/25 11:37:30, Mike West (sloooooow) wrote:
> > > Why do you think this triggers only on redirects?
> > 
> > Because I think you should remove the early-exit for frame navigations that
> > we're talking about in `MixedContentChecker.cpp` in order to make sure that
we
> > don't change the ordering of MIX and HSTS, which has the side-effect of
> ensuring
> > that we'd only hit this throttle for redirects (or cases in which the user
> > explicitly allowed mixed content, which is vanishingly small). :)
> 
> The main problem I see with this solution is that we'll have to appropriately
> handle the combinations of "where does the request started" with "is the
request
> allowed or blocked":
> 
> a) Request starts on the browser then goes to the renderer (PlzNavigate):
> 	1) Request is blocked on the browser; renderer never checks mixed content.
> 	2) Request is allowed by the browser; renderer executes the check too.
> b) Request starts on the renderer then goes to the browser (most/all (?) cases
> today):
> 	1) Request is blocked on the renderer; browser never checks mixed content.
> 	2) Request is allowed by the renderer; browser executes the check too.
> 
> So in cases a1 and b1 where only one side is actually run, all reporting --
> encompassing any sort of signals sent, not only UseCounter -- must happen at
> that side. This means that both sides must must be able to do the reporting.
> Moreover, for a2 and b2 where both sides run the code, one of them will have
to
> run "silent", doing no reporting at all.
> 
> To be able to support this we have to be able to properly detect in which
final
> combination we are, both in the browser and renderer. For the
> where-did-the-navigation-start variable the
> NavigationHandle::IsRendererInitiated is a good signal but I'm unsure if
that's
> enough. I don't know if there is an equivalent flag on the renderer side. And
to
> support the will-this-be-blocked variable we'd have to holding reporting until
> after we know if that request will be allowed or not.
> 
> Beyond my uncertainties about this being even feasible, it seems this would
make
> the code change a lot and become much more complicated.
> 
> Finally, as it seems like the long-term plan is to move mixed content checks
to
> the browser, isn't it more natural to also HSTS working there? That would
> certainly simplify this situation.

During an offline meeting we agreed to keep running the checks isolatedly
between the browser and the renderer because:
a) This is only enabled for PlzNavigate and won't affect the current behavior.
b) HSTS will be implemented as a throttle in the browser and that will solve the
HSTS/mixed-content ordering issue (this now becomes a requirement for
PlzNavigate to be launched).

Given that, we also agreed that the UseCounter reporting could stay as is as the
browser side only usage will happen more often.

(we didn’t keep notes during the meeting so please let me know if I missed
anything)

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:48: if
(!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
On 2017/01/23 22:32:37, nasko (very slow) wrote:
> On 2017/01/21 02:54:58, carlosk wrote:
> > On 2017/01/12 18:32:37, nasko wrote:
> > > What happens if the commit was for an error page?
> > 
> > In PlzNavigate, the RenderFrame that would render the error page would be
the
> > same as if the navigation had succeeded: either the current one if it was a
> > same-site navigation or a new one if it was cross-site. I confirmed this
with
> > clamy@. So letting this execute in the error page case should be the right
> thing
> > to do.
> 
> Do we have a test that ensures that is the case and we don't regress it?

To clarify: this specific check will control if mixed contents user controlled
settings are reset or not in a navigation to an error page. Two points here:
- I don't think the expected behavior is specified anywhere.
- I don't think this is a very important case and that either behavior is
better.

There's no existing test that checks this. I did add this as an item to my list
of "tests to be added later".

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:224: //
MixedContentChecker::handleCertificateError.
On 2017/01/21 02:54:58, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > Should this be fixed before we consider mixed content checking browser side
> > complete? Maybe put a crbug link here.
> 
> I am honestly unsure about this. This is checked mainly when the
> "DidReceiveResponse" chain of calls happen on the renderer which I think
happen
> normally even with PlzNavigate enabled. For PlzNavigate* the reason for moving
> mixed content checking to the browser is to avoid extra IPC-round trips to the
> renderer and *I think* that doesn't apply for this method.
> 
> WDYT? Should we involve clamy@ in this matter?
> 
> I clarified the comment a little and added a reference to the main issue of
this
> very CL. I also updated that issue's description to explain this.
> 
> (*): Mind that OOPIF has different reasons to want to move mixed content
checks
> to the browser so it *could* be a requirement from their side.

Bumping this comment as I had not specific response and it might be important.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.h:67: };
On 2017/01/25 11:37:30, Mike West (sloooooow) wrote:
> On 2017/01/21 at 02:54:59, carlosk wrote:
> > On 2017/01/13 13:20:00, Mike West (sloooooow) wrote:
> > > I know we talked about this a while ago, but looking at the code, I think
> I'm
> > > convinced that it does more harm than good to try to jam these metrics
into
> the
> > > existing use counter. Especially given that this only triggers in redirect
> > > cases, I don't think the value of the information we're gathering here
> justifies
> > > the complexity.
> > >
> > > The only metric here I'm actively evaluating is
> > > `MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME`, which I don't think
> this
> > > patch actually affects (as we're gathering it after a response comes in).
> > > 
> > > I'd suggest removing `UseCounterFeature`, as well as
> `mixed_content_features_`
> > > and its usage throughout.
> > 
> > Why do you think this triggers only on redirects?
> 
> Because I think you should remove the early-exit for frame navigations that
> we're talking about in `MixedContentChecker.cpp` in order to make sure that we
> don't change the ordering of MIX and HSTS, which has the side-effect of
ensuring
> that we'd only hit this throttle for redirects (or cases in which the user
> explicitly allowed mixed content, which is vanishingly small). :)
> 
> > > I do want to keep the CSP reporting, as that's essential to helping
> developers
> > > migrate from HTTP to HTTPS, but that doesn't rely on the usecounter code
at
> all.
> > 
> > OK, this should be correctly done with the whole
> mixedContentFound(ByTheBrowser) IPC and call chain.
> 
> Yup!

I trimmed down the constants list to only contain the used ones.

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/navigation_handle_impl.cc:793: new
MixedContentNavigationThrottle(this));
On 2017/01/23 22:32:37, nasko (very slow) wrote:
> On 2017/01/21 02:54:59, carlosk wrote:
> > On 2017/01/12 18:32:37, nasko wrote:
> > > Does the ordering of when we add this matter? Would DevTools need to know
> and
> > be
> > > able to log the navigation before it is potentially blocked by the mixed
> > content
> > > checker?
> > 
> > I don't know the answer for this.
> > 
> > As this is a security check it seemed appropriate to move it up the chain
but
> > that's just me guessing. I created a new never-to-be-committed CL [1] where
I
> > moved this registration to be the last one and no trybots ever complained
> about
> > it.
> 
> Let's keep it first then, until we hear something breaking.
> 
> > I also noticed the established pattern here of concealing the creation
> decision
> > from each throttle within their classes by having a
> CreateThrottleForNavigation
> > method called from here. I updated my code to do the same.
> 
> Cool, thanks! That's definitely nice.
>  
> > Another thing I'm usure about is if I should register directly to
|throttles_|
> > or through |throttles_to_register| as both cases exist. I chose the latter
> > because 2 out of 3 cases do that...? :O
> 
> Yeah, what you did is good. We should follow up and make them all consistent.
>  
> > [1] https://codereview.chromium.org/2630043002
> 

OK, I'm keeping as is but just reaffirming: trybots didn't complain when I moved
it to being the last one.

I also added a TODO to make this code a bit cleaner.

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/public/browse...
content/public/browser/web_contents_delegate.h:567: virtual bool
ShouldAllowRunningInsecureContent(bool allowed_per_settings,
On 2017/01/23 22:32:37, nasko (very slow) wrote:
> On 2017/01/21 02:54:59, carlosk wrote:
> > On 2017/01/12 18:32:38, nasko wrote:
> > > nit: Shouldn't this be "allower_per_prefs"? When I read settings, I
thought
> > > immediately content settings, which are maintained by the chrome/ layer
and
> > got
> > > confused. Tracing the code, the value comes from WebPreferences.
> > 
> > I'm just following the pattern used in MixedContentsChecked for the mirror
API
> > [1]. I didn't understand your explanation.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/renderer/content_settings_observe...
> 
> My comment meant to say that the source of the input is WebPreferences and not
> ContentSettings, so naming the variable to indicate where it comes from makes
it
> easier to understand.

Understood this time and agreed. Changed all cases accordingly.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.cc
(right):

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.cc:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/01/23 22:32:38, nasko wrote:
> nit: no "(c)".

Done.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.cc:19: if
(tab->HasOpener()) {
On 2017/01/23 22:32:37, nasko wrote:
> nit: if (!tab->HasOpener()) return; will yield a bit easier to read code as it
> won't be as indented.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.h
(right):

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:1: //
Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/01/23 22:32:38, nasko wrote:
> nit: No "(c)" needed.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:15: //
Controls mixed content related settings for the associated, working as the
On 2017/01/23 22:32:38, nasko wrote:
> nit: associated what?

Done.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:71: bool
SecurityOriginIsSecure(const GURL& url) {
On 2017/01/23 22:32:38, nasko wrote:
> nit: IsOriginSecure? We don't have "SecurityOrigin" concept in browser code,
> which makes reading the name of this method a bit awkward.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:74:
HasPotentiallySecureScheme(*url.inner_url())) ||
On 2017/01/23 22:32:38, nasko wrote:
> Let's use a different code pattern for extracting the data from Filesystem and
> Blob URLs, so we are consistent across the codebase. Once we have one of
these,
> we can construct an url::Origin from the URL and check its scheme. An example
> can be seen in ExtensionNavigationThrottle::WillStartRequest.

Done.

But looking at this again made me concerned about the SecurityOrigin
"uniqueness" aspect that is queried in the Blink implementation when it calls to
SecurityPolicy::isOriginWhiteListedTrustworthy. I did a lot of changes and I
think it is now properly done in the browser too.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:90: // to
same-origin contexts, so they are not blocked either.
On 2017/01/23 22:32:38, nasko wrote:
> nit: no need for " either" as the rest of the comment from the Blink code is
not
> here and the "either" doesn't make much sense.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:95: //
SecurityOrigin::isPotentiallyTrustworthy without duplicating much of the
On 2017/01/23 22:32:38, nasko wrote:
> It is a bit sad to have this code and
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_re....
> Any chance we can combine the two somehow? Possibly in a separate CL?

Done and this method changed quite considerably because of that.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:100: if
(IsUniqueScheme(url) || HasLocalScheme(url) ||
On 2017/01/23 22:32:38, nasko (very slow) wrote:
> This doesn't match what isPotentiallyTrustworthy does, as it checks the
> uniqueness of the origin, which is not what this code does. You could very
well
> have an http(s) based document that is in a unique origin, through iframe
> sandboxing. Would this work correctly in those cases?

With my latest patch I tried to mirror the uniqueness check too.

I'm not sure of what would happen in the case you described. Should I add a new
to-test item to my ongoing list?

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:107:
net::IsLocalHostname(url.HostNoBrackets(), nullptr)) {
On 2017/01/23 22:32:38, nasko wrote:
> Why IsLocalHostname vs IsLocalhost above? 

This is due to a CL by mkwst@ that I had to mirror and caused the need to use of
both:
https://chromium.googlesource.com/chromium/src/+/130ee686fa00b617bfc001ceb3bb...

Note that the IsLocalhost call moved into IsPotentiallyTrustworthyOrigin.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:216: if
(!ShouldTreatURLSchemeAsCORSEnabled(handle_impl->GetURL())) {
On 2017/01/23 22:32:38, nasko wrote:
> nit: no need of {} for one line if statements.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:19:
const bool expectation;
On 2017/01/23 22:32:38, nasko wrote:
> nit: What is the meaning of "expectation"? Maybe rename it more explicitly
> "expected_mixed_content" or something like that.

Done.

https://codereview.chromium.org/1905033002/diff/1130001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle_unittest.cc:31:
{"https://example.com/foo", "blob:http://example.com/foo", false},
On 2017/01/23 22:32:38, nasko wrote:
> nit: I know this is how the code works, but this just feels wrong :(.

Acknowledged.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:217: const GURL&
origin_url = mixed_content_node->current_origin().GetURL();
nasko@: this was the only change related the concern of not using the correct
Origin for these checks. All other situations get the origin correctly from the
frame tree node.

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 04:44:56 UTC) #222

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-08 04:44:58 UTC) #223

nasko

Another round of comments. I'd lean on mkwst@ to ensure the mixed content implementation match ...

3 years, 10 months ago (2017-02-10 01:08:21 UTC) #224

Another round of comments. I'd lean on mkwst@ to ensure the mixed content
implementation match the Blink side.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:24: // level as Blink
does.
On 2017/01/21 02:54:58, carlosk wrote:
> On 2017/01/12 18:32:37, nasko wrote:
> > Hmm, I think we now track openers at the frame level even browser side. Do
we
> > want to have frame granularity here? If not, then the comment might be more
> > misleading than helpful the way it is now.
> 
> We want to keep frame granularity at tab-level as it is today.
> 
> I tried clarifying the comment. WDYT?
> 

Acknowledged.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:48: if
(!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
On 2017/02/08 02:59:02, carlosk wrote:
> On 2017/01/23 22:32:37, nasko (very slow) wrote:
> > On 2017/01/21 02:54:58, carlosk wrote:
> > > On 2017/01/12 18:32:37, nasko wrote:
> > > > What happens if the commit was for an error page?
> > > 
> > > In PlzNavigate, the RenderFrame that would render the error page would be
> the
> > > same as if the navigation had succeeded: either the current one if it was
a
> > > same-site navigation or a new one if it was cross-site. I confirmed this
> with
> > > clamy@. So letting this execute in the error page case should be the right
> > thing
> > > to do.
> > 
> > Do we have a test that ensures that is the case and we don't regress it?
> 
> To clarify: this specific check will control if mixed contents user controlled
> settings are reset or not in a navigation to an error page. Two points here:
> - I don't think the expected behavior is specified anywhere.
> - I don't think this is a very important case and that either behavior is
> better.
> 
> There's no existing test that checks this. I did add this as an item to my
list
> of "tests to be added later".

Ok. Where would one find that list?

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/content/browser/frame...
content/browser/frame_host/navigation_handle_impl.cc:793: new
MixedContentNavigationThrottle(this));
On 2017/02/08 02:59:02, carlosk wrote:
> On 2017/01/23 22:32:37, nasko (very slow) wrote:
> > On 2017/01/21 02:54:59, carlosk wrote:
> > > On 2017/01/12 18:32:37, nasko wrote:
> > > > Does the ordering of when we add this matter? Would DevTools need to
know
> > and
> > > be
> > > > able to log the navigation before it is potentially blocked by the mixed
> > > content
> > > > checker?
> > > 
> > > I don't know the answer for this.
> > > 
> > > As this is a security check it seemed appropriate to move it up the chain
> but
> > > that's just me guessing. I created a new never-to-be-committed CL [1]
where
> I
> > > moved this registration to be the last one and no trybots ever complained
> > about
> > > it.
> > 
> > Let's keep it first then, until we hear something breaking.
> > 
> > > I also noticed the established pattern here of concealing the creation
> > decision
> > > from each throttle within their classes by having a
> > CreateThrottleForNavigation
> > > method called from here. I updated my code to do the same.
> > 
> > Cool, thanks! That's definitely nice.
> >  
> > > Another thing I'm usure about is if I should register directly to
> |throttles_|
> > > or through |throttles_to_register| as both cases exist. I chose the latter
> > > because 2 out of 3 cases do that...? :O
> > 
> > Yeah, what you did is good. We should follow up and make them all
consistent.
> >  
> > > [1] https://codereview.chromium.org/2630043002
> > 
> 
> OK, I'm keeping as is but just reaffirming: trybots didn't complain when I
moved
> it to being the last one.
> 
> I also added a TODO to make this code a bit cleaner.

Acknowledged.

https://codereview.chromium.org/1905033002/diff/1350001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.h
(right):

https://codereview.chromium.org/1905033002/diff/1350001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:15: //
Controls mixed content related settings for the associated WebContent,
nit: WebContents

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:37: bool
HasPotentiallySecureScheme(const GURL& url) {
Do we still need this one if it is only used in one place?

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:43: bool
ShouldTreatURLSchemeAsCORSEnabled(const GURL& url) {
Same for this one, it is only used in one place.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:90: void
UpdateRendererOnMixedContentFound(NavigationHandleImpl* handle_impl,
nit: navigation_handle

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:129: return
should_block ? ThrottleCheckResult::CANCEL
Should this be a cancel or a block?

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:165: 
nit: No need for an empty line.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:217: const GURL&
origin_url = mixed_content_node->current_origin().GetURL();
On 2017/02/08 02:59:03, carlosk wrote:
> nasko@: this was the only change related the concern of not using the correct
> Origin for these checks. All other situations get the origin correctly from
the
> frame tree node.

Acknowledged.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/web_c...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/web_c...
content/browser/web_contents/web_contents_impl.cc:3570: const GURL& target_url)
{
Huh, we pass target_url just for logging?!

carlosk

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

3 years, 10 months ago (2017-02-11 01:38:11 UTC) #225

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-11 01:38:43 UTC) #226

carlosk

Thanks. https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/content_settings/mixed_content_settings.cc File chrome/browser/content_settings/mixed_content_settings.cc (right): https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/content_settings/mixed_content_settings.cc#newcode48 chrome/browser/content_settings/mixed_content_settings.cc:48: if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) On 2017/02/10 01:08:20, nasko ...

3 years, 10 months ago (2017-02-11 01:40:22 UTC) #227

Thanks.

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings.cc (right):

https://codereview.chromium.org/1905033002/diff/1070001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings.cc:48: if
(!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
On 2017/02/10 01:08:20, nasko wrote:
> On 2017/02/08 02:59:02, carlosk wrote:
> > On 2017/01/23 22:32:37, nasko (very slow) wrote:
> > > On 2017/01/21 02:54:58, carlosk wrote:
> > > > On 2017/01/12 18:32:37, nasko wrote:
> > > > > What happens if the commit was for an error page?
> > > > 
> > > > In PlzNavigate, the RenderFrame that would render the error page would
be
> > the
> > > > same as if the navigation had succeeded: either the current one if it
was
> a
> > > > same-site navigation or a new one if it was cross-site. I confirmed this
> > with
> > > > clamy@. So letting this execute in the error page case should be the
right
> > > thing
> > > > to do.
> > > 
> > > Do we have a test that ensures that is the case and we don't regress it?
> > 
> > To clarify: this specific check will control if mixed contents user
controlled
> > settings are reset or not in a navigation to an error page. Two points here:
> > - I don't think the expected behavior is specified anywhere.
> > - I don't think this is a very important case and that either behavior is
> > better.
> > 
> > There's no existing test that checks this. I did add this as an item to my
> list
> > of "tests to be added later".
> 
> Ok. Where would one find that list?

It's currently in a task in my Asana task tracker. I will move it out of there
and into a doc soon(TM).

https://codereview.chromium.org/1905033002/diff/1350001/chrome/browser/conten...
File chrome/browser/content_settings/mixed_content_settings_tab_helper.h
(right):

https://codereview.chromium.org/1905033002/diff/1350001/chrome/browser/conten...
chrome/browser/content_settings/mixed_content_settings_tab_helper.h:15: //
Controls mixed content related settings for the associated WebContent,
On 2017/02/10 01:08:20, nasko wrote:
> nit: WebContents

Done.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:37: bool
HasPotentiallySecureScheme(const GURL& url) {
On 2017/02/10 01:08:20, nasko wrote:
> Do we still need this one if it is only used in one place?

See below.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:43: bool
ShouldTreatURLSchemeAsCORSEnabled(const GURL& url) {
On 2017/02/10 01:08:20, nasko wrote:
> Same for this one, it is only used in one place.

I preferred keeping these here to make it clear they mirror specific Blink
methods.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:90: void
UpdateRendererOnMixedContentFound(NavigationHandleImpl* handle_impl,
On 2017/02/10 01:08:20, nasko wrote:
> nit: navigation_handle

Done.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:129: return
should_block ? ThrottleCheckResult::CANCEL
On 2017/02/10 01:08:20, nasko wrote:
> Should this be a cancel or a block?

We tested this together and figured it should indeed be cancel given the page
behavior: it stays the same (no error page), only a message is logged to the
console.

In any case: mkwst@, can you confirm if this is the right thing to do?

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:165: 
On 2017/02/10 01:08:21, nasko wrote:
> nit: No need for an empty line.

Done.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/web_c...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/web_c...
content/browser/web_contents/web_contents_impl.cc:3570: const GURL& target_url)
{
On 2017/02/10 01:08:21, nasko wrote:
> Huh, we pass target_url just for logging?!

Yes but a) this is the current behavior and b) see the TODO above.

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-11 07:39:54 UTC) #228

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/309270) linux_chromium_rel_ng on ...

3 years, 10 months ago (2017-02-11 07:39:56 UTC) #229

Mike West

lgtm https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode39 content/browser/frame_host/mixed_content_navigation_throttle.cc:39: } The comments say that these two functions ...

3 years, 10 months ago (2017-02-13 13:17:44 UTC) #230

Mike West

Sorry, I hit "Quick LGTM" instead of "Reply". I'd actually like answers to some things: ...

3 years, 10 months ago (2017-02-13 13:21:56 UTC) #231

carlosk

Description was changed from ========== Move navigation-level mixed content checks to the browser. This is ...

3 years, 10 months ago (2017-02-13 22:22:09 UTC) #232

carlosk

Description was changed from ========== PlzNavigate: Move navigation-level mixed content checks to the browser. This ...

3 years, 10 months ago (2017-02-13 22:23:51 UTC) #233

Description was changed from

==========
PlzNavigate: Move navigation-level mixed content checks to the browser.

This is a step towards moving to the browser some navigation security checks
currently made in the renderer. This is needed for PlzNavigate to function
properly, avoid extra delays caused by unnecessary IPC exchanges and might pave
the way for similar changes needed by OOPIF.

This change only affects PlzNavigate; the current implementation is unaffected.
In that context, note that this is only a partial move of the checks currently
performed by MixedContentChecker: only frame navigation resource loads will be
intercepted by the browser, by the newly created MixedContentNavigationThrottle.

And for this implementation to work correctly with HSTS it requires an
implementation of the latter as a navigation throttle. This is now a requirement
for PlzNavigate to be launched as otherwise HSTS checks would be broken.

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

to

==========
PlzNavigate: Move navigation-level mixed content checks to the browser.

This is a step towards moving to the browser some navigation security checks
currently made in the renderer. This is needed for PlzNavigate to function
properly, avoid extra delays caused by unnecessary IPC exchanges and might pave
the way for similar changes needed by OOPIF.

This change only affects PlzNavigate; the current implementation is unaffected.
In that context, note that this is only a partial move of the checks currently
performed by MixedContentChecker: only frame navigation resource loads will be
intercepted by the browser, by the newly created MixedContentNavigationThrottle.

And for this implementation to work correctly with HSTS it requires an
implementation of the latter as a navigation throttle. This is now a requirement
for PlzNavigate to be launched as otherwise HSTS checks would be broken.

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

carlosk

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

3 years, 10 months ago (2017-02-13 22:53:07 UTC) #234

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-13 22:53:54 UTC) #235

carlosk

On 2017/02/13 13:21:56, Mike West (sloooooow) wrote: > In general the WebKit side of the ...

3 years, 10 months ago (2017-02-14 01:42:59 UTC) #236

On 2017/02/13 13:21:56, Mike West (sloooooow) wrote:
> In general the WebKit side of the patch looks reasonable, and the Throttle
> implementation looks correct. Passing the existing tests gives me enough
> confidence in that. We should be pulling in the web-platform-test set of mixed
> content checks shortly as well, which will give us even more confidence
> (+foolip@: This is why I was asking you about that delayed import job... :) ).

Awesome! But while reading your comments I noticed that my description was still
outdated and mentioned this patch would always be active and that was wrong.
*This patch is only active when PlzNavigate is enabled!* I updated the
description now.

But what matters in what you are saying is that for web-platform-test to
actually test this implementation we will need to run them with the
--enable-browser-side-navigation command line switch.

> That said, the HSTS/MIX ordering is a problem that I'd like to see an answer
to.
> Landing this as-is would change our web-facing behavior. I'm not in favor of
> doing that. I'd like to understand what y'all's plan is. I'd also like a
> reminder about why simply removing the Blink-side check isn't enough. We
talked
> about it in MTV, but I didn't record the result of that discussion, and the
> details are lost to me. :(

With this patch no web-facing change should happen for the current navigation
implementation. It will only have any effect when PlzNavigate is enabled.

From a preview message, this is how I recall our conclusions from that meeting:

"""
During an offline meeting we agreed to keep running the checks isolatedly
between the browser and the renderer because:
a) This is only enabled for PlzNavigate and won't affect the current behavior.
b) HSTS will be implemented as a throttle in the browser and that will solve the
HSTS/mixed-content ordering issue (this now becomes a requirement for
PlzNavigate to be launched).

Given that, we also agreed that the UseCounter reporting could stay as is as the
browser side only usage will happen more often.

(we didn’t keep notes during the meeting so please let me know if I missed
anything)
"""

Does this answers your concerns?

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:37: bool
HasPotentiallySecureScheme(const GURL& url) {
On 2017/02/11 01:40:22, carlosk wrote:
> On 2017/02/10 01:08:20, nasko wrote:
> > Do we still need this one if it is only used in one place?
> 
> See below.

For this one it indeed makes sense to eliminate it as it's just a wrapper around
the previous one.

https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:129: return
should_block ? ThrottleCheckResult::CANCEL
On 2017/02/11 01:40:22, carlosk wrote:
> On 2017/02/10 01:08:20, nasko wrote:
> > Should this be a cancel or a block?
> 
> We tested this together and figured it should indeed be cancel given the page
> behavior: it stays the same (no error page), only a message is logged to the
> console.
> 
> In any case: mkwst@, can you confirm if this is the right thing to do?

mkwst@: could you PTAL at this?

More details: AFAICT the difference here between a CANCEL and a BLOCK_REQUEST is
that the final error code would change from net::ERR_ABORTED to
net::ERR_BLOCKED_BY_CLIENT, respectively. On the Blink side the error resource
created when mixed content is blocked used the same error code as CANCEL. So my
conclusion is that things should stay as is.

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:39: }
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> The comments say that these two functions are the same. I don't think they're
> the same?

They were as the 2nd was just a GURL helper for the first. But following an
earlier suggestion by nasko@ I removed it.

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:75: //
potentially trustworthy.
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> Nit: Please change this TODO to something like "Remove this once we implement
> https://tools.ietf.org/html/draft-west-let-localhost-be-localhost.%22

I updated the comment. Is there a crbug for it?

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:86: bool
DoesOriginSchemeRestricsMixedContent(const url::Origin& origin) {
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> Nit: Typo: s/Restrics/Restrict/

Done.

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:207: bool
should_ask_embedder =
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> We're in the embedder, right?

We're in content/ here. My understanding is that the embedder is Chrome, who
implements ShouldAllowRunningInsecureContent, that is called next only if
|should_ask_embedder| is true.

But indeed this is misleading here and that method is implemented by a few
different classes. I renamed it to refer to the delegate instead.

https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:302: }
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> As we've discussed, landing this change will invert the web-visible behavior
of
> MIX and HSTS (for both PlzNavigate and status quo). What's the plan for
> remedying that?

This change will not affect the current navigation implementation; it
falls-through in case PlzNavigate is not enabled. My understanding from our
meeting is that the remediation for PlzNavigate will be to have HSTS working as
a throttle implementation in the browser.

> Could you add some justification to the CL description as to why we're not
doing
> an initial check on the Blink-side before sending things up to the browser?

My CL description was outdated: it still said this implementation would be
active for both the current navigation implementation and for PlzNavigate. *That
is not true anymore!*. I updated it accordingly.

I might not be understanding what you mean but this is what I did:
* The comment above these lines already explains why there's this early return
here.
* And there's this statement in the description: "(...) note that this is only a
partial move of the checks currently performed by MixedContentChecker: only
frame navigation resource loads will be intercepted by the browser, by the newly
created MixedContentNavigationThrottle."

https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:587: if (policy) {
On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> Is it possible for this to be false? Shouldn't every security context have a
> policy?

This mimics the way these same reports are implemented in MixedContentChecker.

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-14 02:46:05 UTC) #237

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-14 02:46:07 UTC) #238

Mike West

//third_party/WebKit and //content/shell LGTM. IPC too, FWIW. The throttle looks like it'll produce the correct ...

3 years, 10 months ago (2017-02-14 07:37:52 UTC) #239

//third_party/WebKit and //content/shell LGTM. IPC too, FWIW.

The throttle looks like it'll produce the correct behavior, and the fact that
you're passing all the existing LayoutTests is enough to make me confident that
you're doing the right thing. :)

>
https://codereview.chromium.org/1905033002/diff/1350001/content/browser/frame...
> content/browser/frame_host/mixed_content_navigation_throttle.cc:129: return
should_block ? ThrottleCheckResult::CANCEL
> On 2017/02/11 01:40:22, carlosk wrote:
> > On 2017/02/10 01:08:20, nasko wrote:
> > > Should this be a cancel or a block?
> > 
> > We tested this together and figured it should indeed be cancel given the
page
> > behavior: it stays the same (no error page), only a message is logged to the
> > console.
> > 
> > In any case: mkwst@, can you confirm if this is the right thing to do?
> 
> mkwst@: could you PTAL at this?
> 
> More details: AFAICT the difference here between a CANCEL and a BLOCK_REQUEST
is that the final error code would change from net::ERR_ABORTED to
net::ERR_BLOCKED_BY_CLIENT, respectively. On the Blink side the error resource
created when mixed content is blocked used the same error code as CANCEL. So my
conclusion is that things should stay as is.

Cancelling the request seems fine for the moment. I think it would be nice if we
eventually changed this to something that would generate a unique error code so
that we could have a nice error page displayed to the user rather than a blank
iframe, but that's a nice-to-have for the future, not a requirement for this
patch.

>
https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
> content/browser/frame_host/mixed_content_navigation_throttle.cc:75: //
potentially trustworthy.
> On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> > Nit: Please change this TODO to something like "Remove this once we
implement
> > https://tools.ietf.org/html/draft-west-let-localhost-be-localhost.%22
> 
> I updated the comment. Is there a crbug for it?

There is now: https://crbug.com/691930

>
https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
> third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:302: }
> On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> > As we've discussed, landing this change will invert the web-visible behavior
of
> > MIX and HSTS (for both PlzNavigate and status quo). What's the plan for
> > remedying that?
> 
> This change will not affect the current navigation implementation; it
falls-through in case PlzNavigate is not enabled. My understanding from our
meeting is that the remediation for PlzNavigate will be to have HSTS working as
a throttle implementation in the browser.

Got it. I missed the PlzNavigate check on the Blink side.

Have you already filed a bug for that work? If so, please add it in as a TODO.

carlosk

Thanks! https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode75 content/browser/frame_host/mixed_content_navigation_throttle.cc:75: // potentially trustworthy. On 2017/02/14 01:42:59, carlosk wrote: ...

3 years, 10 months ago (2017-02-14 19:32:15 UTC) #240

Thanks!

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
File content/browser/frame_host/mixed_content_navigation_throttle.cc (right):

https://codereview.chromium.org/1905033002/diff/1390001/content/browser/frame...
content/browser/frame_host/mixed_content_navigation_throttle.cc:75: //
potentially trustworthy.
On 2017/02/14 01:42:59, carlosk wrote:
> On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> > Nit: Please change this TODO to something like "Remove this once we
implement
> > https://tools.ietf.org/html/draft-west-let-localhost-be-localhost.%22
> 
> I updated the comment. Is there a crbug for it?

Added reference to https://crbug.com/691930.

https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1905033002/diff/1390001/third_party/WebKit/So...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:302: }
On 2017/02/14 01:42:59, carlosk wrote:
> On 2017/02/13 13:17:44, Mike West (sloooooow) wrote:
> > As we've discussed, landing this change will invert the web-visible behavior
> of
> > MIX and HSTS (for both PlzNavigate and status quo). What's the plan for
> > remedying that?
> 
> This change will not affect the current navigation implementation; it
> falls-through in case PlzNavigate is not enabled. My understanding from our
> meeting is that the remediation for PlzNavigate will be to have HSTS working
as
> a throttle implementation in the browser.
> 
> > Could you add some justification to the CL description as to why we're not
> doing
> > an initial check on the Blink-side before sending things up to the browser?
> 
> My CL description was outdated: it still said this implementation would be
> active for both the current navigation implementation and for PlzNavigate.
*That
> is not true anymore!*. I updated it accordingly.
> 
> I might not be understanding what you mean but this is what I did:
> * The comment above these lines already explains why there's this early return
> here.
> * And there's this statement in the description: "(...) note that this is only
a
> partial move of the checks currently performed by MixedContentChecker: only
> frame navigation resource loads will be intercepted by the browser, by the
newly
> created MixedContentNavigationThrottle."

Filed https://crbug.com/692157 to track the HSTS throttle work and added a TODO
to mixed_content_navigation_throttle.h.

clamy

Note: I'm removing myself from the review since I did not follow the patch at ...

3 years, 10 months ago (2017-02-15 16:34:55 UTC) #241

clamy

clamy@chromium.org changed reviewers: - clamy@chromium.org

3 years, 10 months ago (2017-02-15 16:35:49 UTC) #242

nasko

Phew! Some nits, but nothing else. LGTM! https://codereview.chromium.org/1905033002/diff/1430001/content/browser/frame_host/mixed_content_navigation_throttle.cc File content/browser/frame_host/mixed_content_navigation_throttle.cc (right): https://codereview.chromium.org/1905033002/diff/1430001/content/browser/frame_host/mixed_content_navigation_throttle.cc#newcode82 content/browser/frame_host/mixed_content_navigation_throttle.cc:82: bool DoesOriginSchemeRestrictsMixedContent(const ...

3 years, 10 months ago (2017-02-15 17:41:39 UTC) #243

carlosk

Thanks! Just to be sure I did run locally an arbitrary subset of tests with ...

3 years, 10 months ago (2017-02-15 21:46:31 UTC) #244

carlosk

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, mkwst@chromium.org, nasko@chromium.org Link ...

3 years, 10 months ago (2017-02-15 21:47:40 UTC) #246

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-15 21:48:49 UTC) #247

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-15 23:26:42 UTC) #248

commit-bot: I haz the power

Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/119824)

3 years, 10 months ago (2017-02-15 23:26:45 UTC) #249

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-16 05:49:45 UTC) #251

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1450001, "attempt_start_ts": 1487224125000270, "parent_rev": "0c2fcf56d2df0bb7b31ccb1509662255b5852451", "commit_rev": "d9d979428e6e9301e3044b875de92af4fa96344a"}

3 years, 10 months ago (2017-02-16 08:58:14 UTC) #252

commit-bot: I haz the power

Description was changed from ========== PlzNavigate: Move navigation-level mixed content checks to the browser. This ...

3 years, 10 months ago (2017-02-16 08:59:00 UTC) #253

Message was sent while issue was closed.

Description was changed from

==========
PlzNavigate: Move navigation-level mixed content checks to the browser.

This is a step towards moving to the browser some navigation security checks
currently made in the renderer. This is needed for PlzNavigate to function
properly, avoid extra delays caused by unnecessary IPC exchanges and might pave
the way for similar changes needed by OOPIF.

This change only affects PlzNavigate; the current implementation is unaffected.
In that context, note that this is only a partial move of the checks currently
performed by MixedContentChecker: only frame navigation resource loads will be
intercepted by the browser, by the newly created MixedContentNavigationThrottle.

And for this implementation to work correctly with HSTS it requires an
implementation of the latter as a navigation throttle. This is now a requirement
for PlzNavigate to be launched as otherwise HSTS checks would be broken.

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

to

==========
PlzNavigate: Move navigation-level mixed content checks to the browser.

This is a step towards moving to the browser some navigation security checks
currently made in the renderer. This is needed for PlzNavigate to function
properly, avoid extra delays caused by unnecessary IPC exchanges and might pave
the way for similar changes needed by OOPIF.

This change only affects PlzNavigate; the current implementation is unaffected.
In that context, note that this is only a partial move of the checks currently
performed by MixedContentChecker: only frame navigation resource loads will be
intercepted by the browser, by the newly created MixedContentNavigationThrottle.

And for this implementation to work correctly with HSTS it requires an
implementation of the latter as a navigation throttle. This is now a requirement
for PlzNavigate to be launched as otherwise HSTS checks would be broken.

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

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

commit-bot: I haz the power

3 years, 10 months ago (2017-02-16 08:59:05 UTC) #254

Message was sent while issue was closed.

Committed patchset #56 (id:1450001) as
https://chromium.googlesource.com/chromium/src/+/d9d979428e6e9301e3044b875de9...

Issue 1905033002: PlzNavigate: Move navigation-level mixed content checks to the browser. (Closed)