Plumb document's strict mixed content checking for RemoteFrames

Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

BUG=486936
Committed: https://crrev.com/a886b8d6c10c4b218621fb762026fab7b426f608
Cr-Commit-Position: refs/heads/master@{#366177}

[estark, 2015-12-02 18:13:18]

Description was changed from

==========
Plumb document's strict mixed content checking for RemoteFrames

BUG=
==========

to

==========
Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

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

[estark, 2015-12-02 18:14:29]

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

[estark, 2015-12-02 18:14:29]

alexmos: could you take a look at this from a site-isolation-y point of view?
I'm mostly interested in whether this high-level approach is good, though happy
to get detailed review as well. This is the first step towards making mixed
content checks work for OOPIFs. The goal here is, when
document()->shouldEnforceStrictMixedContentChecking() changes on a frame, to
propagate the new value to the corresponding RemoteFrames.

[alexmos, 2015-12-03 20:43:26]

+site-isolation-reviews

Thanks for taking this on, Emily!  I think this makes sense overall -- a few
high-level questions and comments below.

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

https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:218:
replication_state_.should_enforce_strict_mixed_content_checking =
What's the lifetime of this flag in Blink?  Does it get reset to false on each
frame navigation?  If it does (and I think it does), then we would need to reset
it in the replication state for each navigation, since replication state (and
FrameTreeNode in general) stays around across navigations (both same-site and
cross-site) of the same frame.

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

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_me...
content/common/frame_messages.h:811:
FrameHostMsg_DidChangeShouldEnforceStrictMixedContentChecking,
Can this flag change dynamically while the Document is around, or can it only be
set once, when the document is being loaded and CSP is parsed?  If the latter,
do we need this separate message, or could we just send the flag over as part of
DidCommitProvisionalLoad, similar to how we set the replicated origin today (in
NavigatorImpl::DidNavigate)?

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
content/common/frame_replication_state.h:76: bool
should_enforce_strict_mixed_content_checking;
Does this bit need to be processed when creating new remote frames for a
FrameTreeNode?  See RenderFrameProxy::CreateFrameProxy and specifically
RenderFrameProxy::SetReplicatedState.  Here's an example where this might
matter:
- start with A.com embedding two same-site frames, F1 and F2.
- F2 sets its strict mixed content checking bit.
- F1 navigates cross-site to B.com.  That creates process B, in which we will
create a frame for F1, a proxy for the root frame, and a proxy for F2.  That
proxy for F2 should have the mixed content bit set.   I think this would matter
if we later add another B.com child frame to F2, as that will check the mixed
content bit on that F2 proxy (the new frame's parent) when doing
initSecurityContext for the new frame.

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

https://codereview.chromium.org/1489253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2487: blink::WebLocalFrame* frame,
I think you can just remove |frame| from the arg list, given that you're already
calling this on the right RenderFrameImpl.

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5757:
frame()->loader().client()->didChangeShouldEnforceStrictMixedContentChecking(true);
It seems that Blink only ever flips the bit from false to true, but the
replication API is more generic and allows flipping it from true to false as
well.  Do we need that, or should the plumbing be more of the form
didEnforceStrictMixedContentChecking()?

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:33: ,
m_shouldEnforceStrictMixedContentChecking(false)
Would it make sense to store m_shouldEnforceStrictMixedContentChecking in
SecurityContext instead, similar to sandbox flags?  That would avoid having to
store it in two different places for local and remote frames (since
RemoteSecurityContext inherits from SecurityContext).

[estark, 2015-12-04 00:19:32]

Thanks Alex. By the way, do you know of any examples that I should look at for
writing tests for this CL? Ideally I'd like to, say, write a browser test that
sets the bit in a frame and checks that the correct IPCs are sent to the other
renderers, or something like that.

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

https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:218:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/03 20:43:26, alexmos wrote:
> What's the lifetime of this flag in Blink?  Does it get reset to false on each
> frame navigation?  If it does (and I think it does), then we would need to
reset
> it in the replication state for each navigation, since replication state (and
> FrameTreeNode in general) stays around across navigations (both same-site and
> cross-site) of the same frame.

Ah, yes, it should be reset on navigation. I've now done this by resetting it in
RFHManager::DidNavigateFrame() -- does that seem correct?

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

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_me...
content/common/frame_messages.h:811:
FrameHostMsg_DidChangeShouldEnforceStrictMixedContentChecking,
On 2015/12/03 20:43:26, alexmos wrote:
> Can this flag change dynamically while the Document is around, or can it only
be
> set once, when the document is being loaded and CSP is parsed?  If the latter,
> do we need this separate message, or could we just send the flag over as part
of
> DidCommitProvisionalLoad, similar to how we set the replicated origin today
(in
> NavigatorImpl::DidNavigate)?

It can change dynamically. (For example, if a <meta> element with a CSP gets
added via JS dynamically.)

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
content/common/frame_replication_state.h:76: bool
should_enforce_strict_mixed_content_checking;
On 2015/12/03 20:43:26, alexmos wrote:
> Does this bit need to be processed when creating new remote frames for a
> FrameTreeNode?  See RenderFrameProxy::CreateFrameProxy and specifically
> RenderFrameProxy::SetReplicatedState.  Here's an example where this might
> matter:
> - start with A.com embedding two same-site frames, F1 and F2.
> - F2 sets its strict mixed content checking bit.
> - F1 navigates cross-site to B.com.  That creates process B, in which we will
> create a frame for F1, a proxy for the root frame, and a proxy for F2.  That
> proxy for F2 should have the mixed content bit set.   I think this would
matter
> if we later add another B.com child frame to F2, as that will check the mixed
> content bit on that F2 proxy (the new frame's parent) when doing
> initSecurityContext for the new frame.

Yeah, I agree with you that it's important to set the bit when creating new
remote frames. However I am setting it in SetReplicatedState()... is there
something else I need to do to wire it up?

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

https://codereview.chromium.org/1489253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2487: blink::WebLocalFrame* frame,
On 2015/12/03 20:43:26, alexmos wrote:
> I think you can just remove |frame| from the arg list, given that you're
already
> calling this on the right RenderFrameImpl.

Done.

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5757:
frame()->loader().client()->didChangeShouldEnforceStrictMixedContentChecking(true);
On 2015/12/03 20:43:26, alexmos wrote:
> It seems that Blink only ever flips the bit from false to true, but the
> replication API is more generic and allows flipping it from true to false as
> well.  Do we need that, or should the plumbing be more of the form
> didEnforceStrictMixedContentChecking()?

Done for the render frame -> browser API. The browser -> render frame proxy API
does need to be able to flip it from true to false on navigations, though.

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:33: ,
m_shouldEnforceStrictMixedContentChecking(false)
On 2015/12/03 20:43:26, alexmos wrote:
> Would it make sense to store m_shouldEnforceStrictMixedContentChecking in
> SecurityContext instead, similar to sandbox flags?  That would avoid having to
> store it in two different places for local and remote frames (since
> RemoteSecurityContext inherits from SecurityContext).

Sure, that seems reasonable to me. Done.

[alexmos, 2015-12-04 23:41:11]

> Thanks Alex. By the way, do you know of any examples that I should look at for
> writing tests for this CL? Ideally I'd like to, say, write a browser test that
> sets the bit in a frame and checks that the correct IPCs are sent to the other
> renderers, or something like that.

Yes, most of our tests are in content/browser/site_per_process_browsertest.cc. 
There are many examples there that you could follow; some of the replication
tests (DynamicWindowName, DynamicSandboxFlags, OriginUpdatesReachProxies) could
be good starting points.  If you wanted to verify the actual IPCs being sent, I
think you'd have to use unit tests; there are a couple of examples in
render_frame_host_manager_unittest.cc (such as
CreateOpenerProxiesWithCycleOnOpenerChain).

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

https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:218:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/04 00:19:31, estark wrote:
> On 2015/12/03 20:43:26, alexmos wrote:
> > What's the lifetime of this flag in Blink?  Does it get reset to false on
each
> > frame navigation?  If it does (and I think it does), then we would need to
> reset
> > it in the replication state for each navigation, since replication state
(and
> > FrameTreeNode in general) stays around across navigations (both same-site
and
> > cross-site) of the same frame.
> 
> Ah, yes, it should be reset on navigation. I've now done this by resetting it
in
> RFHManager::DidNavigateFrame() -- does that seem correct?

Yes, that seems fine.  Well, to make sure I understand: does the bit always get
set after a frame has committed a navigation?  Just want to verify that we won't
ever try to set the bit and then clear it when we receive
DidCommitProvisionalLoad.

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

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_me...
content/common/frame_messages.h:811:
FrameHostMsg_DidChangeShouldEnforceStrictMixedContentChecking,
On 2015/12/04 00:19:31, estark wrote:
> On 2015/12/03 20:43:26, alexmos wrote:
> > Can this flag change dynamically while the Document is around, or can it
only
> be
> > set once, when the document is being loaded and CSP is parsed?  If the
latter,
> > do we need this separate message, or could we just send the flag over as
part
> of
> > DidCommitProvisionalLoad, similar to how we set the replicated origin today
> (in
> > NavigatorImpl::DidNavigate)?
> 
> It can change dynamically. (For example, if a <meta> element with a CSP gets
> added via JS dynamically.)

Acknowledged.

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1489253002/diff/20001/content/common/frame_re...
content/common/frame_replication_state.h:76: bool
should_enforce_strict_mixed_content_checking;
On 2015/12/04 00:19:31, estark wrote:
> On 2015/12/03 20:43:26, alexmos wrote:
> > Does this bit need to be processed when creating new remote frames for a
> > FrameTreeNode?  See RenderFrameProxy::CreateFrameProxy and specifically
> > RenderFrameProxy::SetReplicatedState.  Here's an example where this might
> > matter:
> > - start with A.com embedding two same-site frames, F1 and F2.
> > - F2 sets its strict mixed content checking bit.
> > - F1 navigates cross-site to B.com.  That creates process B, in which we
will
> > create a frame for F1, a proxy for the root frame, and a proxy for F2.  That
> > proxy for F2 should have the mixed content bit set.   I think this would
> matter
> > if we later add another B.com child frame to F2, as that will check the
mixed
> > content bit on that F2 proxy (the new frame's parent) when doing
> > initSecurityContext for the new frame.
> 
> Yeah, I agree with you that it's important to set the bit when creating new
> remote frames. However I am setting it in SetReplicatedState()... is there
> something else I need to do to wire it up?

Ah, you're right, that case is indeed fine. :)

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1489253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5757:
frame()->loader().client()->didChangeShouldEnforceStrictMixedContentChecking(true);
On 2015/12/04 00:19:31, estark wrote:
> On 2015/12/03 20:43:26, alexmos wrote:
> > It seems that Blink only ever flips the bit from false to true, but the
> > replication API is more generic and allows flipping it from true to false as
> > well.  Do we need that, or should the plumbing be more of the form
> > didEnforceStrictMixedContentChecking()?
> 
> Done for the render frame -> browser API. The browser -> render frame proxy
API
> does need to be able to flip it from true to false on navigations, though.

Acknowledged.

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

https://codereview.chromium.org/1489253002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:213:
render_manager_.OnDidUpdateShouldEnforceStrictMixedContentChecking(
What was the reason for removing the check and always sending updates?  E.g., it
seems like in the common case, this will get called with "false" a lot on each
navigation, and we don't want to send updates in that case.

[estark, 2015-12-07 20:39:33]

Thanks for the pointers about testing; I'll take a look. In the meantime see
please question inline about when to reset the flag.

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

https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:218:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/04 23:41:11, alexmos wrote:
> On 2015/12/04 00:19:31, estark wrote:
> > On 2015/12/03 20:43:26, alexmos wrote:
> > > What's the lifetime of this flag in Blink?  Does it get reset to false on
> each
> > > frame navigation?  If it does (and I think it does), then we would need to
> > reset
> > > it in the replication state for each navigation, since replication state
> (and
> > > FrameTreeNode in general) stays around across navigations (both same-site
> and
> > > cross-site) of the same frame.
> > 
> > Ah, yes, it should be reset on navigation. I've now done this by resetting
it
> in
> > RFHManager::DidNavigateFrame() -- does that seem correct?
> 
> Yes, that seems fine.  Well, to make sure I understand: does the bit always
get
> set after a frame has committed a navigation?  Just want to verify that we
won't
> ever try to set the bit and then clear it when we receive
> DidCommitProvisionalLoad.

Ah, no, you're right, there's probably no guarantee that it gets set after
commit. In fact, the flag is attached to the Document, so it's also maybe
possible (?) for a new Document to get created and the flag to be unset even
without a navigation happening at all.

Right now when a Document is created, I'm sending an update if
shouldEnforceStrictMixedContentChecking() is true; I think I should be sending
an update either way, so that the frame replication state gets updated to true
if the Document is enforcing strict mixed content checking and false if it
isn't. WDYT of that approach?

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

https://codereview.chromium.org/1489253002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:213:
render_manager_.OnDidUpdateShouldEnforceStrictMixedContentChecking(
On 2015/12/04 23:41:11, alexmos wrote:
> What was the reason for removing the check and always sending updates?  E.g.,
it
> seems like in the common case, this will get called with "false" a lot on each
> navigation, and we don't want to send updates in that case.

Oops, just a mistake. Fixed.

[alexmos, 2015-12-08 01:06:36]

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

https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:218:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/07 20:39:33, estark wrote:
> On 2015/12/04 23:41:11, alexmos wrote:
> > On 2015/12/04 00:19:31, estark wrote:
> > > On 2015/12/03 20:43:26, alexmos wrote:
> > > > What's the lifetime of this flag in Blink?  Does it get reset to false
on
> > each
> > > > frame navigation?  If it does (and I think it does), then we would need
to
> > > reset
> > > > it in the replication state for each navigation, since replication state
> > (and
> > > > FrameTreeNode in general) stays around across navigations (both
same-site
> > and
> > > > cross-site) of the same frame.
> > > 
> > > Ah, yes, it should be reset on navigation. I've now done this by resetting
> it
> > in
> > > RFHManager::DidNavigateFrame() -- does that seem correct?
> > 
> > Yes, that seems fine.  Well, to make sure I understand: does the bit always
> get
> > set after a frame has committed a navigation?  Just want to verify that we
> won't
> > ever try to set the bit and then clear it when we receive
> > DidCommitProvisionalLoad.
> 
> Ah, no, you're right, there's probably no guarantee that it gets set after
> commit. In fact, the flag is attached to the Document, so it's also maybe
> possible (?) for a new Document to get created and the flag to be unset even
> without a navigation happening at all.
> 
> Right now when a Document is created, I'm sending an update if
> shouldEnforceStrictMixedContentChecking() is true; I think I should be sending
> an update either way, so that the frame replication state gets updated to true
> if the Document is enforcing strict mixed content checking and false if it
> isn't. WDYT of that approach?

Yes, I agree that we should send the update from Blink regardless of whether the
flag is true or false, but I think we might not want to tie it to Document
creation, as in that case we might accidentally set/reset the flag for a
Document that ends up not committing.

I've just discussed this with Nasko to be sure, and we think the right approach
is probably to set/reset the browser-side bit at commit time.  At that point in
time, it's guaranteed that the bit (whatever it is) has taken effect for a
particular Document and is safe to use for any future subframes that want to
inherit it, etc.  

So I think you could just pass it in
FrameHostMsg_DidCommitProvisionalLoad_Params, whether it's true or false, just
like the replicated origin.  It sounds like you will still need your
renderer->browser IPC though, to allow for dynamic
updates that may happen later.

[estark, 2015-12-10 02:04:01]

On 2015/12/08 01:06:36, alexmos wrote:
>
https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/frame_tree_node.cc (right):
> 
>
https://codereview.chromium.org/1489253002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/frame_tree_node.cc:218:
> replication_state_.should_enforce_strict_mixed_content_checking =
> On 2015/12/07 20:39:33, estark wrote:
> > On 2015/12/04 23:41:11, alexmos wrote:
> > > On 2015/12/04 00:19:31, estark wrote:
> > > > On 2015/12/03 20:43:26, alexmos wrote:
> > > > > What's the lifetime of this flag in Blink?  Does it get reset to false
> on
> > > each
> > > > > frame navigation?  If it does (and I think it does), then we would
need
> to
> > > > reset
> > > > > it in the replication state for each navigation, since replication
state
> > > (and
> > > > > FrameTreeNode in general) stays around across navigations (both
> same-site
> > > and
> > > > > cross-site) of the same frame.
> > > > 
> > > > Ah, yes, it should be reset on navigation. I've now done this by
resetting
> > it
> > > in
> > > > RFHManager::DidNavigateFrame() -- does that seem correct?
> > > 
> > > Yes, that seems fine.  Well, to make sure I understand: does the bit
always
> > get
> > > set after a frame has committed a navigation?  Just want to verify that we
> > won't
> > > ever try to set the bit and then clear it when we receive
> > > DidCommitProvisionalLoad.
> > 
> > Ah, no, you're right, there's probably no guarantee that it gets set after
> > commit. In fact, the flag is attached to the Document, so it's also maybe
> > possible (?) for a new Document to get created and the flag to be unset even
> > without a navigation happening at all.
> > 
> > Right now when a Document is created, I'm sending an update if
> > shouldEnforceStrictMixedContentChecking() is true; I think I should be
sending
> > an update either way, so that the frame replication state gets updated to
true
> > if the Document is enforcing strict mixed content checking and false if it
> > isn't. WDYT of that approach?
> 
> Yes, I agree that we should send the update from Blink regardless of whether
the
> flag is true or false, but I think we might not want to tie it to Document
> creation, as in that case we might accidentally set/reset the flag for a
> Document that ends up not committing.
> 
> I've just discussed this with Nasko to be sure, and we think the right
approach
> is probably to set/reset the browser-side bit at commit time.  At that point
in
> time, it's guaranteed that the bit (whatever it is) has taken effect for a
> particular Document and is safe to use for any future subframes that want to
> inherit it, etc.  
> 
> So I think you could just pass it in
> FrameHostMsg_DidCommitProvisionalLoad_Params, whether it's true or false, just
> like the replicated origin.  It sounds like you will still need your
> renderer->browser IPC though, to allow for dynamic
> updates that may happen later.

Ok, that sounds reasonable. I've done that and added some unit tests -- PTAL.

I do think it'll make sense to add browser tests too, but that'll be easier to
do once I've done the next step of adapting MixedContentChecker to work with
RemoteFrames. (Once I've done that, I can write browser tests that load various
pages with mixed content and make sure that the rules get enforced properly.)

[alexmos, 2015-12-11 02:52:27]

Great.  Just a few nits in the test and a couple of other questions.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1189: for (const auto&
pair : *proxy_hosts_) {
Are there other situations in which another frame's strict mixed content flag is
needed, other than inheriting it for new child frames?  
This logic is probably still fine, but I'm just curious how we might go about
optimizing the number of IPCs we send in the future.

Also, is the flag inherited for popups/openers (since proxy_hosts_ might include
those)?  

If inheriting across openers isn't needed, we shouldn't need this in default
Chrome, so it would be good to add a 
  if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())
    return;

before the loop.  (We would only have popup/opener proxies if cross-process
frames are off).

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3149: void
CheckMixedContentIPC(TestRenderFrameHost* rfh, bool expected_param) {
nit: maybe put this on top in the existing anonymous namespace, after
VerifyPageFocusMessage which is kind of similar, or use a lambda to keep it in
the test's body.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3153:
ASSERT_TRUE(message);
Could we also verify that the message's routing ID corresponds to the right
proxy?  See VerifyPageFocusMessage for an example of how to get routing IDs for
proxies.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3198:
main_test_rfh()->OnDidEnforceStrictMixedContentChecking();
Can we also check that the bit in the corresponding replication_state is
correct?

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3203:
ASSERT_NO_FATAL_FAILURE(CheckMixedContentIPC(main_test_rfh(), true));
It might be good to document somewhere (here?) why a parent frame's process
might need to know a child frame's strict mixed content flag.  Am I correct that
it would be needed if we did A-embed-B, B sets the flag, then B embeds another
A?  We would need the flag in process A to properly inherit it for the new
grandchild.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3207:
main_test_rfh()->GetProcess()->sink().ClearMessages();
nit: CheckMixedContentIPC also clears messages in its last line - remove it from
there to keep it side-effect-free?

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3220:
child_host->SendNavigateWithParams(&commit_params);
Similarly, can we check that the replication state bit was reset after this?

[estark, 2015-12-12 01:35:43]

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1189: for (const auto&
pair : *proxy_hosts_) {
On 2015/12/11 02:52:27, alexmos wrote:
> Are there other situations in which another frame's strict mixed content flag
is
> needed, other than inheriting it for new child frames?  
> This logic is probably still fine, but I'm just curious how we might go about
> optimizing the number of IPCs we send in the future.

Well, it's checked whenever mixed content loads in a child frame. That is, if
https://foo.com has a child frame to https://bar.com which tries to load mixed
content, we need to check the https://foo.com flag:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(On that line, |mixedFrame| will be the top-level frame in this case.)

> 
> Also, is the flag inherited for popups/openers (since proxy_hosts_ might
include
> those)?  
> 
> If inheriting across openers isn't needed, we shouldn't need this in default
> Chrome, so it would be good to add a 
>   if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())
>     return;
> 
> before the loop.  (We would only have popup/opener proxies if cross-process
> frames are off).

Done. It's not needed for openers.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3149: void
CheckMixedContentIPC(TestRenderFrameHost* rfh, bool expected_param) {
On 2015/12/11 02:52:27, alexmos wrote:
> nit: maybe put this on top in the existing anonymous namespace, after
> VerifyPageFocusMessage which is kind of similar, or use a lambda to keep it in
> the test's body.

Done.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3153:
ASSERT_TRUE(message);
On 2015/12/11 02:52:27, alexmos wrote:
> Could we also verify that the message's routing ID corresponds to the right
> proxy?  See VerifyPageFocusMessage for an example of how to get routing IDs
for
> proxies.

Done.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3198:
main_test_rfh()->OnDidEnforceStrictMixedContentChecking();
On 2015/12/11 02:52:27, alexmos wrote:
> Can we also check that the bit in the corresponding replication_state is
> correct?

Done.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3203:
ASSERT_NO_FATAL_FAILURE(CheckMixedContentIPC(main_test_rfh(), true));
On 2015/12/11 02:52:27, alexmos wrote:
> It might be good to document somewhere (here?) why a parent frame's process
> might need to know a child frame's strict mixed content flag.  Am I correct
that
> it would be needed if we did A-embed-B, B sets the flag, then B embeds another
> A?  We would need the flag in process A to properly inherit it for the new
> grandchild.

Yeah, that sounds right. Added a comment here.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3207:
main_test_rfh()->GetProcess()->sink().ClearMessages();
On 2015/12/11 02:52:27, alexmos wrote:
> nit: CheckMixedContentIPC also clears messages in its last line - remove it
from
> there to keep it side-effect-free?

Done.

https://codereview.chromium.org/1489253002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3220:
child_host->SendNavigateWithParams(&commit_params);
On 2015/12/11 02:52:27, alexmos wrote:
> Similarly, can we check that the replication state bit was reset after this?

Done.

[alexmos, 2015-12-15 17:49:17]

Thanks, LGTM with nits.

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:538:
ProxiesReceiveShouldEnforceStrictMixedContentChecking);
nit: it's probably a bit cleaner to plumb the
OnDidEnforceStrictMixedContentChecking call in the test via TestRenderFrameHost
(see DidChangeOpener for an example).

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3202:
->GetRenderFrameProxyHost(child_host->GetSiteInstance())
nit: declaring the proxies in separate vars and using that here and below might
make this a bit more readable.

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

https://codereview.chromium.org/1489253002/diff/200001/content/common/frame_m...
content/common/frame_messages.h:829: // Sent when the frame starts enforcing
strict mixed content checking.
nit: maybe also mention why just sending it in DidCommitProvisionalLoad isn't
sufficient and when this is needed.  E.g., when the document dynamically inserts
a <meta> with the CSP.

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1052: bool
shouldEnforceStrictMixedContentChecking() const { return
m_strictMixedContentCheckingEnforced; }
This isn't needed anymore, right?  A Document can get this via SecurityContext,
from which it inherits.

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1398: bool
m_strictMixedContentCheckingEnforced;
ditto

[estark, 2015-12-15 18:25:29]

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

[estark, 2015-12-15 18:25:30]

alexmos: thank you very much for the review! I'll address your nits shortly (my
workstation is currently in transit).

In the meantime...
mkwst: could you please review the blink half for OWNERS?
nasko: could you please review the content half for OWNERS?
For context: this CL is the first baby step towards making the mixed content
check work with OOPIFs. It makes the parent document's strict mixed content
setting available on RemoteFrames.

[estark, 2015-12-15 21:28:50]

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:538:
ProxiesReceiveShouldEnforceStrictMixedContentChecking);
On 2015/12/15 17:49:16, alexmos wrote:
> nit: it's probably a bit cleaner to plumb the
> OnDidEnforceStrictMixedContentChecking call in the test via
TestRenderFrameHost
> (see DidChangeOpener for an example).

Done.

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3202:
->GetRenderFrameProxyHost(child_host->GetSiteInstance())
On 2015/12/15 17:49:16, alexmos wrote:
> nit: declaring the proxies in separate vars and using that here and below
might
> make this a bit more readable.

Done.

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

https://codereview.chromium.org/1489253002/diff/200001/content/common/frame_m...
content/common/frame_messages.h:829: // Sent when the frame starts enforcing
strict mixed content checking.
On 2015/12/15 17:49:17, alexmos wrote:
> nit: maybe also mention why just sending it in DidCommitProvisionalLoad isn't
> sufficient and when this is needed.  E.g., when the document dynamically
inserts
> a <meta> with the CSP.

Done.

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1052: bool
shouldEnforceStrictMixedContentChecking() const { return
m_strictMixedContentCheckingEnforced; }
On 2015/12/15 17:49:17, alexmos wrote:
> This isn't needed anymore, right?  A Document can get this via
SecurityContext,
> from which it inherits.

Done.

https://codereview.chromium.org/1489253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1398: bool
m_strictMixedContentCheckingEnforced;
On 2015/12/15 17:49:17, alexmos wrote:
> ditto

Done.

[Mike West, 2015-12-16 08:22:27]

Blink LGTM, thanks!

[dcheng, 2015-12-16 10:18:18]

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

[dcheng, 2015-12-16 10:18:19]

https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5773: if (frame())
So I have a dumb question.

It looks like the only time frame() will be null is if we're creating a detached
Document via something like document.implementation.createDocument().

What is the effect of enabling CSP on a detached Document? Is there a way to
load this detached Document directly?

[estark, 2015-12-16 21:01:18]

https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5773: if (frame())
On 2015/12/16 10:18:19, dcheng wrote:
> So I have a dumb question.
> 
> It looks like the only time frame() will be null is if we're creating a
detached
> Document via something like document.implementation.createDocument().
> 
> What is the effect of enabling CSP on a detached Document? Is there a way to
> load this detached Document directly?

Hmmm... I have no idea. Mike, do you know the answer to this?

[estark, 2015-12-17 16:17:46]

nasko: friendly ping :)

[nasko, 2015-12-17 17:16:33]

Thanks a lot for tackling this one Emily! Looks good, with mostly nits and
naming.

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:219:
replication_state_.should_enforce_strict_mixed_content_checking =
Is there a specific reason why the update is done after the notification to
proxies?

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1398:
frame_tree_node()->SetShouldEnforceStrictMixedContentChecking(true);
Why do we have mismatched names of the methods? The FTN one contains "Should"
where this method doesn't.

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3202:
ASSERT_NO_FATAL_FAILURE(
s/ASSERT/EXPECT/

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3226: // when
the child navigates within the same process.
Does it matter whether the navigation is cross-process or same process?

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

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_m...
content/common/frame_messages.h:669:
IPC_MESSAGE_ROUTED1(FrameMsg_DidUpdateShouldEnforceStrictMixedContentChecking,
nit: I'd drop the "DidUpdateShould" part of the name. We are telling the proxy
to enforce the checks and the boolean clarifies whether it is on or not.

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_m...
content/common/frame_messages.h:834:
IPC_MESSAGE_ROUTED0(FrameHostMsg_DidEnforceStrictMixedContentChecking)
I find it equally readable, if not more if we drop the "Did", as it is a clear
instruction that checking must be enforced. Here we don't even have an option.

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_r...
content/common/frame_replication_state.h:75: // --site-per-process mode.
nit: s/--site-per-process mode/when frames live in different processes/.

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4190:
frame->shouldEnforceStrictMixedContentChecking();
nit: Why not put it right next to params.security_info? In this location it is
not ordered by struct layout or alphabetically, so combining it with other parts
of the struct that are in the same category (security) can make it easy to look
it up later.

[alexmos, 2015-12-17 17:43:02]

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4190:
frame->shouldEnforceStrictMixedContentChecking();
On 2015/12/17 17:16:33, nasko wrote:
> nit: Why not put it right next to params.security_info? In this location it is
> not ordered by struct layout or alphabetically, so combining it with other
parts
> of the struct that are in the same category (security) can make it easy to
look
> it up later.

Right now this is grouped together with setting the replicated origin, so if we
move this one, we should probably move the origin as well?

[estark, 2015-12-17 22:19:11]

Thanks, nasko.

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:219:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/17 17:16:32, nasko wrote:
> Is there a specific reason why the update is done after the notification to
> proxies?

Not that I know of. I was just following the example of the other similar
methods (SetFrameName(), SetCurrentOrigin(), etc.).

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1398:
frame_tree_node()->SetShouldEnforceStrictMixedContentChecking(true);
On 2015/12/17 17:16:33, nasko wrote:
> Why do we have mismatched names of the methods? The FTN one contains "Should"
> where this method doesn't.

Done.

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3202:
ASSERT_NO_FATAL_FAILURE(
On 2015/12/17 17:16:33, nasko wrote:
> s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3226: // when
the child navigates within the same process.
On 2015/12/17 17:16:33, nasko wrote:
> Does it matter whether the navigation is cross-process or same process?

Hmm, no, I don't think so, not sure why I wrote that. Removed.

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

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_m...
content/common/frame_messages.h:669:
IPC_MESSAGE_ROUTED1(FrameMsg_DidUpdateShouldEnforceStrictMixedContentChecking,
On 2015/12/17 17:16:33, nasko wrote:
> nit: I'd drop the "DidUpdateShould" part of the name. We are telling the proxy
> to enforce the checks and the boolean clarifies whether it is on or not.

Done.

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_m...
content/common/frame_messages.h:834:
IPC_MESSAGE_ROUTED0(FrameHostMsg_DidEnforceStrictMixedContentChecking)
On 2015/12/17 17:16:33, nasko wrote:
> I find it equally readable, if not more if we drop the "Did", as it is a clear
> instruction that checking must be enforced. Here we don't even have an option.

Done.

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1489253002/diff/220001/content/common/frame_r...
content/common/frame_replication_state.h:75: // --site-per-process mode.
On 2015/12/17 17:16:33, nasko wrote:
> nit: s/--site-per-process mode/when frames live in different processes/.

Done.

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4190:
frame->shouldEnforceStrictMixedContentChecking();
On 2015/12/17 17:43:02, alexmos wrote:
> On 2015/12/17 17:16:33, nasko wrote:
> > nit: Why not put it right next to params.security_info? In this location it
is
> > not ordered by struct layout or alphabetically, so combining it with other
> parts
> > of the struct that are in the same category (security) can make it easy to
> look
> > it up later.
> 
> Right now this is grouped together with setting the replicated origin, so if
we
> move this one, we should probably move the origin as well?

Ok, I moved both origin and this up to where security_info is set.

[Mike West, 2015-12-18 07:37:01]

On 2015/12/16 at 21:01:18, estark wrote:
>
https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Document.cpp:5773: if (frame())
> On 2015/12/16 10:18:19, dcheng wrote:
> > So I have a dumb question.
> > 
> > It looks like the only time frame() will be null is if we're creating a
detached
> > Document via something like document.implementation.createDocument().
> > 
> > What is the effect of enabling CSP on a detached Document? Is there a way to
> > load this detached Document directly?
> 
> Hmmm... I have no idea. Mike, do you know the answer to this?

Hrm. Detached documents should inherit the policy of their creators, but since
they don't actually load any resources, I think the risk is low. The creator
document's policy will be enforced if/when the detached document's DOM is
extracted and inserted into the main document, so there's little risk of bypass
here.

[dcheng, 2015-12-18 08:27:14]

On 2015/12/18 at 07:37:01, mkwst wrote:
> On 2015/12/16 at 21:01:18, estark wrote:
> >
https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > 
> >
https://codereview.chromium.org/1489253002/diff/220001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Document.cpp:5773: if (frame())
> > On 2015/12/16 10:18:19, dcheng wrote:
> > > So I have a dumb question.
> > > 
> > > It looks like the only time frame() will be null is if we're creating a
detached
> > > Document via something like document.implementation.createDocument().
> > > 
> > > What is the effect of enabling CSP on a detached Document? Is there a way
to
> > > load this detached Document directly?
> > 
> > Hmmm... I have no idea. Mike, do you know the answer to this?
> 
> Hrm. Detached documents should inherit the policy of their creators, but since
they don't actually load any resources, I think the risk is low. The creator
document's policy will be enforced if/when the detached document's DOM is
extracted and inserted into the main document, so there's little risk of bypass
here.

I wasn't really thinking about risk of bypass, just understanding whether or not
it made sense to enter these codepaths at all for detached Documents. This is in
line with what I expected, but I wanted to confirm that I wasn't missing
something.

To be clear, the current patch is fine, I'm just thinking about how to improve
the state of null checking FrameClient / WebFrameClient as a whole.

[nasko, 2015-12-18 18:27:12]

LGTM

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1489253002/diff/220001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:219:
replication_state_.should_enforce_strict_mixed_content_checking =
On 2015/12/17 22:19:10, estark wrote:
> On 2015/12/17 17:16:32, nasko wrote:
> > Is there a specific reason why the update is done after the notification to
> > proxies?
> 
> Not that I know of. I was just following the example of the other similar
> methods (SetFrameName(), SetCurrentOrigin(), etc.).

Acknowledged.

[estark, 2015-12-18 18:33:18]

The CQ bit was checked by estark@chromium.org

[estark, 2015-12-18 18:33:19]

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

[commit-bot: I haz the power, 2015-12-18 18:34:46]

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

[commit-bot: I haz the power, 2015-12-18 20:10:29]

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

[commit-bot: I haz the power, 2015-12-18 20:10:30]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[estark, 2015-12-18 20:48:11]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-12-18 20:48:53]

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

[commit-bot: I haz the power, 2015-12-18 21:53:15]

Description was changed from

==========
Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

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

to

==========
Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

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

[commit-bot: I haz the power, 2015-12-18 21:53:16]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-12-18 21:54:07]

Description was changed from

==========
Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

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

to

==========
Plumb document's strict mixed content checking for RemoteFrames

When a document's enforcement of strict mixed content checking changes,
propagate the new value to corresponding RemoteFrames. This will allow
MixedContentChecker to work with OOPIFs.

BUG=486936

Committed: https://crrev.com/a886b8d6c10c4b218621fb762026fab7b426f608
Cr-Commit-Position: refs/heads/master@{#366177}
==========

[commit-bot: I haz the power, 2015-12-18 21:54:08]

Patchset 13 (id:??) landed as
https://crrev.com/a886b8d6c10c4b218621fb762026fab7b426f608
Cr-Commit-Position: refs/heads/master@{#366177}