Adapt MixedContentChecker for remote frames

Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936
Committed: https://crrev.com/56dc8e2191815bb5b64a5b81b5f1c4db7828b6ed
Cr-Commit-Position: refs/heads/master@{#371534}

[estark, 2015-12-28 22:46:12]

Description was changed from

==========
still# Enter a description of the change.
Adapt MixedContentChecker for remote frames

BUG=486936
==========

to

==========
Adapt MixedContentChecker for remote frames

BUG=486936
==========

[estark, 2016-01-07 19:49:50]

Description was changed from

==========
Adapt MixedContentChecker for remote frames

BUG=486936
==========

to

==========
Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936
==========

[alexmos, 2016-01-11 22:02:24]

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

[alexmos, 2016-01-11 22:02:24]

A couple of preliminary comments, but the CL looks good overall.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
nit: no need for indent.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
nit: comment explaining the need for this function?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:52: if
(!frame->isLocalFrame())
nit: frame->isRemoteFrame()

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:80: bool
MixedContentChecker::isMixedContent(SecurityOrigin* securityOrigin, const KURL&
url)
This is used in DOMWindow::postMessage, which has another TODO about mixed
content for OOPIF.  Do you mind checking if that call site needs any more work?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:307: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
Do you know why this didn't call effectiveFrameForFrameType previously?  Was
this a bug?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ?
toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame),
url, requestContext, allowed);
This feels a bit weird, in that the sometimes the message ends up on the parent
frame and sometimes on current frame (depending on whether OOPIFs are used). 
What was wrong with just logging on |frame|?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:485:
FrameLoaderClient* client = frame->loader().client();
nit: comment about why this changed from effectiveFrame to frame (elsewhere as
well?)

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:489:
client->didRunContentWithCertificateErrors(response.url(),
response.getSecurityInfo(), KURL(KURL(),
effectiveFrame->securityContext()->securityOrigin()->toString()),
localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());
Is it possible to reuse mainResourceUrlForConsoleLog here?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:495:
client->didDisplayContentWithCertificateErrors(response.url(),
response.getSecurityInfo(), KURL(KURL(),
effectiveFrame->securityContext()->securityOrigin()->toString()),
localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());
Ditto

[estark, 2016-01-14 23:05:49]

Addressed Alex's comments and started adding some basic browser tests to
site_per_process_browsertest.cc, though I'm not entirely sure they belong there.
One of these tests caught a bug in how the
|should_enforce_strict_mixed_content_checking| flag is passed around which I
fixed in frame_messages.h.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
On 2016/01/11 22:02:24, alexmos wrote:
> nit: no need for indent.

Hm, `git cl format` seems to undo it if I make that change.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
On 2016/01/11 22:02:24, alexmos wrote:
> nit: comment explaining the need for this function?  

Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:52: if
(!frame->isLocalFrame())
On 2016/01/11 22:02:24, alexmos wrote:
> nit: frame->isRemoteFrame()

Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:80: bool
MixedContentChecker::isMixedContent(SecurityOrigin* securityOrigin, const KURL&
url)
On 2016/01/11 22:02:24, alexmos wrote:
> This is used in DOMWindow::postMessage, which has another TODO about mixed
> content for OOPIF.  Do you mind checking if that call site needs any more
work?

To me it looks like everything should be fine there. Okay with you if I remove
the FIXME?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:307: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/11 22:02:24, alexmos wrote:
> Do you know why this didn't call effectiveFrameForFrameType previously?  Was
> this a bug?

The callsite (in FrameFetchContext.cpp) used to call it. I moved it in here so
that I'd always have access to a LocalFrame in here. Otherwise, |frame| might be
remote and we wouldn't have any LocalFrame in here.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ?
toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame),
url, requestContext, allowed);
On 2016/01/11 22:02:24, alexmos wrote:
> This feels a bit weird, in that the sometimes the message ends up on the
parent
> frame and sometimes on current frame (depending on whether OOPIFs are used). 
> What was wrong with just logging on |frame|?

I suppose always using |frame| makes sense, but it causes a slight regression:
when https://foo.com has an iframe to http://bar.com, the console message won't
say the line number, I guess because there is no line number for loading the
main resource for a frame. So e.g. the layout test results will be "CONSOLE
ERROR" instead of "CONSOLE ERROR (line X)". What do you think?

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:485:
FrameLoaderClient* client = frame->loader().client();
On 2016/01/11 22:02:24, alexmos wrote:
> nit: comment about why this changed from effectiveFrame to frame (elsewhere as
> well?)

Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:489:
client->didRunContentWithCertificateErrors(response.url(),
response.getSecurityInfo(), KURL(KURL(),
effectiveFrame->securityContext()->securityOrigin()->toString()),
localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());
On 2016/01/11 22:02:24, alexmos wrote:
> Is it possible to reuse mainResourceUrlForConsoleLog here?

Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:495:
client->didDisplayContentWithCertificateErrors(response.url(),
response.getSecurityInfo(), KURL(KURL(),
effectiveFrame->securityContext()->securityOrigin()->toString()),
localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());
On 2016/01/11 22:02:24, alexmos wrote:
> Ditto

Done.

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

https://codereview.chromium.org/1550723003/diff/120001/content/common/frame_m...
content/common/frame_messages.h:366: IPC_STRUCT_TRAITS_MEMBER(scope)
I'm not actually sure if this line is necessary... should this section only
contain fields that we care about passing across the IPC boundary, or is it
supposed to contain all the fields in the FrameReplicationState struct?

[alexmos, 2016-01-16 00:49:50]

[+site-isolation-reviews]

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
On 2016/01/14 23:05:48, estark wrote:
> On 2016/01/11 22:02:24, alexmos wrote:
> > nit: no need for indent.
> 
> Hm, `git cl format` seems to undo it if I make that change.

