ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad kills

ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
Committed: https://crrev.com/470457d18d306a0f32ea548ad111371c24e509e1
Cr-Commit-Position: refs/heads/master@{#391678}

[ncarter (slow), 2016-05-04 18:10:18]

nick@chromium.org changed reviewers:
+ creis@chromium.org

[ncarter (slow), 2016-05-04 18:10:18]

Charlie, PTAL

[Charlie Reis, 2016-05-04 19:38:17]

LGTM if we remove the NOTREACHED.

https://codereview.chromium.org/1945173003/diff/1/content/browser/child_proce...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1945173003/diff/1/content/browser/child_proce...
content/browser/child_process_security_policy_impl.cc:823: NOTREACHED();
Let's remove the NOTREACHED.  Looks like this is hit in the tests, plus we know
it happens in practice today.  We'll just have to think about how to deal with
it or make it not possible after this CL.

[Charlie Reis, 2016-05-04 19:38:31]

Description was changed from

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
==========

to

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
==========

[ncarter (slow), 2016-05-04 19:57:57]

https://codereview.chromium.org/1945173003/diff/1/content/browser/child_proce...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1945173003/diff/1/content/browser/child_proce...
content/browser/child_process_security_policy_impl.cc:823: NOTREACHED();
On 2016/05/04 19:38:17, Charlie Reis wrote:
> Let's remove the NOTREACHED.  Looks like this is hit in the tests, plus we
know
> it happens in practice today.  We'll just have to think about how to deal with
> it or make it not possible after this CL.

The test that hits it, also fails if we remove the NOTREACHED(). I've edited the
test.

My hope was that the NOTREACHED() might help us track down repro steps.

[dcheng, 2016-05-04 20:00:40]

Drive-by.

https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc
(right):

https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc:24:
const char OTHER_SITE[] = "https://other.com";
=/

Can we name the new constants with the correct naming style rather than
continuing this?

[ncarter (slow), 2016-05-04 20:11:20]

On 2016/05/04 20:00:40, dcheng wrote:
> Drive-by.
> 
>
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
> File
> content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc:24:
> const char OTHER_SITE[] = "https://other.com";
> =/
> 
> Can we name the new constants with the correct naming style rather than
> continuing this?

Not in this patch, I'm hoping to merge it.

[dcheng, 2016-05-04 20:14:46]

On 2016/05/04 at 20:11:20, nick wrote:
> On 2016/05/04 20:00:40, dcheng wrote:
> > Drive-by.
> > 
> >
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
> > File
> > content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc
> > (right):
> > 
> >
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
> >
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc:24:
> > const char OTHER_SITE[] = "https://other.com";
> > =/
> > 
> > Can we name the new constants with the correct naming style rather than
> > continuing this?
> 
> Not in this patch, I'm hoping to merge it.

Using kNamingStyle for just the new constants shouldn't affect mergability. I'm
OK with eventual consistency here.

[ncarter (slow), 2016-05-04 20:19:30]

On 2016/05/04 20:14:46, dcheng wrote:
> On 2016/05/04 at 20:11:20, nick wrote:
> > On 2016/05/04 20:00:40, dcheng wrote:
> > > Drive-by.
> > > 
> > >
>
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
> > > File
> > >
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/1945173003/diff/20001/content/browser/rendere...
> > >
>
content/browser/renderer_host/media/webrtc_identity_service_host_unittest.cc:24:
> > > const char OTHER_SITE[] = "https://other.com";
> > > =/
> > > 
> > > Can we name the new constants with the correct naming style rather than
> > > continuing this?
> > 
> > Not in this patch, I'm hoping to merge it.
> 
> Using kNamingStyle for just the new constants shouldn't affect mergability.
I'm
> OK with eventual consistency here.

A dependent patch w/ the fix is already in your inbox.

[Charlie Reis, 2016-05-04 20:27:34]

LGTM

[ncarter (slow), 2016-05-04 20:36:52]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-05-04 20:37:12]

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

[commit-bot: I haz the power, 2016-05-04 23:06:32]

Description was changed from

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
==========

to

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
==========

[commit-bot: I haz the power, 2016-05-04 23:06:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-04 23:08:33]

Description was changed from

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441
==========

to

==========
ChildProcessSecurityPolicy::CanAccessDataForOrigin workaround to suppress bad
kills

crbug.com/600441 apparently involves a race where we check
ChildProcessSecurityPolicy for a process_id that it doesn't know -- probably
a renderer shutdown race. Returning true instead of false here is a temporary
workaround to suppress these bad kills, which are affecting the non-oopif
user population. This is a tolerable short-term behavior as far as security
is concerned, since CanAccessDataForOrigin is only meant to offer meaningful
protection in --site-per-process mode, which is not yet launched.

BUG=600441

Committed: https://crrev.com/470457d18d306a0f32ea548ad111371c24e509e1
Cr-Commit-Position: refs/heads/master@{#391678}
==========

[commit-bot: I haz the power, 2016-05-04 23:08:34]

Patchset 3 (id:??) landed as
https://crrev.com/470457d18d306a0f32ea548ad111371c24e509e1
Cr-Commit-Position: refs/heads/master@{#391678}