Block platform verification and file IO in the CDM adapter if the CDM configuration disallows them.

Block platform verification and file IO in the CDM adapter if the CDM configuration disallows access to them.

Based on the configured value of distinctiveIdentifier and persistentState, we compute permission bits and pass them through PPAPI to the CDM adapter. This enables us to enforce 'not-allowed' configs even when user permission has been granted.

BUG=487452
Committed: https://crrev.com/f92575a5735f510fb159f3f1928f614e31c8a0f8
Cr-Commit-Position: refs/heads/master@{#319798}

[sandersd (OOO until July 31), 2015-03-06 21:12:02]

[email protected] changed reviewers:
+ [email protected], [email protected]

[sandersd (OOO until July 31), 2015-03-06 21:15:15]

Patchset #1 (id:1) has been deleted

[jrummell, 2015-03-06 21:40:48]

lgtm w/nits.

https://codereview.chromium.org/985113003/diff/20001/media/blink/cdm_session_...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/cdm_session_...
media/blink/cdm_session_adapter.h:36: bool allow_distincitve_identifier,
s/citve/ctive/

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:72:
blink::WebMediaKeySystemConfiguration::Requirement::Required);
Should Requirement::Optional be allowed here? If it's optional I would assume it
is allowed.

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:345: 
nit: I don't think 2 blank lines are needed.

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:30: * @param[in]
allow_perisistent_state Instruct the CDM to allow storing
s/peris/pers/

[ddorwin, 2015-03-06 21:42:53]

[email protected] changed reviewers:
+ [email protected]

[ddorwin, 2015-03-06 21:42:54]