That's interesting/weird.  The Blink coding style seems to imply that the indent
isn't needed in this case: (https://www.chromium.org/blink/coding-style, part 3
in Indentation).  I've also never seen it indented in other code. :)

I've asked dcheng@, and he suggests that clang-format hasn't really been
well-tested with Blink code and shouldn't be trusted.  He also agrees that there
shouldn't be indent here, so it seems this may be a bug in how git cl format is
set up for Blink.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:80: bool
MixedContentChecker::isMixedContent(SecurityOrigin* securityOrigin, const KURL&
url)
On 2016/01/14 23:05:48, estark wrote:
> On 2016/01/11 22:02:24, alexmos wrote:
> > This is used in DOMWindow::postMessage, which has another TODO about mixed
> > content for OOPIF.  Do you mind checking if that call site needs any more
> work?
> 
> To me it looks like everything should be fine there. Okay with you if I remove
> the FIXME?

Yes, let's remove it.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:307: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/14 23:05:48, estark wrote:
> On 2016/01/11 22:02:24, alexmos wrote:
> > Do you know why this didn't call effectiveFrameForFrameType previously?  Was
> > this a bug?
> 
> The callsite (in FrameFetchContext.cpp) used to call it. I moved it in here so
> that I'd always have access to a LocalFrame in here. Otherwise, |frame| might
be
> remote and we wouldn't have any LocalFrame in here.

Ah yes, for FrameFetchContext that makes sense, but are other call sites ok with
this change?  E.g., the one in WebLocalFrameImpl::checkIfRunInsecureContent
seems like it may end up using the parent frame in new code but not in old code
- is this ok?  (There's also a comment there about that function used only for
NPAPI and going away, so this may not matter much).

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ?
toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame),
url, requestContext, allowed);
On 2016/01/14 23:05:48, estark wrote:
> On 2016/01/11 22:02:24, alexmos wrote:
> > This feels a bit weird, in that the sometimes the message ends up on the
> parent
> > frame and sometimes on current frame (depending on whether OOPIFs are used).

> > What was wrong with just logging on |frame|?
> 
> I suppose always using |frame| makes sense, but it causes a slight regression:
> when https://foo.com has an iframe to http://bar.com, the console message
won't
> say the line number, I guess because there is no line number for loading the
> main resource for a frame. So e.g. the layout test results will be "CONSOLE
> ERROR" instead of "CONSOLE ERROR (line X)". What do you think?

Ah, I see.  It would be nice if the test expectations matched up with and
without site isolation, as this will make running layout tests with OOPIFs
easier.  So my own slight preference would be to keep this as-is, but I'm not
sure we can get away with it (I'd also be ok if we restored the line number
behavior).  Maybe run this by mkwst@ to see if he's ok removing this?  Also, how
many layout tests are affected (I don't see this coming up in the expectation
changes you have so far)?

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4678:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PassiveMixedContent) {
Hmm, it doesn't look like this test exercises any OOPIF functionality, though it
does seem useful to have.  I'd suggest moving it to something like
web_contents_impl_browsertest.cc, except that it'd be nice to keep all of these
tests together.  Actually, do you think the other tests would be useful to run
without --site-per-process as well?  If so, something like
web_contents_impl_browsertest or render_frame_host_manager_browsertest (or even
a new test file) might be a good home for them.  These would run without
--site-per-process normally, and with --site-per-process on our FYI and try bots
(which should be enabled the CQ some time soon).

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4688:
IN_PROC_BROWSER_TEST_F(SitePerProcessIgnoreCertErrorsBrowserTest,
A comment about why these tests need to ignore cert errors would be nice.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4697:
EXPECT_TRUE(shell()->web_contents()->DisplayedInsecureContent());
Does navigating the subframe to another (secure) cross-site URL clear
DisplayedInsecureContent?  What about navigating the main frame?  This may be
worth checking in the test.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4709:
EXPECT_FALSE(shell()->web_contents()->DisplayedInsecureContent());
Maybe also check the strict mixed content flags stored on the FrameTreeNodes in
current_replication_state()?

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

https://codereview.chromium.org/1550723003/diff/120001/content/common/frame_m...
content/common/frame_messages.h:366: IPC_STRUCT_TRAITS_MEMBER(scope)
On 2016/01/14 23:05:49, estark wrote:
> I'm not actually sure if this line is necessary... should this section only
> contain fields that we care about passing across the IPC boundary, or is it
> supposed to contain all the fields in the FrameReplicationState struct?

Good catch!  Yes, all the fields of the struct should be declared here.  Not
having this might be breaking serialization/deserialization (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_r...),
logging
(https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_l...),
and who knows what else.  Not having |scope| there was a bug.

https://codereview.chromium.org/1550723003/diff/120001/content/test/data/mixe...
File content/test/data/mixed-content/basic-passive-in-iframe.html (right):

https://codereview.chromium.org/1550723003/diff/120001/content/test/data/mixe...
content/test/data/mixed-content/basic-passive-in-iframe.html:9: var url =
"https://a.com:" + window.location.port + "/mixed-content/basic-passive.html";
To embed cross-site iframes, we typically use something like <iframe
src="/cross-site/a.com/title1.html">, which is a bit cleaner.  (For example, see
frame_tree/page_with_two_frames.html.)  Would that work here, and with the HTTPS
server?  You probably will need to call SetupCrossSiteRedirector() on it.

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

https://codereview.chromium.org/1550723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:448:
mainResourceUrlForFrame(mixedFrame).elidedString().utf8().data(),
url.elidedString().utf8().data());
The messages for fetch and WebSocket cases seem to use |frame| for main resource
URL source, but this one now uses |mixedFrame|.  Is this ok?

[estark, 2016-01-20 05:56:22]

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:50: KURL
mainResourceUrlForConsoleLog(Frame* frame)
On 2016/01/16 00:49:49, alexmos wrote:
> On 2016/01/14 23:05:48, estark wrote:
> > On 2016/01/11 22:02:24, alexmos wrote:
> > > nit: no need for indent.
> > 
> > Hm, `git cl format` seems to undo it if I make that change.
> 
> That's interesting/weird.  The Blink coding style seems to imply that the
indent
> isn't needed in this case: (https://www.chromium.org/blink/coding-style, part
3
> in Indentation).  I've also never seen it indented in other code. :)
> 
> I've asked dcheng@, and he suggests that clang-format hasn't really been
> well-tested with Blink code and shouldn't be trusted.  He also agrees that
there
> shouldn't be indent here, so it seems this may be a bug in how git cl format
is
> set up for Blink.

