Move mixed content settings histograms into browser

Move mixed content settings histograms into browser

This CL moves the content settings histograms for mixed content into the
browser, called via a separate FrameClient method from the existing
FrameLoaderClient::allowDisplayingInsecureContent/allowRunningInsecureContent
methods.

The purpose of doing this is to make mixed content checks work with
OOPIFs. The histograms require knowledge of the full URL of a possibly
remote frame, whereas checking whether the content is allowed can be
done from the local frame client. Thus, this CL separates out the
histogram logic (and makes it available on a RemoteFrameClient) from the
content settings check (which is always done on the LocalFrame that
loaded the mixed content, since the content settings for mixed content
are always applied per WebContents).

BUG=486936

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

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

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

mkwst, alexmos: could you take a look at this rough-draft CL and its follow-up
https://codereview.chromium.org/1550723003/? No tests yet... looking for
high-level feedback mostly. The general idea is as follows.

Suppose https://a.com loads an iframe https://b.com which loads a mixed image.
Currently, MixedContentChecker considers the image mixed with respect to the
top-level frame, so it uses a.com's FrameLoaderClient methods to ask the
embedder whether the mixed content should be blocked or allowed. This doesn't
work out-of-the-box if a.com is a remote frame. However, for the most part, the
embedder doesn't actually care which frame these methods are called on, and
we'll get the same result whether we use a.com's FrameLoaderClient or b.com's
FrameLoaderClient... except for when the embedder records histograms, when it
needs to know the actual URL of the mixed frame (e.g. if we were to call b.com's
FLC, it would want to know a.com's full URL). So this CL separates out the
histograms code into its own FrameClient method that can be called on a remote
frame.

This basically means that when MixedContentChecker detects mixed content, it now
does two steps:
1. Notify the (possibly remote) frame with respect to which the content is
considered mixed.
2. Ask the local frame whether the content should be allowed.

(This CL just separates out the histograms code, and the follow-up CL actually
changes MixedContentChecker to operate on possibly-remote frames.)

Does that make any sense? Is this too weird? Thanks!

[Charlie Reis, 2016-01-07 20:13:25]

Description was changed from

==========
Move mixed content settings histograms into browser

This CL moves the content settings histograms for mixed content into the
browser, called via a separate FrameClient method from the existing
FrameLoaderClient::allowDisplayingInsecureContent/allowRunningInsecureContent
methods.

The purpose of doing this is to make mixed content checks work with
OOPIFs. The histograms require knowledge of the full URL of a possibly
remote frame, whereas checking whether the content is allowed can be
done from the local frame client. Thus, this CL separates out the
histogram logic (and makes it available on a RemoteFrameClient) from the
content settings check (which is always done on the LocalFrame that
loaded the mixed content, since the content settings for mixed content
are always applied per WebContents).

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

to

==========
Move mixed content settings histograms into browser

This CL moves the content settings histograms for mixed content into the
browser, called via a separate FrameClient method from the existing
FrameLoaderClient::allowDisplayingInsecureContent/allowRunningInsecureContent
methods.

The purpose of doing this is to make mixed content checks work with
OOPIFs. The histograms require knowledge of the full URL of a possibly
remote frame, whereas checking whether the content is allowed can be
done from the local frame client. Thus, this CL separates out the
histogram logic (and makes it available on a RemoteFrameClient) from the
content settings check (which is always done on the LocalFrame that
loaded the mixed content, since the content settings for mixed content
are always applied per WebContents).

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

[alexmos, 2016-01-09 01:39:02]

Thanks!  Overall, this approach looks reasonable to me, and moving histographs
over seems like a step in the right direction considering we eventually want all
of mixed content checking in the browser process, though I'm curious whether the
RemoteFrame's URL is actually necessary (see below).  A few thoughts below (I
also skipped over a bunch of places that need comments), and looking forward to
tests.

