Certificate Transparency: Adding finch and NetLog logging for EV certs

Certificate Transparency: Adding finch and NetLog logging for EV certs

This is a follow-up patch to address some of rsleevi@'s comments on
a previous patch that policy compliance checks for EV certificates.
The remaining todo (adding an integration test to SSLClientSocket*)
will be addressed in a separate patch.

BUG=437766
Committed: https://crrev.com/18a019275b2995f77fbc95691c5784fb2ca83836
Cr-Commit-Position: refs/heads/master@{#308971}

[Eran Messeri, 2014-12-08 20:49:22]

eranm@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2014-12-08 22:42:35]

One comment about net-internals painting, but otherwise looks good.

https://codereview.chromium.org/782333002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/782333002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:332: return true;
Suuuper nitpicky nit that completely doesn't matter: I suppose you may as well
reorder these too. Matches the function above and there's no need to query the
field trial if you're going to bail out on the switch early anyway.

https://codereview.chromium.org/782333002/diff/1/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/782333002/diff/1/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:591: //                         
CERT_VERIFIER_JOB.
I think you need to add a block to
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
so net-internals displays it right.

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
net/cert/cert_policy_enforcer.cc:79: 
Style nit: I probably wouldn't bother with the blank lines since the function's
so short.

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforcer.h
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
net/cert/cert_policy_enforcer.h:39: // |cert| prior to invoking this method.
I'd add a sentence: If it is non-conforming, the certificate is logged to
|net_log|.

[Eran Messeri, 2014-12-09 19:57:45]

eranm@chromium.org changed reviewers:
+ mmenke@chromium.org

[Eran Messeri, 2014-12-09 19:58:15]

Addressed all review comments.
Adding mmenke@ to as a reviewer for
chrome/browser/resources/net_internals/log_view_painter.js and
chrome/browser/io_thread.cc

https://codereview.chromium.org/782333002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/782333002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:332: return true;
On 2014/12/08 22:42:35, David Benjamin wrote:
> Suuuper nitpicky nit that completely doesn't matter: I suppose you may as well
> reorder these too. Matches the function above and there's no need to query the
> field trial if you're going to bail out on the switch early anyway.

My understanding, from go/finch-and-flags, is that you should call FindFullName
regardless of the flag value for UMA to report the correct group.

https://codereview.chromium.org/782333002/diff/1/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/782333002/diff/1/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:591: //                         
CERT_VERIFIER_JOB.
On 2014/12/08 22:42:35, David Benjamin wrote:
> I think you need to add a block to
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> so net-internals displays it right.

Done.

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
net/cert/cert_policy_enforcer.cc:79: 
On 2014/12/08 22:42:35, David Benjamin wrote:
> Style nit: I probably wouldn't bother with the blank lines since the
function's
> so short.

Done.

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforcer.h
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/1/net/cert/cert_policy_enforce...
net/cert/cert_policy_enforcer.h:39: // |cert| prior to invoking this method.
On 2014/12/08 22:42:35, David Benjamin wrote:
> I'd add a sentence: If it is non-conforming, the certificate is logged to
> |net_log|.

Done.

[mmenke, 2014-12-09 20:13:32]

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:603: if
(typeof(entry.params.verified_cert) == 'object') {
I don't think we need 3 copies of basically identical code.

Suggest a function:

function writeCertificateParam(entry, out, consumerParam, paramName) {
  var certs = entry.params[paramName].certificates.reduce(...);
  out.writeArrowKey(paramName);
  out.writeSpaceIndentedLines(8, certs);
  consumedParams[paramName] = true;
}

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:643: 
nit:  Should only be one blank line here.

[davidben, 2014-12-09 20:28:12]

net/ lgtm

[Ryan Sleevi, 2014-12-09 20:50:42]

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

[Ryan Sleevi, 2014-12-09 20:50:43]

Sorry to be "That Guy", but I'm actually going to block this (although hopefully
temporarily).

With NetLog, we really want to log as much data as possible to understand "why"
something is. Here, the "why" is what caused a certificate to be treated as
logged (since we may encounter / need to debug false positives in the future) as
well as what caused it to be not logged, as well as sufficient data to
understand the issue (such as what SCTs were logged / validated / etc)