Oh ok, good to know! Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:80: bool
MixedContentChecker::isMixedContent(SecurityOrigin* securityOrigin, const KURL&
url)
On 2016/01/16 00:49:49, alexmos wrote:
> On 2016/01/14 23:05:48, estark wrote:
> > On 2016/01/11 22:02:24, alexmos wrote:
> > > This is used in DOMWindow::postMessage, which has another TODO about mixed
> > > content for OOPIF.  Do you mind checking if that call site needs any more
> > work?
> > 
> > To me it looks like everything should be fine there. Okay with you if I
remove
> > the FIXME?
> 
> Yes, let's remove it.

Done.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:307: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/16 00:49:50, alexmos wrote:
> On 2016/01/14 23:05:48, estark wrote:
> > On 2016/01/11 22:02:24, alexmos wrote:
> > > Do you know why this didn't call effectiveFrameForFrameType previously? 
Was
> > > this a bug?
> > 
> > The callsite (in FrameFetchContext.cpp) used to call it. I moved it in here
so
> > that I'd always have access to a LocalFrame in here. Otherwise, |frame|
might
> be
> > remote and we wouldn't have any LocalFrame in here.
> 
> Ah yes, for FrameFetchContext that makes sense, but are other call sites ok
with
> this change?  E.g., the one in WebLocalFrameImpl::checkIfRunInsecureContent
> seems like it may end up using the parent frame in new code but not in old
code
> - is this ok?  (There's also a comment there about that function used only for
> NPAPI and going away, so this may not matter much).

Ahh, I missed that one, thanks. (I think it shouldn't matter for the other call
sites).

I'm not sure about this because I don't quite understand why that call site uses
FrameTypeNested; I'll ask Mike when he takes a look at this CL.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ?
toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame),
url, requestContext, allowed);
On 2016/01/16 00:49:49, alexmos wrote:
> On 2016/01/14 23:05:48, estark wrote:
> > On 2016/01/11 22:02:24, alexmos wrote:
> > > This feels a bit weird, in that the sometimes the message ends up on the
> > parent
> > > frame and sometimes on current frame (depending on whether OOPIFs are
used).
> 
> > > What was wrong with just logging on |frame|?
> > 
> > I suppose always using |frame| makes sense, but it causes a slight
regression:
> > when https://foo.com has an iframe to http://bar.com, the console message
> won't
> > say the line number, I guess because there is no line number for loading the
> > main resource for a frame. So e.g. the layout test results will be "CONSOLE
> > ERROR" instead of "CONSOLE ERROR (line X)". What do you think?
> 
> Ah, I see.  It would be nice if the test expectations matched up with and
> without site isolation, as this will make running layout tests with OOPIFs
> easier.  So my own slight preference would be to keep this as-is, but I'm not
> sure we can get away with it (I'd also be ok if we restored the line number
> behavior).  Maybe run this by mkwst@ to see if he's ok removing this?  Also,
how
> many layout tests are affected (I don't see this coming up in the expectation
> changes you have so far)?

