[Win] Count number of distinct certificates covering third party modules.

[Win] Count number of distinct certificates covering third party modules.

The real cost in validating signed modules is validating individual certificates. Since many certificates are provided en masse by catalogs, this counts catalogs as providing a single certificate.

BUG=617176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

[chrisha, 2016-10-04 17:12:16]

Description was changed from

==========
[Win] Count number of distinct certificates covering third party modules.

The real cost in validating signed modules is validating individual
certificates. Since many certificates are provided en masse by catalogs, this
counts catalogs as providing a single certificate.

BUG=617176
==========

to

==========
[Win] Count number of distinct certificates covering third party modules.

The real cost in validating signed modules is validating individual
certificates. Since many certificates are provided en masse by catalogs, this
counts catalogs as providing a single certificate.

BUG=617176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[chrisha, 2016-10-04 17:16:45]

chrisha@chromium.org changed reviewers:
+ gab@chromium.org, rkaplow@chromium.org

[chrisha, 2016-10-04 17:16:46]

PTAL?

[rkaplow, 2016-10-04 17:19:27]

lgtm

[gab, 2016-10-04 19:40:40]

lgtm % comments

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:316: cert_info->subject.clear();
DCHECK that it's empty instead? No user seems to be re-using and I think it'd be
less error prone to always use an empty one (e.g. avoids forgetting to clear a
new field in the future).

Or we could just return by value since you're just copying all fields at the end
of this method anyways (and copy elision on return should make this equivalent)?

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:826:
data->SetString("digital_signer", module->cert_info.subject);
Should we rename this? Can't find any internal tool that parses this. Only
reference I found is in chrome/browser/resources/about_conflicts.html.

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model_unittest.cc (right):

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model_unittest.cc:121: }
Can we test the new code?

FYI testing through histogram is possible, e.g.
https://cs.chromium.org/chromium/src/chrome/browser/prefs/tracked/pref_hash_b...,
might require custom setup in a unit test though not sure.

[chrisha, 2016-10-04 21:47:50]

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:316: cert_info->subject.clear();
On 2016/10/04 19:40:39, gab wrote:
> DCHECK that it's empty instead? No user seems to be re-using and I think it'd
be
> less error prone to always use an empty one (e.g. avoids forgetting to clear a
> new field in the future).
> 
> Or we could just return by value since you're just copying all fields at the
end
> of this method anyways (and copy elision on return should make this
equivalent)?

Good point. Moving to DCHECKs.

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:826:
data->SetString("digital_signer", module->cert_info.subject);
On 2016/10/04 19:40:39, gab wrote:
> Should we rename this? Can't find any internal tool that parses this. Only
> reference I found is in chrome/browser/resources/about_conflicts.html.

Yeah, it's only used in the chrome://conflicts UI. digital_signer is as a good a
name as any for this, so I didn't change it. It could also be
certificate_subject, but that's got less meaning in the context of that UI.

I don't feel too strongly about this one way or the other.

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model_unittest.cc (right):

https://codereview.chromium.org/2384413004/diff/20001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model_unittest.cc:121: }
On 2016/10/04 19:40:39, gab wrote:
> Can we test the new code?
> 
> FYI testing through histogram is possible, e.g.
>
https://cs.chromium.org/chromium/src/chrome/browser/prefs/tracked/pref_hash_b...,
> might require custom setup in a unit test though not sure.

Yes is the short answer. But with significant refactoring. This is intended as a
cheap and dirty experiment for getting numbers to justify the third party module
blocking and estimate its impact. We'll be completely rewriting pretty much all
of the code in here with proper unittests on the way to that goal.

So I'm going to punt on this for the moment. With a promise to do some of that
work in the next CL, which I'll be working on the rest of this week.

[gab, 2016-10-05 13:59:39]

https://codereview.chromium.org/2384413004/diff/40001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2384413004/diff/40001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:316:
DCHECK(cert_info->subject.clear());
Hmmm DCHECK clear()? Does that even compile?

[chrisha, 2016-10-05 14:14:59]

https://codereview.chromium.org/2384413004/diff/40001/chrome/browser/win/enum...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2384413004/diff/40001/chrome/browser/win/enum...
chrome/browser/win/enumerate_modules_model.cc:316:
DCHECK(cert_info->subject.clear());
On 2016/10/05 13:59:39, gab wrote:
> Hmmm DCHECK clear()? Does that even compile?

Nope, and that's why I didn't submit this. Have another CL on my machine that I
didn't get time to upload :)

[chrisha, 2016-10-05 15:45:18]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2016-10-05 15:45:19]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2384413004/#ps60001
(title: "Now even compiles!")

[commit-bot: I haz the power, 2016-10-05 15:46:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 16:21:33]

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

[commit-bot: I haz the power, 2016-10-05 16:21:34]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chrisha, 2016-10-06 13:12:39]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2016-10-06 13:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2384413004/#ps80001
(title: "Fix missing include.")

[commit-bot: I haz the power, 2016-10-06 13:13:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 13:51:21]

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

[commit-bot: I haz the power, 2016-10-06 13:51:23]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chrisha, 2016-10-06 18:33:01]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2016-10-06 18:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2384413004/#ps100001
(title: "Fix initializer lists.")

[commit-bot: I haz the power, 2016-10-06 18:34:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 20:37:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 20:39:42]

Description was changed from

==========
[Win] Count number of distinct certificates covering third party modules.

The real cost in validating signed modules is validating individual
certificates. Since many certificates are provided en masse by catalogs, this
counts catalogs as providing a single certificate.

BUG=617176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[Win] Count number of distinct certificates covering third party modules.

The real cost in validating signed modules is validating individual
certificates. Since many certificates are provided en masse by catalogs, this
counts catalogs as providing a single certificate.

BUG=617176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

[commit-bot: I haz the power, 2016-10-06 20:39:43]

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