https://codereview.chromium.org/782333002/diff/20001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/20001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:118: net_log_callback);
Ok, I'm going to push back on this with a "Not LGTM" for now. I know, I know, a
pain.

The problem is that you're only logging a failure path, instead of any of the
other (relevant) details that would be neded during debugging (namely, line 97,
100, 104, 109)

Rather than focusing on a single Event that indicates failure, you should look
to log a start/end event with relevant details. See the CertVerifier as an
example, where the begin event includes the input parameters (for example, the
ct_result, potentially whitelist metadata in the future) and the end event logs
the result (which of the paths this function exited)

[Eran Messeri, 2014-12-10 15:38:39]

Addressed *all* the comments! PTAL.
Hopefully Ryan's concern of not logging all the data is now addressed.
Matt - changed log_view_painter.js as suggested, although I've not yet been able
to see the newly event type rendered. Any guidance on how to verify this is
welcome.

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:603: if
(typeof(entry.params.verified_cert) == 'object') {
On 2014/12/09 20:13:31, mmenke wrote:
> I don't think we need 3 copies of basically identical code.
> 
> Suggest a function:
> 
> function writeCertificateParam(entry, out, consumerParam, paramName) {
>   var certs = entry.params[paramName].certificates.reduce(...);
>   out.writeArrowKey(paramName);
>   out.writeSpaceIndentedLines(8, certs);
>   consumedParams[paramName] = true;
> }

Done.

https://codereview.chromium.org/782333002/diff/20001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:643: 
On 2014/12/09 20:13:32, mmenke wrote:
> nit:  Should only be one blank line here.

Done.

https://codereview.chromium.org/782333002/diff/20001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/20001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:118: net_log_callback);
On 2014/12/09 20:50:42, Ryan Sleevi wrote:
> Ok, I'm going to push back on this with a "Not LGTM" for now. I know, I know,
a
> pain.
> 
> The problem is that you're only logging a failure path, instead of any of the
> other (relevant) details that would be neded during debugging (namely, line
97,
> 100, 104, 109)
> 
> Rather than focusing on a single Event that indicates failure, you should look
> to log a start/end event with relevant details. See the CertVerifier as an
> example, where the begin event includes the input parameters (for example, the
> ct_result, potentially whitelist metadata in the future) and the end event
logs
> the result (which of the paths this function exited)

I understand the main concern is not logging all paths and all relevant details
that are necessary to debug why the EV treatment was not granted.

Per your suggestion I have changed the code to log an event in all circumstances
with all details.

I have tried using BeginEvent/EndEvent but that ended up looking a bit funny, as
in practice the code is much simpler if I do all the checks and NetLog the
result (which contains all the information) in a single place.

[Ryan Sleevi, 2014-12-10 20:09:26]

Logging concerns addressed, so removing my block with an LGTM

Matt should double check he's happy with the logging; I can't help but feel a
little weird at the PolicyComplianceResult vs calling the class simply some log
helper struct, but I fully defer to him, and can live with the code as is.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:137: base::Unretained(&result));
You don't log anything about the ev_whitelist, such as the version that was
fetched.

What about situations where the user is using an old version of the EV
whitelist? What about situations where the user is using a new version of the EV
whitelist? What are the implications to diagnostics? How would that materially
affect this design? For example, it seems somewhat weird to set
"ev_whitelist_version" in the PolicyComplianceResult, but maybe not too weird.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:27: }  // namespace
This is a .h file. You can't use unnamed namespaces here.

Just forward declare this struct as a private member struct of
CertPolicyEnforcer. eg: line 52

private:
  struct CertPolicyEnforcer;

  bool IsCertificateInWhitelist(...);

[mmenke, 2014-12-10 20:36:40]

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:94: void Reset() {
Not needed.  Can just do all this in the constructor.  As-is, you're calling
Reset() twice in a row, which doesn't get you anything.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:102: CTComplianceStatus result;
result->result is a bit ugly.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:102: CTComplianceStatus result;
May want to document these (Disclaimer:  I know nothing about certs, so they may
be obvious to someone who does)

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:113: ComplianceStatusToString(result->result));
Logging this when one of the others is false seems a bit weird, as does logging
build_timely when ct_presence_required is false.