Ok, yeah, I agree that it would be nice if the test expectations were
consistent, so I'll add this to my ask-Mike list. I think it'll affect about 8
layout tests (I didn't make the changes yet in case we want to change it back),
but I'm kicking off a trybot run to make sure there aren't any others that I
didn't notice.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4678:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PassiveMixedContent) {
On 2016/01/16 00:49:50, alexmos wrote:
> Hmm, it doesn't look like this test exercises any OOPIF functionality, though
it
> does seem useful to have.  I'd suggest moving it to something like
> web_contents_impl_browsertest.cc, except that it'd be nice to keep all of
these
> tests together.  Actually, do you think the other tests would be useful to run
> without --site-per-process as well?  If so, something like
> web_contents_impl_browsertest or render_frame_host_manager_browsertest (or
even
> a new test file) might be a good home for them.  These would run without
> --site-per-process normally, and with --site-per-process on our FYI and try
bots
> (which should be enabled the CQ some time soon).

Hmm... actually, maybe we don't need this test at all, since as you say, it
doesn't really test OOPIFs, and mixed content in general is covered pretty well
by layout tests. So maybe I should just remove this one? For the rest of the
tests I added here, it certainly wouldn't hurt to run them without
--site-per-process, but I do think we already have non-site-per-process test
coverage (in the form of layout tests) for this same functionality. So, maybe
they should stay here and only run for --site-per-process? What do you think?

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4688:
IN_PROC_BROWSER_TEST_F(SitePerProcessIgnoreCertErrorsBrowserTest,
On 2016/01/16 00:49:50, alexmos wrote:
> A comment about why these tests need to ignore cert errors would be nice.

Done.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4697:
EXPECT_TRUE(shell()->web_contents()->DisplayedInsecureContent());
On 2016/01/16 00:49:50, alexmos wrote:
> Does navigating the subframe to another (secure) cross-site URL clear
> DisplayedInsecureContent?  What about navigating the main frame?  This may be
> worth checking in the test.

Done.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4709:
EXPECT_FALSE(shell()->web_contents()->DisplayedInsecureContent());
On 2016/01/16 00:49:50, alexmos wrote:
> Maybe also check the strict mixed content flags stored on the FrameTreeNodes
in
> current_replication_state()?

Done.

https://codereview.chromium.org/1550723003/diff/120001/content/test/data/mixe...
File content/test/data/mixed-content/basic-passive-in-iframe.html (right):

https://codereview.chromium.org/1550723003/diff/120001/content/test/data/mixe...
content/test/data/mixed-content/basic-passive-in-iframe.html:9: var url =
"https://a.com:" + window.location.port + "/mixed-content/basic-passive.html";
On 2016/01/16 00:49:50, alexmos wrote:
> To embed cross-site iframes, we typically use something like <iframe
> src="/cross-site/a.com/title1.html">, which is a bit cleaner.  (For example,
see
> frame_tree/page_with_two_frames.html.)  Would that work here, and with the
HTTPS
> server?  You probably will need to call SetupCrossSiteRedirector() on it.

Yeah, works like a charm! Done.

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

https://codereview.chromium.org/1550723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:448:
mainResourceUrlForFrame(mixedFrame).elidedString().utf8().data(),
url.elidedString().utf8().data());
On 2016/01/16 00:49:50, alexmos wrote:
> The messages for fetch and WebSocket cases seem to use |frame| for main
resource
> URL source, but this one now uses |mixedFrame|.  Is this ok?

I think the messages for fetch and WebSocket also use |mixedFrame| -- line 377
for example passes |mixedFrame| into mainResourceUrlForFrame(). Am I
misunderstanding the question?

[alexmos, 2016-01-20 22:43:55]

Thanks, just a couple of nits below and a question about active mixed content
tests.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:307: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/20 05:56:22, estark wrote:
> On 2016/01/16 00:49:50, alexmos wrote:
> > On 2016/01/14 23:05:48, estark wrote:
> > > On 2016/01/11 22:02:24, alexmos wrote:
> > > > Do you know why this didn't call effectiveFrameForFrameType previously? 
> Was
> > > > this a bug?
> > > 
> > > The callsite (in FrameFetchContext.cpp) used to call it. I moved it in
here
> so
> > > that I'd always have access to a LocalFrame in here. Otherwise, |frame|
> might
> > be
> > > remote and we wouldn't have any LocalFrame in here.
> > 
> > Ah yes, for FrameFetchContext that makes sense, but are other call sites ok
> with
> > this change?  E.g., the one in WebLocalFrameImpl::checkIfRunInsecureContent
> > seems like it may end up using the parent frame in new code but not in old
> code
> > - is this ok?  (There's also a comment there about that function used only
for
> > NPAPI and going away, so this may not matter much).
> 
> Ahh, I missed that one, thanks. (I think it shouldn't matter for the other
call
> sites).
> 
> I'm not sure about this because I don't quite understand why that call site
uses
> FrameTypeNested; I'll ask Mike when he takes a look at this CL.

Sounds good.

https://codereview.chromium.org/1550723003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ?
toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame),
url, requestContext, allowed);
On 2016/01/20 05:56:22, estark wrote:
> On 2016/01/16 00:49:49, alexmos wrote:
> > On 2016/01/14 23:05:48, estark wrote:
> > > On 2016/01/11 22:02:24, alexmos wrote:
> > > > This feels a bit weird, in that the sometimes the message ends up on the
> > > parent
> > > > frame and sometimes on current frame (depending on whether OOPIFs are
> used).
> > 
> > > > What was wrong with just logging on |frame|?
> > > 
> > > I suppose always using |frame| makes sense, but it causes a slight
> regression:
> > > when https://foo.com has an iframe to http://bar.com, the console message
> > won't
> > > say the line number, I guess because there is no line number for loading
the
> > > main resource for a frame. So e.g. the layout test results will be
"CONSOLE
> > > ERROR" instead of "CONSOLE ERROR (line X)". What do you think?
> > 
> > Ah, I see.  It would be nice if the test expectations matched up with and
> > without site isolation, as this will make running layout tests with OOPIFs
> > easier.  So my own slight preference would be to keep this as-is, but I'm
not
> > sure we can get away with it (I'd also be ok if we restored the line number
> > behavior).  Maybe run this by mkwst@ to see if he's ok removing this?  Also,
> how
> > many layout tests are affected (I don't see this coming up in the
expectation
> > changes you have so far)?
> 
> Ok, yeah, I agree that it would be nice if the test expectations were
> consistent, so I'll add this to my ask-Mike list. I think it'll affect about 8
> layout tests (I didn't make the changes yet in case we want to change it
back),
> but I'm kicking off a trybot run to make sure there aren't any others that I
> didn't notice.