+Charlie for my question about the race below.

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:876: const
GURL& url) {
It's a little unclear what origin and url refer to (here and elsewhere).  Maybe
something like mixed_frame_origin and resource_url?  Although mixed_frame_origin
is probably also confusing and would need to be explained.

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:879:
DCHECK_EQ(frame_gurl.host(), origin_host);
Hmm, I'm thinking there may be a race here, where the top frame is navigating to
a new URL in parallel to mixed content occurring.  In that case,
GetLastCommittedURL might return the new URL, whereas the origin from the
renderer is old and thus may not match.  

Charlie, I know you know about many such races - is such a situation possible? 
The context here is A-embed-B, B has a mixed image, and B sends up an IPC to A's
RFPH, which ends up here (rfh is for A) to record histograms.

Also, couldn't we just get the origin on the browser side as well, rather than
passing it from the renderer?  We now have
RenderFrameHost::GetLastCommittedOrigin.  This would at least make the origin
and URL consistent, though possibly out of sync with the renderer due to race
above, but maybe that's ok for histograms?   This also wouldn't let the renderer
lie about the origin.

All that said, I think the DCHECK is still problematic, because the origin and
URL may be inconsistent in certain scenarios (e.g., when origin is inherited
from parent for about:blank).

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:895: if
(base::StartsWith(frame_gurl.path(), kGoogleReaderPathPrefix,
Another thought: there are three uses of frame_gurl.path().  Do we know that we
still need to record histograms for all three?  I'd bet this Reader one isn't
needed anymore, but unsure about others.  Maybe we could avoid needing the
remote frame's URL if we can deprecate these three stats?

https://codereview.chromium.org/1550233002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (left):

https://codereview.chromium.org/1550233002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:520: Send(new
ChromeViewHostMsg_DidBlockDisplayingInsecureContent(routing_id()));
Just curious, how does this still work once you change the frame on which this
is called to always be the LocalFrame?  Does this go through the correct
WebContents on the browser side regardless of which of its frames it's routed
through?

https://codereview.chromium.org/1550233002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1550233002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:411: void
OnTriedDisplayingInsecureContent(const GURL& origin, const GURL& url);
Let's keep these private and along with the other IPC handlers (see also my
comment on RFPH)

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

https://codereview.chromium.org/1550233002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:368:
frame_tree_node_->current_frame_host()->OnTriedDisplayingInsecureContent(
Instead of referencing IPC handlers directly and making them public, I think you
could just call into the WebContents directly via
frame_tree_node_->current_frame_host()->delegate()->TriedDisplayingInsecureContent()

https://codereview.chromium.org/1550233002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1550233002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:854:
IPC_MESSAGE_ROUTED2(FrameHostMsg_TriedDisplayingInsecureContent, GURL, GURL)
nit: document what each parameter is.

https://codereview.chromium.org/1550233002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1550233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2611: routing_id_,
GURL(origin.toString().utf8()), url));
If we do end up passing origin through this, it's probably better to pass it as
url::Origin.

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

https://codereview.chromium.org/1550233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:329: if
(!strictMode)
I know this preserves original behavior, but do you know if it's intentional
that we aren't recording histograms in strictMode?

https://codereview.chromium.org/1550233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:331: allowed =
!strictMode && client->allowDisplayingInsecureContent(settings &&
settings->allowDisplayOfInsecureContent(), securityOrigin, url);
This could benefit from a comment somewhere explaining why we call
triedDisplaying and allowDisplaying on different frames - basically a summary of
what you already have in the CL message/description.

https://codereview.chromium.org/1550233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:393:
FrameLoaderClient* client = mixedFrame->loader().client();
Should this also become frame->loader().client()?  Or, I actually see you're
changing this in the next CL, so maybe the one in shouldBlockFetch should also
be changed in that CL?

[Charlie Reis, 2016-01-12 18:10:47]

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:879:
DCHECK_EQ(frame_gurl.host(), origin_host);
On 2016/01/09 01:39:02, alexmos wrote:
> Hmm, I'm thinking there may be a race here, where the top frame is navigating
to
> a new URL in parallel to mixed content occurring.  In that case,
> GetLastCommittedURL might return the new URL, whereas the origin from the
> renderer is old and thus may not match.  
> 
> Charlie, I know you know about many such races - is such a situation possible?