I suggest getting rid of PolicyComplianceResult and adding two values of the
enums instead.  Can then just have CheckCTEVPolicyCompliance return a
CTComplianceStatus.  Think it results in much cleaner code.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:219: void
CertPolicyEnforcer::CheckCTEVPolicyCompliance(
This doesn't seem to need to be a member of CertPolicyEnforcer.  Just move it up
into the anonymous namespace, and out of the header.

[Eran Messeri, 2014-12-10 20:37:04]

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:137: base::Unretained(&result));
On 2014/12/10 20:09:26, Ryan Sleevi wrote:
> You don't log anything about the ev_whitelist, such as the version that was
> fetched.
> 
> What about situations where the user is using an old version of the EV
> whitelist? What about situations where the user is using a new version of the
EV
> whitelist? What are the implications to diagnostics? How would that materially
> affect this design? For example, it seems somewhat weird to set
> "ev_whitelist_version" in the PolicyComplianceResult, but maybe not too weird.

Good point, I'll add logging of the EV whitelist version.

[Eran Messeri, 2014-12-12 12:44:56]

Per your review comments:
- I've converted most methods to functions in the anonymous namespace (as well
as the struct with the compliance check results).
- I've added logging of the EV whitelist version number (although this increases
the size of this patch).
- Only logged the values when they are meaningful.

PTAL, if the version logging in particular looks reasonable I'll add a reviewer
for the component updater code.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:94: void Reset() {
On 2014/12/10 20:36:39, mmenke wrote:
> Not needed.  Can just do all this in the constructor.  As-is, you're calling
> Reset() twice in a row, which doesn't get you anything.

Done.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:102: CTComplianceStatus result;
On 2014/12/10 20:36:40, mmenke wrote:
> result->result is a bit ugly.

Done, changed.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:102: CTComplianceStatus result;
On 2014/12/10 20:36:39, mmenke wrote:
> May want to document these (Disclaimer:  I know nothing about certs, so they
may
> be obvious to someone who does)

Done.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:113: ComplianceStatusToString(result->result));
On 2014/12/10 20:36:40, mmenke wrote:
> Logging this when one of the others is false seems a bit weird, as does
logging
> build_timely when ct_presence_required is false.
> 
> I suggest getting rid of PolicyComplianceResult and adding two values of the
> enums instead.  Can then just have CheckCTEVPolicyCompliance return a
> CTComplianceStatus.  Think it results in much cleaner code.

Acknowledged - it indeed does not make sense to log all values when they may be
meaningless sometimes. I've changed the logging logic to not log in these cases.
I have not added values to the enum because:
(1) It is used for UMA and I would not like to include the cases where the build
timely or enforcement was not required there (it will clutter the histogram -
policy enforcement is completely under our control using finch and build
timeliness is completely out of our control).
(2) There's additional information to log - such as the EV whitelist version,
which Ryan pointed out.

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:219: void
CertPolicyEnforcer::CheckCTEVPolicyCompliance(
On 2014/12/10 20:36:40, mmenke wrote:
> This doesn't seem to need to be a member of CertPolicyEnforcer.  Just move it
up
> into the anonymous namespace, and out of the header.

Done, although that led to a behaviour change as it was calling
HasRequiredNumberOfSCTs which does need a class member. Turns out that was a
desirable change :)

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/40001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:27: }  // namespace
On 2014/12/10 20:09:26, Ryan Sleevi wrote:
> This is a .h file. You can't use unnamed namespaces here.
> 
> Just forward declare this struct as a private member struct of
> CertPolicyEnforcer. eg: line 52
> 
> private:
>   struct CertPolicyEnforcer;
> 
>   bool IsCertificateInWhitelist(...);

Done.

[Eran Messeri, 2014-12-15 17:18:58]