Acknowledged.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4678:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PassiveMixedContent) {
On 2016/01/20 05:56:22, estark wrote:
> On 2016/01/16 00:49:50, alexmos wrote:
> > Hmm, it doesn't look like this test exercises any OOPIF functionality,
though
> it
> > does seem useful to have.  I'd suggest moving it to something like
> > web_contents_impl_browsertest.cc, except that it'd be nice to keep all of
> these
> > tests together.  Actually, do you think the other tests would be useful to
run
> > without --site-per-process as well?  If so, something like
> > web_contents_impl_browsertest or render_frame_host_manager_browsertest (or
> even
> > a new test file) might be a good home for them.  These would run without
> > --site-per-process normally, and with --site-per-process on our FYI and try
> bots
> > (which should be enabled the CQ some time soon).
> 
> Hmm... actually, maybe we don't need this test at all, since as you say, it
> doesn't really test OOPIFs, and mixed content in general is covered pretty
well
> by layout tests. So maybe I should just remove this one? For the rest of the
> tests I added here, it certainly wouldn't hurt to run them without
> --site-per-process, but I do think we already have non-site-per-process test
> coverage (in the form of layout tests) for this same functionality. So, maybe
> they should stay here and only run for --site-per-process? What do you think?

That sounds good to me.  Let's remove this one and keep the others in this file.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4689:
PassiveMixedContentInIframe) {
Do you plan to add similar tests for active mixed content?  Seems like that
might be worth having.

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

https://codereview.chromium.org/1550723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:448:
mainResourceUrlForFrame(mixedFrame).elidedString().utf8().data(),
url.elidedString().utf8().data());
On 2016/01/20 05:56:22, estark wrote:
> On 2016/01/16 00:49:50, alexmos wrote:
> > The messages for fetch and WebSocket cases seem to use |frame| for main
> resource
> > URL source, but this one now uses |mixedFrame|.  Is this ok?
> 
> I think the messages for fetch and WebSocket also use |mixedFrame| -- line 377
> for example passes |mixedFrame| into mainResourceUrlForFrame(). Am I
> misunderstanding the question?

Indeed, this looks fine.  I must've confused myself while looking at something
else, sorry!

https://codereview.chromium.org/1550723003/diff/160001/content/test/data/mixe...
File content/test/data/mixed-content/basic-passive-in-iframe.html (right):

https://codereview.chromium.org/1550723003/diff/160001/content/test/data/mixe...
content/test/data/mixed-content/basic-passive-in-iframe.html:12:
document.body.appendChild(iframe);
Is it still necessary to add the iframe via script?  Why not just <iframe
src="/cross-site/a.com/mixed-content/basic-passive.html"> ?

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4897: NavigateToURL(shell(),
iframe_url);
nit: EXPECT_TRUE (for this and other uses of NavigateToURL below)

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4902: GURL
navigate_url(https_server.GetURL("/title1.html"));
nit: maybe use GetURL("b.com","/title1.html") to exercise more cross-process
logic :)

[estark, 2016-01-21 04:40:13]

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4678:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PassiveMixedContent) {
On 2016/01/20 22:43:55, alexmos wrote:
> On 2016/01/20 05:56:22, estark wrote:
> > On 2016/01/16 00:49:50, alexmos wrote:
> > > Hmm, it doesn't look like this test exercises any OOPIF functionality,
> though
> > it
> > > does seem useful to have.  I'd suggest moving it to something like
> > > web_contents_impl_browsertest.cc, except that it'd be nice to keep all of
> > these
> > > tests together.  Actually, do you think the other tests would be useful to
> run
> > > without --site-per-process as well?  If so, something like
> > > web_contents_impl_browsertest or render_frame_host_manager_browsertest (or
> > even
> > > a new test file) might be a good home for them.  These would run without
> > > --site-per-process normally, and with --site-per-process on our FYI and
try
> > bots
> > > (which should be enabled the CQ some time soon).
> > 
> > Hmm... actually, maybe we don't need this test at all, since as you say, it
> > doesn't really test OOPIFs, and mixed content in general is covered pretty
> well
> > by layout tests. So maybe I should just remove this one? For the rest of the
> > tests I added here, it certainly wouldn't hurt to run them without
> > --site-per-process, but I do think we already have non-site-per-process test
> > coverage (in the form of layout tests) for this same functionality. So,
maybe
> > they should stay here and only run for --site-per-process? What do you
think?
> 
> That sounds good to me.  Let's remove this one and keep the others in this
file.

Done.

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4689:
PassiveMixedContentInIframe) {
On 2016/01/20 22:43:55, alexmos wrote:
> Do you plan to add similar tests for active mixed content?  Seems like that
> might be worth having.

I should have done that earlier, sorry. I just added a couple, but the only way
I could figure out to do them is to put them in
chrome_site_per_process_browsertest.cc so that I could use the
|kAllowRunningInsecureContent| switch in chrome/. It's a bummer to have the
passive mixed content ones here and the active somewhere else... What do you
think of that? Do you know of any way to set a webkit setting from a content
browser test?

https://codereview.chromium.org/1550723003/diff/160001/content/test/data/mixe...
File content/test/data/mixed-content/basic-passive-in-iframe.html (right):

https://codereview.chromium.org/1550723003/diff/160001/content/test/data/mixe...
content/test/data/mixed-content/basic-passive-in-iframe.html:12:
document.body.appendChild(iframe);
On 2016/01/20 22:43:55, alexmos wrote:
> Is it still necessary to add the iframe via script?  Why not just <iframe
> src="/cross-site/a.com/mixed-content/basic-passive.html"> ?

Oops, leftover from when I was using document.location.port. Done.

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4897: NavigateToURL(shell(),
iframe_url);
On 2016/01/20 22:43:55, alexmos wrote:
> nit: EXPECT_TRUE (for this and other uses of NavigateToURL below)

Done.

https://codereview.chromium.org/1550723003/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4902: GURL
navigate_url(https_server.GetURL("/title1.html"));
On 2016/01/20 22:43:55, alexmos wrote:
> nit: maybe use GetURL("b.com","/title1.html") to exercise more cross-process
> logic :)

Done.