> The context here is A-embed-B, B has a mixed image, and B sends up an IPC to
A's
> RFPH, which ends up here (rfh is for A) to record histograms.

Yes, this race is possible.  See the documentation for both GetLastCommittedURL
and GetLastCommittedOrigin on RenderFrameHost-- those methods are confusing
because they actually return the FrameTreeNode's current RenderFrameHost's
URL/origin, even when you call them on a different RenderFrameHost (e.g., one
that has just been replaced during a cross-process navigation).  We're looking
at ways to improve them, but it's tricky since FrameTreeNode isn't exposed
publicly.

Here, you could receive this OnTriedDisplayingInsecureContent IPC from a RFH
that was just replaced by a new RFH, and both the last committed URL and origin
would be from the new RFH.

> 
> Also, couldn't we just get the origin on the browser side as well, rather than
> passing it from the renderer?  We now have
> RenderFrameHost::GetLastCommittedOrigin.  This would at least make the origin
> and URL consistent, though possibly out of sync with the renderer due to race
> above, but maybe that's ok for histograms?   This also wouldn't let the
renderer
> lie about the origin.
> 
> All that said, I think the DCHECK is still problematic, because the origin and
> URL may be inconsistent in certain scenarios (e.g., when origin is inherited
> from parent for about:blank).

[estark, 2016-01-12 18:55:23]

On 2016/01/12 18:10:47, Charlie Reis wrote:
>
https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
> File chrome/browser/content_settings/tab_specific_content_settings.cc (right):
> 
>
https://codereview.chromium.org/1550233002/diff/40001/chrome/browser/content_...
> chrome/browser/content_settings/tab_specific_content_settings.cc:879:
> DCHECK_EQ(frame_gurl.host(), origin_host);
> On 2016/01/09 01:39:02, alexmos wrote:
> > Hmm, I'm thinking there may be a race here, where the top frame is
navigating
> to
> > a new URL in parallel to mixed content occurring.  In that case,
> > GetLastCommittedURL might return the new URL, whereas the origin from the
> > renderer is old and thus may not match.  
> > 
> > Charlie, I know you know about many such races - is such a situation
possible?
> 
> > The context here is A-embed-B, B has a mixed image, and B sends up an IPC to
> A's
> > RFPH, which ends up here (rfh is for A) to record histograms.
> 
> Yes, this race is possible.  See the documentation for both
GetLastCommittedURL
> and GetLastCommittedOrigin on RenderFrameHost-- those methods are confusing
> because they actually return the FrameTreeNode's current RenderFrameHost's
> URL/origin, even when you call them on a different RenderFrameHost (e.g., one
> that has just been replaced during a cross-process navigation).  We're looking
> at ways to improve them, but it's tricky since FrameTreeNode isn't exposed
> publicly.
> 
> Here, you could receive this OnTriedDisplayingInsecureContent IPC from a RFH
> that was just replaced by a new RFH, and both the last committed URL and
origin
> would be from the new RFH.

Gotcha, thanks for the explanation. I started a thread with Enamel to see what
these histograms are currently used for, if anything. I suspect that the
raciness might not be of huge importance, or even that the histogram values that
depend on the current URL of the frame might not even be of interest anymore.

> 
> > 
> > Also, couldn't we just get the origin on the browser side as well, rather
than
> > passing it from the renderer?  We now have
> > RenderFrameHost::GetLastCommittedOrigin.  This would at least make the
origin
> > and URL consistent, though possibly out of sync with the renderer due to
race
> > above, but maybe that's ok for histograms?   This also wouldn't let the
> renderer
> > lie about the origin.
> > 
> > All that said, I think the DCHECK is still problematic, because the origin
and
> > URL may be inconsistent in certain scenarios (e.g., when origin is inherited
> > from parent for about:blank).