eranm@chromium.org changed reviewers:
+ ryan sleevivi@chromium.org, sorin@chromium.org
- rsleevi@chromium.org

[Eran Messeri, 2014-12-15 17:19:15]

Adding Sorin to review the (small!) component updater change.

[Sorin Jianu, 2014-12-16 03:04:30]

lgtm

Thank you!

[Eran Messeri, 2014-12-16 15:22:49]

eranm@chromium.org changed reviewers:
+ rsleevi@chromium.org
- ryan sleevivi@chromium.org

[mmenke, 2014-12-16 16:40:12]

LGTM (Note:  I only reviewed io_thread.h, the js file, and the logging code of
cert_policy_enforcer.cc)

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:330:
base::FieldTrialList::FindFullName("CTRequiredForEVTrial");
nit:  Should put this just before use, per Google style guide.  Admittedly,
doesn't really matter for performance, but still seems weird to always do it
even when we have a switch and end up ignoring the result.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:610:
consumedParams.certificates = true;
This can use writeCertificateParam, too, I believe.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:616: }
nit:  Don't use braces on single line if's (x2)

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:61: bool
HasRequiredNumberOfSCTs(X509Certificate* cert,
optional nit:  This should probably be const X509Certificate& cert

[Ryan Sleevi, 2014-12-16 21:35:01]

Still LGTM

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist_unittest.cc:132: new
PackedEVCertsWhitelist("", base::Version()));
s/""/std::string/ ;)

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:35: // is logged to |net_log|.
This comment isn't accurate, since you always log to |net_log| ;)

https://codereview.chromium.org/782333002/diff/80001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:11: #include "base/version.h"
You don't need to include this; you can forward declare base::Version in this
header.

[Eran Messeri, 2014-12-17 16:19:31]

Thanks for the quick review everyone, addressed all your comments.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:330:
base::FieldTrialList::FindFullName("CTRequiredForEVTrial");
On 2014/12/16 16:40:12, mmenke wrote:
> nit:  Should put this just before use, per Google style guide.  Admittedly,
> doesn't really matter for performance, but still seems weird to always do it
> even when we have a switch and end up ignoring the result.

IIUC in go/finch-and-flags this pattern is explicitly stated for UMA to report
the correct group with each histogram logging.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist_unittest.cc:132: new
PackedEVCertsWhitelist("", base::Version()));
On 2014/12/16 21:35:01, Ryan Sleevi wrote:
> s/""/std::string/ ;)

Done.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:610:
consumedParams.certificates = true;
On 2014/12/16 16:40:12, mmenke wrote:
> This can use writeCertificateParam, too, I believe.

Not quite - this one takes the certificates from entry.params.certificates while
the others take it from entry.params.something.certificates. I've made a slight
change to accommodate both cases.

https://codereview.chromium.org/782333002/diff/80001/chrome/browser/resources...
chrome/browser/resources/net_internals/log_view_painter.js:616: }
On 2014/12/16 16:40:12, mmenke wrote:
> nit:  Don't use braces on single line if's (x2)

Done and done.

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:61: bool
HasRequiredNumberOfSCTs(X509Certificate* cert,
On 2014/12/16 16:40:12, mmenke wrote:
> optional nit:  This should probably be const X509Certificate& cert

Done, here and in IsCertificateInWhitelist

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:35: // is logged to |net_log|.
On 2014/12/16 21:35:01, Ryan Sleevi wrote:
> This comment isn't accurate, since you always log to |net_log| ;)

Done - fixed.

https://codereview.chromium.org/782333002/diff/80001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/782333002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:11: #include "base/version.h"
On 2014/12/16 21:35:01, Ryan Sleevi wrote:
> You don't need to include this; you can forward declare base::Version in this
> header.

Done.

[Eran Messeri, 2014-12-18 08:29:00]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-12-18 08:29:43]

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

[commit-bot: I haz the power, 2014-12-18 08:43:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-18 08:44:15]

Patchset 6 (id:??) landed as
https://crrev.com/18a019275b2995f77fbc95691c5784fb2ca83836
Cr-Commit-Position: refs/heads/master@{#308971}