[alexmos, 2016-01-21 18:13:28]

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4689:
PassiveMixedContentInIframe) {
On 2016/01/21 04:40:13, estark wrote:
> On 2016/01/20 22:43:55, alexmos wrote:
> > Do you plan to add similar tests for active mixed content?  Seems like that
> > might be worth having.
> 
> I should have done that earlier, sorry. I just added a couple, but the only
way
> I could figure out to do them is to put them in
> chrome_site_per_process_browsertest.cc so that I could use the
> |kAllowRunningInsecureContent| switch in chrome/. It's a bummer to have the
> passive mixed content ones here and the active somewhere else... What do you
> think of that? Do you know of any way to set a webkit setting from a content
> browser test?

What do you think about flipping things to instead test that the active mixed
content gets blocked correctly by default?  That would better cover the bug we
originally had with failing open in 486936, and if you use an <iframe> instead
of <script>, you could just check whether an additional FrameTreeNode was
created or not.  This won't require specifying kAllowRunningInsecureContent so
you could do it from content.

For covering the kAllowRunningInsecureContent path, it seems we may already some
tests (e.g., ChromeSecurityStateModelClientTest.MixedContent), though I didn't
check if they end up using OOPIFs for their frames when run with
--site-per-process.

[alexmos, 2016-01-21 18:21:51]

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4689:
PassiveMixedContentInIframe) {
On 2016/01/21 18:13:28, alexmos wrote:
> On 2016/01/21 04:40:13, estark wrote:
> > On 2016/01/20 22:43:55, alexmos wrote:
> > > Do you plan to add similar tests for active mixed content?  Seems like
that
> > > might be worth having.
> > 
> > I should have done that earlier, sorry. I just added a couple, but the only
> way
> > I could figure out to do them is to put them in
> > chrome_site_per_process_browsertest.cc so that I could use the
> > |kAllowRunningInsecureContent| switch in chrome/. It's a bummer to have the
> > passive mixed content ones here and the active somewhere else... What do you
> > think of that? Do you know of any way to set a webkit setting from a content
> > browser test?
> 
> What do you think about flipping things to instead test that the active mixed
> content gets blocked correctly by default?  That would better cover the bug we
> originally had with failing open in 486936, and if you use an <iframe> instead
> of <script>, you could just check whether an additional FrameTreeNode was
> created or not.  This won't require specifying kAllowRunningInsecureContent so
> you could do it from content.
> 
> For covering the kAllowRunningInsecureContent path, it seems we may already
some
> tests (e.g., ChromeSecurityStateModelClientTest.MixedContent), though I didn't
> check if they end up using OOPIFs for their frames when run with
> --site-per-process.

Also, if you did want to override that setting from content/, maybe you could do
something like this?

  WebPreferences prefs =
      GetWebContents()->GetRenderViewHost()->GetWebkitPreferences();
  prefs.allow_running_insecure_content = true;
  GetWebContents()->GetRenderViewHost()->UpdateWebkitPreferences(prefs);

No idea whether that'd work, but BrowserPluginGuest::InitInternal appears to use
something like this
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...)

[estark, 2016-01-21 20:54:07]

On 2016/01/21 18:13:28, alexmos wrote:
>
https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1550723003/diff/120001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:4689:
> PassiveMixedContentInIframe) {
> On 2016/01/21 04:40:13, estark wrote:
> > On 2016/01/20 22:43:55, alexmos wrote:
> > > Do you plan to add similar tests for active mixed content?  Seems like
that
> > > might be worth having.
> > 
> > I should have done that earlier, sorry. I just added a couple, but the only
> way
> > I could figure out to do them is to put them in
> > chrome_site_per_process_browsertest.cc so that I could use the
> > |kAllowRunningInsecureContent| switch in chrome/. It's a bummer to have the
> > passive mixed content ones here and the active somewhere else... What do you
> > think of that? Do you know of any way to set a webkit setting from a content
> > browser test?
> 
> What do you think about flipping things to instead test that the active mixed
> content gets blocked correctly by default?  That would better cover the bug we
> originally had with failing open in 486936, and if you use an <iframe> instead
> of <script>, you could just check whether an additional FrameTreeNode was
> created or not.  This won't require specifying kAllowRunningInsecureContent so
> you could do it from content.
> 
> For covering the kAllowRunningInsecureContent path, it seems we may already
some
> tests (e.g., ChromeSecurityStateModelClientTest.MixedContent), though I didn't
> check if they end up using OOPIFs for their frames when run with
> --site-per-process.

That makes sense to me, especially because IIRC Mike wants to stop allowing
active mixed content completely in the near future, so it makes sense to focus
on tests that active mixed content is blocked when it should be. I moved the
active mixed content test to site_per_process_browsertest.cc and used the frame
tree to decide if the content has been blocked, as you suggested.
ChromeSecurityStateModelClientTest.MixedContent should exercise OOPIFs when run
with --site-per-process (it has an HTTPS frame that embeds a cross-site https
frame that runs mixed content), and I added another one that it similar but with
strict mixed content blocking enforced.

[alexmos, 2016-01-21 22:38:41]

Thanks, LGTM.

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4953:
ActiveMixedContentInIframe) {
This test is great.  Just to make sure, does it fail without your patch?  I'm
just curious whether the http://a.test/ grandchild will return true for
has_committed_real_load(), or does it need to point to a valid page?

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4965:
ASSERT_TRUE(root->child_at(0));
nit: since this would crash if there's no child, a better check for this and the
grandchild might be ASSERT_EQ(1U, root->child_count());

[estark, 2016-01-21 23:24:15]

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4953:
ActiveMixedContentInIframe) {
On 2016/01/21 22:38:41, alexmos wrote:
> This test is great.  Just to make sure, does it fail without your patch?  I'm
> just curious whether the http://a.test/ grandchild will return true for
> has_committed_real_load(), or does it need to point to a valid page?

Yep, it fails if the mixed content is allowed to run.

https://codereview.chromium.org/1550723003/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4965:
ASSERT_TRUE(root->child_at(0));
On 2016/01/21 22:38:41, alexmos wrote:
> nit: since this would crash if there's no child, a better check for this and
the
> grandchild might be ASSERT_EQ(1U, root->child_count());

Done.

[estark, 2016-01-21 23:35:48]

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

[estark, 2016-01-21 23:35:49]

Thanks for the review and all the help, Alex!

mkwst, could you please take a look now?

I've left a couple questions for you inline that came up while Alex was
reviewing that we weren't sure about.

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

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:311: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
Mike: it used to be that FrameFetchContext called effectiveFrameForFrameType()
and passed the result of that in to shouldBlockFetch(). Now I'm calling it in
here because we need access to a LocalFrame here inside shouldBlockFetch(). (And
the result of effectiveFrameForFrameType() is not necessarily local.)

However, Alex pointed out that there are other shouldBlockFetch() callers that
might be affected by this change. The only callsite that I think would be
affected is WebLocalFrameImpl::checkIfRunInsecureContent()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
We'll now end up using the parent frame for that mixed content check instead of
the local frame. However, I don't understand why that callsite uses
FrameTypeNested, and what might explode with this change. Do you have any wisdom
you can share?

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(frame, mainResourceUrlForFrame(mixedFrame), url,
requestContext, allowed);
Mike: there's a small regression here that I want to make sure you're okay with.
We used to print the console log to the effective frame, but now we print it to
the local frame (since we don't right now have any way of printing a console
message to a remote frame and the effective frame might be remote). Practically
speaking, this means that if https://foo.com embeds <iframe
src="http://bar.com">, the console message about the mixed content will no
longer have a line number associated with it, because the message gets printed
to the subframe rather than the parent frame. (This is reflected in the layout
test expectations that I changed.) Do you think that's okay?

[Mike West, 2016-01-22 12:59:22]

I've only skimmed the bits outside of Blink, but this LGTM. Thanks for taking
the time to tackle it!

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

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:63: static void
measureStricterVersionOfIsMixedContent(Frame* frame, const KURL& url)
Thanks for the reminder that I need to look at these counters... :)

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:311: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/21 at 23:35:49, estark wrote:
> Mike: it used to be that FrameFetchContext called effectiveFrameForFrameType()
and passed the result of that in to shouldBlockFetch(). Now I'm calling it in
here because we need access to a LocalFrame here inside shouldBlockFetch(). (And
the result of effectiveFrameForFrameType() is not necessarily local.)
> 
> However, Alex pointed out that there are other shouldBlockFetch() callers that
might be affected by this change. The only callsite that I think would be
affected is WebLocalFrameImpl::checkIfRunInsecureContent()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
We'll now end up using the parent frame for that mixed content check instead of
the local frame. However, I don't understand why that callsite uses
FrameTypeNested, and what might explode with this change. Do you have any wisdom
you can share?

