Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content checks.

Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f672e7ef4c5e3227c2874447ad992dc06f48337d
Cr-Commit-Position: refs/heads/master@{#399008}

[Mike West, 2016-06-07 13:53:23]

Description was changed from

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
==========

to

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-07 13:55:54]

mkwst@chromium.org changed reviewers:
+ alexmos@chromium.org, jochen@chromium.org, yoav@yoav.ws

[Mike West, 2016-06-07 13:55:55]

yoav@: This is the last in the series you've been looking at today. Would you
mind looking at the Blink bits?

alexmos@: Would you mind taking a look at the new replication code?

jochen@: Would you mind evaluating the sanity of the DEPS changes?
WebInsecureRequestPolicy is basically just a typedef at the moment; it'll
eventually be an enum, but there are some bugs to get through first.

Thanks!

[Yoav Weiss, 2016-06-07 14:13:14]

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:664:
frame()->securityContext()->setInsecureRequestPolicy(policy);
Is merging of both policies no longer required?

[Mike West, 2016-06-07 15:56:24]

Apparently I didn't compile all targets. *sigh* I'll fix that tomorrow. :)

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:664:
frame()->securityContext()->setInsecureRequestPolicy(policy);
On 2016/06/07 at 14:13:14, Yoav Weiss wrote:
> Is merging of both policies no longer required?

The previous iteration didn't merge the policies; it overwrote the "block" bit
according to the boolean that was passed in. Since we're now replicating the
whole thing, we don't need to merge.

Note that there's no actual document here: this is populating the
RemoteSecurityContext that's sitting on the RemoteFrame. The actual values are
coming from a distinct process. There's nothing to merge with; the browser is
the canonical source.

[alexmos, 2016-06-07 19:00:06]

Thanks for updating this!  Just a few nits below.  LGTM once the unit test
compile error is fixed.

Also, +site-isolation-reviews.

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3022:
ProxiesReceiveShouldEnforceInsecureRequestPolicy) {
nit: can probably just shorten this to ProxiesReceiveInsecureRequestPolicy.

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3056:
main_test_rfh()->SetInsecureRequestPolicy(blink::kBlockAllMixedContent);
DidEnforceInsecureRequestPolicy?

https://codereview.chromium.org/2046733003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2046733003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6157: GURL
iframe_url_with_strict_blocking(https_server.GetURL(
s/iframe_url_with_strict_blocking/iframe_url_with_upgrade/

https://codereview.chromium.org/2046733003/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2046733003/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:99: // The insecure request policy that
a frame's current is enforcing.
Missing a word?  A frame's current >document< is enforcing.

https://codereview.chromium.org/2046733003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2046733003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.h:173: blink::WebInsecureRequestPolicy
should_enforce);
s/should_enforce/policy/

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:189: virtual void
didEnforceInsecureRequestPolicy(WebInsecureRequestPolicy policy) {}
nit: can drop "policy" from declaration.

[Mike West, 2016-06-08 07:00:27]

Description was changed from

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
==========

to

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-08 07:21:12]

Thanks Alex! Addressed nits, fixed compilation error. Let's see what the bots
say.

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3022:
ProxiesReceiveShouldEnforceInsecureRequestPolicy) {
On 2016/06/07 at 19:00:05, alexmos wrote:
> nit: can probably just shorten this to ProxiesReceiveInsecureRequestPolicy.

Done.

https://codereview.chromium.org/2046733003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3056:
main_test_rfh()->SetInsecureRequestPolicy(blink::kBlockAllMixedContent);
On 2016/06/07 at 19:00:05, alexmos wrote:
> DidEnforceInsecureRequestPolicy?

Done.

https://codereview.chromium.org/2046733003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2046733003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6157: GURL
iframe_url_with_strict_blocking(https_server.GetURL(
On 2016/06/07 at 19:00:06, alexmos wrote:
> s/iframe_url_with_strict_blocking/iframe_url_with_upgrade/

Done.

https://codereview.chromium.org/2046733003/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2046733003/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:99: // The insecure request policy that
a frame's current is enforcing.
On 2016/06/07 at 19:00:06, alexmos wrote:
> Missing a word?  A frame's current >document< is enforcing.

Done.

https://codereview.chromium.org/2046733003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2046733003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.h:173: blink::WebInsecureRequestPolicy
should_enforce);
On 2016/06/07 at 19:00:06, alexmos wrote:
> s/should_enforce/policy/

Done.

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:189: virtual void
didEnforceInsecureRequestPolicy(WebInsecureRequestPolicy policy) {}
On 2016/06/07 at 19:00:06, alexmos wrote:
> nit: can drop "policy" from declaration.

Indeed!

[Yoav Weiss, 2016-06-08 07:54:43]

On 2016/06/07 15:56:24, Mike West (OOO until 30th) wrote:
> Apparently I didn't compile all targets. *sigh* I'll fix that tomorrow. :)
> 
>
https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/2046733003/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:664:
> frame()->securityContext()->setInsecureRequestPolicy(policy);
> On 2016/06/07 at 14:13:14, Yoav Weiss wrote:
> > Is merging of both policies no longer required?
> 
> The previous iteration didn't merge the policies; it overwrote the "block" bit
> according to the boolean that was passed in. Since we're now replicating the
> whole thing, we don't need to merge.
> 
> Note that there's no actual document here: this is populating the
> RemoteSecurityContext that's sitting on the RemoteFrame. The actual values are
> coming from a distinct process. There's nothing to merge with; the browser is
> the canonical source.

Right, I missed the method name change and got myself confused. 

core/ LGTM (and web/ looks good as well but I'm not an owner there)

[jochen (gone - plz use gerrit), 2016-06-08 15:03:15]

lgtm

[Mike West, 2016-06-09 17:53:26]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-09 17:53:27]

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/2046733003/#ps20001
(title: "alexmos@")

[commit-bot: I haz the power, 2016-06-09 17:54:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046733003/20001

[commit-bot: I haz the power, 2016-06-09 20:51:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-09 20:53:20]

Description was changed from

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replicate WebInsecureRequestPolicy instead of a bool for strict mixed content
checks.

This patch is huge, but fairly mechanically replaces the bool sitting on
the replicated frame state with WebInsecureRequestPolicy (which is currently
a bitfield, but will eventually be an enum once https://crbug.com/617584
is addressed). This bitfield holds both 'upgrade-insecure-requests' and
'block-all-mixed-content' state, which means we can take care of replicating
both in one shot.

[Step 1]: https://codereview.chromium.org/2046523005
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: This patch.

BUG=617947
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-06-09 20:53:22]

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