David, Emily: Am I missing something? My main concern would a perf-hit from the repeated hashing. I hadn't realized it wasn't serialized, but in retrospect it makes sense.


Other fields worth noting:
- is_issued_by_known_root (exposed to DevTools UI AIUI)
- client_cert_sent, channel_id_sent, token_binding_negotiated, token_binding_key_param, handshake_type (all make sense as they're per-connection flags)
- pinning_failure_log (expected to be omitted)
- ct_compliance_details_available, ct_cert_policy_compliance, ct_policy_compliance_required, ocsp_result (affects Emily's histograms)

We do want to avoid doing too much work when deserializing HttpResponseInfo, but hopefully taking a few hashes is pretty cheap? I'm mostly thinking how parsing less of the certificate and avoiding OpenSSL's really slow X.509 library improved metrics for resources loaded entirely from disk.

I actually think we should be serializing less on disk cache, perhaps not even serializing the certificate. The things surfaced from disk are misleading since they don't necessarily reflect the current state of the server. Diagnostics-type things like UMA or DevTools could decline to record a data point or show a "(loaded from cached, reload to hit the server)" message, which is a bit more accurate. If a resource has max-age=9999 but was fetched under bad ciphers that the site no longer uses, surfacing that in DevTools seems odd. (This was the motivation for making revalidations pick up the new settings way back, but that still doesn't cover cases when we don't hit the server at all.)

But this is a much more involved change. If we're going to be doing something that needs the hashes on cached resources, recreating them seems reasonable. Then again, as you note, there are a ton of other random fields so skipping the data points might be simpler?

Yes, I think that's a much more involved change - and one which would really need to go through a design doc write-up and UI review. Wan-Teh had a similar discussion 7 years ago (when I added the chain serializing code in r82214 / r82618 ). At the time, UI asymmetry was undesired - the behaviour before then was similar to what you mention.

To the extent we want to measure per-request impact of potential changes - such as ciphersuites or trust changes - then I agree, ignoring the disk cache makes sense, and not even serializing, as such loads would not be affected. That said, we presently have an asymmetry between ciphersuites (since r89490) and SCTs (since r237849).

Any uses of the ct_* fields are supposed to be guarded by checking |ct_compliance_details_available| and are expected to gracefully handle those details not being available, as in a load from disk cache. (As a sidenote, I have a CL that I'm going to mail out soon that removes the ct_compliance_details_available field in favor of representing it with an enum value, since I think it's too easy for consumers to forget to check the separate field.)

My goal for DevTools is to always handle SSL information not being available, though I'm not sure how well we do at that in practice. I'd like us to prompt to hard-reload when stuff isn't available, as suggested in Comment #2. I might be forgetting something, but I'm pretty sure is_issued_by_known_root is only exposed to DevTools indirectly via |pkp_bypassed| -- so DevTools would have a false negative in the case of resources loaded from disk cache that were originally loaded with PKP  bypassed. I think that's okay though not ideal.

There is interstitial code that uses public_key_hashes, but I don't think it's affected by this case, because if we hit a cert error we must be hitting the server, not the disk cache.

I went ahead and marked this WontFix. As pointed out offline (and touched in in Comment #2), for the intended for purposes of these metrics, knowing what might historically be in the disk cache and affected by future CA policies is arguably less relevant to knowing what network loads would be affected by those policies (as it relates to  Issue 787635 )

As it relates to the other uses, Emily covered it in Comment #4, so I'll mark this WontFix.

David - it's unclear from Comment #2 whether we should *stop* serializing SCTs into the disk cache. I could buy that - and possibly not even serializing the cert. Do you want to file bugs for that if you think it's useful?

Comment #2 is more my "if I could magically change everything else, but probably can't" sort of thing. Not serializing certs I think works for most things (DevTools, UMA), but something like how we used to degrade the lock icon based on the certificate would probably be sad.

(Arguably the cached case, in the case of max-age=99999, doesn't work great either though. So I don't know what the answer should be.)

I do think we could get away with not serializing SCTs, we have  issue 647947  for that. I *think* we can just go ahead and do that whenever, the only UI surface that would be affected would be devtools.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d756c2dfa56141b3970cd836fb0a39d6872ca0e6

commit d756c2dfa56141b3970cd836fb0a39d6872ca0e6
Author: Ryan Sleevi <rsleevi@chromium.org>
Date: Fri Dec 01 02:07:48 2017

Update the root store histograms for Windows 2017-11-21 update

Also distinguish between no hashes available (e.g. loaded from disk
cache) and no publicly trusted hashes (e.g. private CA) in the
.Request metric. Given that this only affected Canary metrics for
a week, the metric is intentionally not renamed.

BUG= 787635 , 788563 

Change-Id: I7ff97cd3ecd20b308e6cc85df5c549608251dcbe
Reviewed-on: https://chromium-review.googlesource.com/802214
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520788}
[modify] https://crrev.com/d756c2dfa56141b3970cd836fb0a39d6872ca0e6/net/cert/root_cert_list_generated.h
[modify] https://crrev.com/d756c2dfa56141b3970cd836fb0a39d6872ca0e6/net/data/ssl/root_stores/root_stores.json
[modify] https://crrev.com/d756c2dfa56141b3970cd836fb0a39d6872ca0e6/net/url_request/url_request_http_job.cc
[modify] https://crrev.com/d756c2dfa56141b3970cd836fb0a39d6872ca0e6/net/url_request/url_request_http_job_unittest.cc
[modify] https://crrev.com/d756c2dfa56141b3970cd836fb0a39d6872ca0e6/tools/metrics/histograms/enums.xml