I'm pretty sure we can remove that path entirely, as I'm pretty sure I added the
comment asserting that it was wrapped up in NPAPI terribleness. Will attempt to
kill it in a separate CL: https://codereview.chromium.org/1621503003

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(frame, mainResourceUrlForFrame(mixedFrame), url,
requestContext, allowed);
On 2016/01/21 at 23:35:49, estark wrote:
> Mike: there's a small regression here that I want to make sure you're okay
with. We used to print the console log to the effective frame, but now we print
it to the local frame (since we don't right now have any way of printing a
console message to a remote frame and the effective frame might be remote).
Practically speaking, this means that if https://foo.com embeds <iframe
src="http://bar.com">, the console message about the mixed content will no
longer have a line number associated with it, because the message gets printed
to the subframe rather than the parent frame. (This is reflected in the layout
test expectations that I changed.) Do you think that's okay?

I think that's a reasonable tradeoff. It's unfortunate that we lose the line
number in these cases, but that doesn't seem fatal.

What would be more problematic is if the console message no longer appears on
the console. I don't know off the top of my head if devtools cares what frame
the message is dumped into. If you open up Chrome with this patch, and still see
the error, then I'm happy. :)

[estark, 2016-01-22 17:37:37]

estark@chromium.org changed reviewers:
+ jochen@chromium.org

[estark, 2016-01-22 17:37:37]

Thanks Mike!

jochen, could you please review changes in content/?

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

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:311: Frame*
effectiveFrame = effectiveFrameForFrameType(frame, frameType);
On 2016/01/22 12:59:21, Mike West wrote:
> On 2016/01/21 at 23:35:49, estark wrote:
> > Mike: it used to be that FrameFetchContext called
effectiveFrameForFrameType()
> and passed the result of that in to shouldBlockFetch(). Now I'm calling it in
> here because we need access to a LocalFrame here inside shouldBlockFetch().
(And
> the result of effectiveFrameForFrameType() is not necessarily local.)
> > 
> > However, Alex pointed out that there are other shouldBlockFetch() callers
that
> might be affected by this change. The only callsite that I think would be
> affected is WebLocalFrameImpl::checkIfRunInsecureContent()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> We'll now end up using the parent frame for that mixed content check instead
of
> the local frame. However, I don't understand why that callsite uses
> FrameTypeNested, and what might explode with this change. Do you have any
wisdom
> you can share?
> 
> I'm pretty sure we can remove that path entirely, as I'm pretty sure I added
the
> comment asserting that it was wrapped up in NPAPI terribleness. Will attempt
to
> kill it in a separate CL: https://codereview.chromium.org/1621503003

Perfect, thanks!

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:377:
logToConsoleAboutFetch(frame, mainResourceUrlForFrame(mixedFrame), url,
requestContext, allowed);
On 2016/01/22 12:59:22, Mike West wrote:
> On 2016/01/21 at 23:35:49, estark wrote:
> > Mike: there's a small regression here that I want to make sure you're okay
> with. We used to print the console log to the effective frame, but now we
print
> it to the local frame (since we don't right now have any way of printing a
> console message to a remote frame and the effective frame might be remote).
> Practically speaking, this means that if https://foo.com embeds <iframe
> src="http://bar.com">, the console message about the mixed content will no
> longer have a line number associated with it, because the message gets printed
> to the subframe rather than the parent frame. (This is reflected in the layout
> test expectations that I changed.) Do you think that's okay?
> 
> I think that's a reasonable tradeoff. It's unfortunate that we lose the line
> number in these cases, but that doesn't seem fatal.
> 
> What would be more problematic is if the console message no longer appears on
> the console. I don't know off the top of my head if devtools cares what frame
> the message is dumped into. If you open up Chrome with this patch, and still
see
> the error, then I'm happy. :)