LG overall. Thansk.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:27: bool
allow_distinctive_identifier,
Lots of passing lots of parameters around. We might consider a Params object in
the future. On the other hand, this ensures that we are actually handling new
values in each implementation of MediaKeys (see comments in the factory).

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:47: // TODO(jrummell): Pass
|security_origin| to all constructors.
Regarding a Params object, that would make it harder to see whether we are
always handling parameters like this or the new ones you are adding.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:53: return
scoped_ptr<media::MediaKeys>(new media::AesDecryptor(
DCHECK here or pass in. Passing would abstract whether we have persistent
session support, but is perhaps unnecessary complexity at this point.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:355: // TODO(jrummell):
Remove |session_ready_cb| and |session_keys_change_cb|.
Can we do this now?
I guess it doesn't need to be this CL as long as we're not passing them or
related things across the PPAPI interface.

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmodule_impl.h:32: static void Create(CdmFactory*
cdm_factory,
Why is this non-const param first?

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:345: 
remove

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1117: // verification so that no distinctive
identifier will be available in the
... verification to prevent access to such an identifier.

(It's unclear what "in the first place" means. I think the text above is better
overall.)

https://codereview.chromium.org/985113003/diff/20001/media/cdm/proxy_decrypto...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/cdm/proxy_decrypto...
media/cdm/proxy_decryptor.cc:201: true,  // Prefixed always requires permission,
and always allows
This will DCHECK for AesDecryptor - or at least it will if you add those
DCHECKs. Perhaps a TODO there?
Also, aren't we moving the permission check here at some point? Perhaps TODO to
use the result of that (crbug.com/455271).

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:28: * @param[in]
allow_distinctive_identifier Instruct the CDM to allow using a
Inform the CDM that it may use a...

This is more consistent with our CDM_8 plans. The fact that prevention in the
adapter is more of an implementation detail than part of the API.

Ditto below.

[dmichael (off chromium), 2015-03-06 22:32:31]

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:37: [in] PP_Bool
allow_persistent_state);
Are we trusting the plugin to do the right thing, or is this just advisory?
Meaning...  if a CDM goes rogue and decides to use a distinctive identifier when
it's not allowed, we're going to prevent that at the appropriate API (remote
attestation???), right? (Note that the CDM adapter is no protection; a CDM could
go around it and talk to Pepper directly if it wanted to. Putting protection in
the CDM adapter doesn't seem to prevent abuse at all. Either we trust the CDMs,
in which case the flags don't seem necessary at all. Or...  we don't trust the
CDMs, and we should be preventing the use of the relevant Pepper APIs, which we
can do at a more trusted level like the browser process.)

Assuming that's the case, does the CDM already know a priori whether it should
ask for a distinctive identifier? I would have guessed that a CDM either wants a
distinctive identifier  or it doesn't, and that this flag would not be
meaningful to the CDM (except for identifying the error case where it *wants*
the distinctive identifier but isn't allowed to get it).

So...  backing up, is it possible that we don't need this flag at all, and that
CDMs that want a distinctive identifier just tries to use the appropriate API?

(Similar questions for allow_persistent_state, I guess...  doesn't the CDM
already know if it will use persistent state? Or will CDMs actually make
decisions based off of this flag?)

Please let me know if I'm misunderstanding this.

[sandersd (OOO until July 31), 2015-03-06 22:36:59]

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:27: bool
allow_distinctive_identifier,
On 2015/03/06 21:42:54, ddorwin wrote:
> Lots of passing lots of parameters around. We might consider a Params object
in
> the future. On the other hand, this ensures that we are actually handling new
> values in each implementation of MediaKeys (see comments in the factory).

Acknowledged.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:47: // TODO(jrummell): Pass
|security_origin| to all constructors.
On 2015/03/06 21:42:54, ddorwin wrote:
> Regarding a Params object, that would make it harder to see whether we are
> always handling parameters like this or the new ones you are adding.

Acknowledged.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:53: return
scoped_ptr<media::MediaKeys>(new media::AesDecryptor(
On 2015/03/06 21:42:54, ddorwin wrote:
> DCHECK here or pass in. Passing would abstract whether we have persistent
> session support, but is perhaps unnecessary complexity at this point.

Done.

https://codereview.chromium.org/985113003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/985113003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:355: // TODO(jrummell):
Remove |session_ready_cb| and |session_keys_change_cb|.
On 2015/03/06 21:42:54, ddorwin wrote:
> Can we do this now?
> I guess it doesn't need to be this CL as long as we're not passing them or
> related things across the PPAPI interface.

After discussion with John, I've removed the TODO.

https://codereview.chromium.org/985113003/diff/20001/media/blink/cdm_session_...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/cdm_session_...
media/blink/cdm_session_adapter.h:36: bool allow_distincitve_identifier,
On 2015/03/06 21:40:47, jrummell wrote:
> s/citve/ctive/

Done.

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmodule_impl.h:32: static void Create(CdmFactory*
cdm_factory,
On 2015/03/06 21:42:54, ddorwin wrote:
> Why is this non-const param first?

Presumably for symmetry with CdmSessionAdapter::Initialize(). Ideally it would
be const, but CdmFactory::Create is not a const method (I can't imagine why it
would need to be mutable though).

I can move it down in this CL, but the reality is that it's treated as const,
and if it were const it is already in the right place.

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:72:
blink::WebMediaKeySystemConfiguration::Requirement::Required);
On 2015/03/06 21:40:47, jrummell wrote:
> Should Requirement::Optional be allowed here? If it's optional I would assume
it
> is allowed.

No, an accumulated configuration never has Optional.

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:345: 
On 2015/03/06 21:40:47, jrummell wrote:
> nit: I don't think 2 blank lines are needed.

Done.

https://codereview.chromium.org/985113003/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1117: // verification so that no distinctive
identifier will be available in the
On 2015/03/06 21:42:54, ddorwin wrote:
> ... verification to prevent access to such an identifier.
> 
> (It's unclear what "in the first place" means. I think the text above is
better
> overall.)

Done.

https://codereview.chromium.org/985113003/diff/20001/media/cdm/proxy_decrypto...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/cdm/proxy_decrypto...
media/cdm/proxy_decryptor.cc:201: true,  // Prefixed always requires permission,
and always allows
On 2015/03/06 21:42:54, ddorwin wrote:
> This will DCHECK for AesDecryptor - or at least it will if you add those
> DCHECKs. Perhaps a TODO there?
> Also, aren't we moving the permission check here at some point? Perhaps TODO
to
> use the result of that (crbug.com/455271).

Done.

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:28: * @param[in]
allow_distinctive_identifier Instruct the CDM to allow using a
On 2015/03/06 21:42:54, ddorwin wrote:
> Inform the CDM that it may use a...
> 
> This is more consistent with our CDM_8 plans. The fact that prevention in the
> adapter is more of an implementation detail than part of the API.
> 
> Ditto below.

Done.

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:30: * @param[in]
allow_perisistent_state Instruct the CDM to allow storing
On 2015/03/06 21:40:47, jrummell wrote:
> s/peris/pers/

Done.

[sandersd (OOO until July 31), 2015-03-06 22:50:52]

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/985113003/diff/20001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:37: [in] PP_Bool
allow_persistent_state);
I've just updated the CL description to explain this a little bit, but I'll
answer in detail here.

> Are we trusting the plugin to do the right thing, or is this just advisory?

This is advisory[ish]. Actual use of these APIs is determined by user permission
(for distinctive identifier) or browser state (incognito for persistent state,
but the plan is to add more restrictions).

> Assuming that's the case, does the CDM already know a priori whether it should
> ask for a distinctive identifier?

Not really. The spec is intended to allow us to do all user permission prompting
before creating the CDM instance, and a possible outcome is that we disallow
access even though the user has granted permission. In this case we want to
block access even if the media requests it.

> So...  backing up, is it possible that we don't need this flag at all, and
that
> CDMs that want a distinctive identifier just tries to use the appropriate API?

In summary, no. User permission is not sufficient (by the EME spec) to allow
these CDM features.

> (Similar questions for allow_persistent_state, I guess...  doesn't the CDM
> already know if it will use persistent state? Or will CDMs actually make
> decisions based off of this flag?)

The CDM will make a decision based on the media, which can differ from the
negotiated configuration. An ugly thorn here is that the current CrOS Widevine
CDM will provide access to a distinctive identifier whenever it can; it always
attempts platform verification but doesn't necessarily fail to play media if
verification fails.

[ddorwin, 2015-03-07 02:06:24]

lgtm

Thanks.

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:72:
blink::WebMediaKeySystemConfiguration::Requirement::Required);
On 2015/03/06 22:36:58, sandersd wrote:
> On 2015/03/06 21:40:47, jrummell wrote:
> > Should Requirement::Optional be allowed here? If it's optional I would
assume
> it
> > is allowed.
> 
> No, an accumulated configuration never has Optional.

We could DCHECK != Optional before line 70. Up to you.

https://codereview.chromium.org/985113003/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/985113003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:56: // them and should never
be configured that way.
Reference 249976 so we can find all these and fix them.
Actually, we may be able to add the DCHECKs sooner by fixing 455271.

[sandersd (OOO until July 31), 2015-03-09 18:01:46]

[email protected] changed reviewers:
+ [email protected]

[sandersd (OOO until July 31), 2015-03-09 18:01:46]

[email protected]: Please review changes in ppapi/proxy/ppapi_messages.h.

[dmichael (off chromium), 2015-03-09 19:58:47]

ppapi lgtm

[sandersd (OOO until July 31), 2015-03-09 20:01:36]

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/985113003/diff/20001/media/blink/webcontentde...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:72:
blink::WebMediaKeySystemConfiguration::Requirement::Required);
On 2015/03/07 02:06:24, ddorwin wrote:
> On 2015/03/06 22:36:58, sandersd wrote:
> > On 2015/03/06 21:40:47, jrummell wrote:
> > > Should Requirement::Optional be allowed here? If it's optional I would
> assume
> > it
> > > is allowed.
> > 
> > No, an accumulated configuration never has Optional.
> 
> We could DCHECK != Optional before line 70. Up to you.

Done.

https://codereview.chromium.org/985113003/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/985113003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:56: // them and should never
be configured that way.
On 2015/03/07 02:06:24, ddorwin wrote:
> Reference 249976 so we can find all these and fix them.
> Actually, we may be able to add the DCHECKs sooner by fixing 455271.

Done.

[dcheng, 2015-03-09 20:57:23]

rs lgtm

Has there been any consideration of using enums instead of bools? Is there a
convention in ppapi?

https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1124: linked_ptr<PepperPlatformChallengeResponse>
response(
Are there any plans to make PPAPI's callback system support a similar set of
primitives as base::Callback so that we'd be able to use scoped_ptr (and
eventually unique_ptr) here?

[dmichael (off chromium), 2015-03-09 21:02:07]

https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1124: linked_ptr<PepperPlatformChallengeResponse>
response(
On 2015/03/09 20:57:23, dcheng wrote:
> Are there any plans to make PPAPI's callback system support a similar set of
> primitives as base::Callback so that we'd be able to use scoped_ptr (and
> eventually unique_ptr) here?
No...  it's not really worth the effort and complexity to do that. Though it
would be fun.

[dcheng, 2015-03-09 21:04:46]

On 2015/03/09 at 21:02:07, dmichael wrote:
>
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> File media/cdm/ppapi/cdm_adapter.cc (right):
> 
>
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> media/cdm/ppapi/cdm_adapter.cc:1124:
linked_ptr<PepperPlatformChallengeResponse> response(
> On 2015/03/09 20:57:23, dcheng wrote:
> > Are there any plans to make PPAPI's callback system support a similar set of
> > primitives as base::Callback so that we'd be able to use scoped_ptr (and
> > eventually unique_ptr) here?
> No...  it's not really worth the effort and complexity to do that. Though it
would be fun.

Would the existing system work with unique_ptr though? It doesn't seem like it.
I think it's worth the effort to do that--it'd certainly be nice to get rid of
linked_ptr, which would be obsolete with unique_ptr and move support.

[dmichael (off chromium), 2015-03-09 21:38:12]

On 2015/03/09 21:04:46, dcheng wrote:
> On 2015/03/09 at 21:02:07, dmichael wrote:
> >
>
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> > File media/cdm/ppapi/cdm_adapter.cc (right):
> > 
> >
>
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> > media/cdm/ppapi/cdm_adapter.cc:1124:
> linked_ptr<PepperPlatformChallengeResponse> response(
> > On 2015/03/09 20:57:23, dcheng wrote:
> > > Are there any plans to make PPAPI's callback system support a similar set
of
> > > primitives as base::Callback so that we'd be able to use scoped_ptr (and
> > > eventually unique_ptr) here?
> > No...  it's not really worth the effort and complexity to do that. Though it
> would be fun.
> 
> Would the existing system work with unique_ptr though? It doesn't seem like
it.
> I think it's worth the effort to do that--it'd certainly be nice to get rid of
> linked_ptr, which would be obsolete with unique_ptr and move support.
I didn't think we could use C++11 library features yet. If we could, I could see
adding unique_ptr support to CompletionCallbackFactory and friends. That would
be good for Pepper consumers in general. Meanwhile, I suppose they could use
shared_ptr instead of linked_ptr.

[dcheng, 2015-03-09 21:39:56]

On 2015/03/09 at 21:38:12, dmichael wrote:
> On 2015/03/09 21:04:46, dcheng wrote:
> > On 2015/03/09 at 21:02:07, dmichael wrote:
> > >
> >
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> > > File media/cdm/ppapi/cdm_adapter.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/985113003/diff/80001/media/cdm/ppapi/cdm_adap...
> > > media/cdm/ppapi/cdm_adapter.cc:1124:
> > linked_ptr<PepperPlatformChallengeResponse> response(
> > > On 2015/03/09 20:57:23, dcheng wrote:
> > > > Are there any plans to make PPAPI's callback system support a similar
set of
> > > > primitives as base::Callback so that we'd be able to use scoped_ptr (and
> > > > eventually unique_ptr) here?
> > > No...  it's not really worth the effort and complexity to do that. Though
it
> > would be fun.
> > 
> > Would the existing system work with unique_ptr though? It doesn't seem like
it.
> > I think it's worth the effort to do that--it'd certainly be nice to get rid
of
> > linked_ptr, which would be obsolete with unique_ptr and move support.
> I didn't think we could use C++11 library features yet. If we could, I could
see adding unique_ptr support to CompletionCallbackFactory and friends. That
would be good for Pepper consumers in general. Meanwhile, I suppose they could
use shared_ptr instead of linked_ptr.

Right, unique_ptr isn't on the table quite yet, but it will be at some point in
the future. As long as we don't mind supporting that when the time comes, then
that sgtm =)

[sandersd (OOO until July 31), 2015-03-09 22:22:02]

The CQ bit was checked by [email protected]

[sandersd (OOO until July 31), 2015-03-09 22:22:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
[email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/985113003/#ps80001 (title:
"Comments")

[commit-bot: I haz the power, 2015-03-09 22:22:20]

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

[commit-bot: I haz the power, 2015-03-09 23:04:43]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2015-03-09 23:04:44]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[sandersd (OOO until July 31), 2015-03-09 23:30:59]

The CQ bit was checked by [email protected]

[sandersd (OOO until July 31), 2015-03-09 23:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
[email protected], [email protected], [email protected],
[email protected]
Link to the patchset: https://codereview.chromium.org/985113003/#ps100001
(title: "Fix DCHECK types.")

[commit-bot: I haz the power, 2015-03-09 23:32:18]

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

[commit-bot: I haz the power, 2015-03-10 00:56:19]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-03-10 00:57:06]

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