So the message does get printed to the console, but the catch is that DevTools
is currently frame-specific for OOPIFs. So you'll see the message if you inspect
the frame to which the message gets printed. There is, however, a general
DevTools refactor that'll happen at some point to make the whole thing make more
sense when OOPIFs is on. I had a thread going with site-isolation-dev@ a few
weeks ago about this; I can add you if you want to see more detail.

[jochen (gone - plz use gerrit), 2016-01-25 11:33:29]

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https-expected.txt
(right):

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https-expected.txt:1:
CONSOLE ERROR: Mixed Content: The page at
'https://127.0.0.1:8443/security/mixedContent/strict-mode-image-in-frame-blocked.https.html'
was loaded over HTTPS, but requested an insecure image
'http://127.0.0.1:8080/security/resources/compass.jpg'. This request has been
blocked; the content must be served over HTTPS.
this expectation update looks odd - from the CL description, I would have
expected something like https://127.0.0.1:8443/ was loaded over HTTPS (i.e. just
the origin), but instead, it's a different URL?

[estark, 2016-01-25 16:21:06]

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https-expected.txt
(right):

https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https-expected.txt:1:
CONSOLE ERROR: Mixed Content: The page at
'https://127.0.0.1:8443/security/mixedContent/strict-mode-image-in-frame-blocked.https.html'
was loaded over HTTPS, but requested an insecure image
'http://127.0.0.1:8080/security/resources/compass.jpg'. This request has been
blocked; the content must be served over HTTPS.
On 2016/01/25 11:33:29, jochen wrote:
> this expectation update looks odd - from the CL description, I would have
> expected something like https://127.0.0.1:8443/ was loaded over HTTPS (i.e.
just
> the origin), but instead, it's a different URL?

Hmm, yes, there are a couple different things going on here, which I'll try to
explain here to see if they make sense:

First, we only would print the origin if the mixed frame (the frame with respect
to which we are considering the content mixed, i.e. the |mixedFrame| variable in
MixedContentChecker) was a cross-site OOPIF; otherwise it's a local frame and we
have access to the full URL. Since it's not OOP in the layout test, the test
prints the whole URL, not just the origin. Though, now come to think of it,
maybe we should always just print the origin for consistency in both
site-isolated and not-site-isolated worlds.

Second, I did change which URL gets printed, which is why the URL changed here.
The old MixedContentChecker used |frame| to retrieve the main resource URL to
print in this message, whereas the new MixedContentChecker uses |mixedFrame| --
the frame with respect to which we are considering the content mixed. In the old
world, it could get weird because if https://a.com embeds http://b.com which
loads http://b.com/foo.jpg, the image would have been considered mixed content
with respect to the top-level frame (https://a.com) but we would have printed a
confusing message about the embedded frame: "The page at http://b.com was loaded
over HTTPS but requested an insecure image...". So I thought it made more sense
to print the top-level frame.

Another approach might be to say that once a frame loads as mixed content, we
don't need to print console messages about every subresource inside the frame
also being mixed content. So we would continue to use the frame that actually
requested the resource to get the URL for the console message, but we wouldn't
print the message when the subresource is not considered mixed content with
respect to that frame.

[jochen (gone - plz use gerrit), 2016-01-26 11:44:58]

lgtm

[estark, 2016-01-26 16:10:09]

The CQ bit was checked by estark@chromium.org

[estark, 2016-01-26 16:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1550723003/#ps280001
(title: "fix test assertion")

[commit-bot: I haz the power, 2016-01-26 16:10:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550723003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550723003/280001

[commit-bot: I haz the power, 2016-01-26 16:12:27]

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

[commit-bot: I haz the power, 2016-01-26 16:12:28]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[estark, 2016-01-26 16:21:22]

The CQ bit was checked by estark@chromium.org

[estark, 2016-01-26 16:21:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1550723003/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-26 16:21:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550723003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550723003/300001

[commit-bot: I haz the power, 2016-01-26 17:58:39]

Description was changed from

==========
Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936
==========

to

==========
Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936
==========

[commit-bot: I haz the power, 2016-01-26 17:58:40]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-01-26 17:59:50]

Description was changed from

==========
Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936
==========

to

==========
Adapt MixedContentChecker for remote frames

This CL adapts MixedContentChecker to do correct mixed content checks
for content that is mixed with respect to a remote frame. There are a
couple caveats/behavior changes:

- When mixed content is detected, the message logged to the console is
  changed slightly: when the mixed frame is remote, it prints the origin
  instead of the full URL.
- Subresources loaded with certificate errors are not yet correctly
  treated as mixed content when they are loaded in a remote HTTPS parent
  frame.
- FrameLoaderClient methods, for the most part, go to the client for the
  frame that loaded the resource, which is not necessarily the "mixed
  frame". This is weird, but in practice, the implementations of these
  FrameLoaderClient methods do not care which frame they are actually
  called on.
BUG=486936

Committed: https://crrev.com/56dc8e2191815bb5b64a5b81b5f1c4db7828b6ed
Cr-Commit-Position: refs/heads/master@{#371534}
==========

[commit-bot: I haz the power, 2016-01-26 17:59:51]

Patchset 16 (id:??) landed as
https://crrev.com/56dc8e2191815bb5b64a5b81b5f1c4db7828b6ed
Cr-Commit-Position: refs/heads/master@{#371534}