assigning to myself on behalf of carlosil@ (still waiting for his chromium account / edit bugs acccess)

To trigger the UI that we want to measure, visit https://expired.badssl.com > click Advanced > Proceed to expired.badssl.com. Then click Not Secure in the omnibox > the “Re-enable warnings” in that dialog is what we’re interested in recording. The question we want to answer is: on what % of pages loaded with certificate errors is that button clicked?

UMA is Chrome’s telemetry system, used for recording metrics and answering questions like this one.

Example of recording a metric like this: https://cs.chromium.org/chromium/src/chrome/browser/ui/page_info/page_info.cc?q=PageInfo.Action&sq=package:chromium&l=328. Adding a histogram requires a corresponding change in tools/metrics/histograms/histograms.xml: search for “Security.PageInfo.Action” in that file for an example.

UI in Chrome generally has a Views version (UI framework used for Windows and Linux), a Mac version, and an Android version. In this case, it looks like all the UI code calls the same cross-platform function (https://cs.chromium.org/chromium/src/chrome/browser/ui/page_info/page_info.cc?type=cs&q=OnRevokeSSLErrorBypassButtonPressed&l=439), so that function would be a good place to record the histogram.

https://cs.chromium.org/chromium/src/chrome/browser/ui/page_info/page_info_unittest.cc?l=734 is an example of how to test that histograms are recorded properly.

We also will need a histogram to record the # of pages that were loaded after bypassing a certificate error; I don't think we have that already, and we'll need it to answer question (1) in the bug description. A good place to record a histogram for that would be in SecurityStateTabHelper::DidFinishNavigation; you can check if there was a bypass certificate error there with:

net::IsCertStatusError(security_info.cert_status) && !net::IsCertStatusMinorError(security_info.cert_status)

It looks like (2) is already being measured by the 'interstitial.ssl.did_user_revoke_decisions' histogram, which at the time of closing the Page Info, records if OnRevokeSSLErrorBypassButtonPressed() was called, is that the case or am I misunderstanding what (2) is supposed to measure?

Further digging revealed that while it does measure how many times the "Re-Enable" button is pressed, it counts them out of the total times PageInfo has ever been opened (even on secure pages), so if you go to a secure site and open the PageInfo 5 times, it logs 5 entries for "didn't click re-enable", even though you haven't seen the button so far (since the site is secure).

I'm going to go ahead and "fix" that so it only logs when the button was in fact visible. If it turns out that histogram was working as intended and we do need one that counts out of total PageInfo views, I'll create a separate histogram that counts only when the button was visible.

Oh I didn't know we had that histogram, nice find! Your proposed fix sounds good to me. One note is that you probably want to deprecate the current interstitial.ssl.did_user_revoke_decisions histogram and create a new one (you can just name the new one "interstitial.ssl.did_user_revoke_decisions2"), that'll make it easier to analyze the data and separate the new from the old. You can search for "deprecated" in histograms.xml to see an example of how to mark a histogram as deprecated.

I added the check so now it only logs to the histogram when the button is visible. However this happens in the OnUIClosing method, which is not called on Android, so the histogram doesn't get metrics from Android users (and thus, the test I added fails on Android).

I did notice that on Android the button is different (you have to hit Details first, and then the link says Stop using an invalid certificate, instead of re-enable warnings, so I'm wondering if we actually want that case to log to the same histogram (in which case I'll move the call to somewhere else) or not (in which case I'll make the Test not run for android).

Hrm, I think it would be useful to have this histogram on Android as well. Maybe we could move the histogram call into OnRevokeSSLErrorBypassButtonPressed? It looks like that gets called on all platforms. Do you think that would work?

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

commit e5dfda7f3c425d24a2519eecc0aa53d52f1b1463
Author: Carlos IL <carlosil@chromium.org>
Date: Thu Sep 21 02:33:05 2017

Change PageInfo to record Re-enable Warnings button UMA when the button is visible

Modified interstitial.ssl.did_user_revoke_decisions histogram so it now
only counts views when the PageInfo contains a button for re-enable
warnings. It is also now called from the destructor instead of from
OnUIClosing() so it works on Android. That histogram now performs what
point (2) of  bug 747667  requires. To keep data separate, histogram was
also renamed interstitial.ssl.did_user_revoke_decisions2 and the old
one was deprecated. Added a test for this histogram that verifies the
new behavior

Bug:  747667 
Change-Id: Icf0ec8cd4460c5636643d45f301495f55b109879
Reviewed-on: https://chromium-review.googlesource.com/669581
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503322}
[modify] https://crrev.com/e5dfda7f3c425d24a2519eecc0aa53d52f1b1463/chrome/browser/ui/page_info/page_info.cc
[modify] https://crrev.com/e5dfda7f3c425d24a2519eecc0aa53d52f1b1463/chrome/browser/ui/page_info/page_info.h
[modify] https://crrev.com/e5dfda7f3c425d24a2519eecc0aa53d52f1b1463/chrome/browser/ui/page_info/page_info_unittest.cc
[modify] https://crrev.com/e5dfda7f3c425d24a2519eecc0aa53d52f1b1463/tools/metrics/histograms/histograms.xml

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

commit 5acee2d0d934f66d3c5f8c13cc7eac7c501b9ea4
Author: Carlos IL <carlosil@chromium.org>
Date: Wed Sep 27 17:36:44 2017

Added histogram that logs visits to sites with certificate errors.

Added a histogram to security_state_tab_helper that logs every time a
site is visited after clicking through a certificate warning
interstitial. Added tests to verify this histogram is being logged
correctly

Bug:  747667 
Change-Id: I86f613965eaba18a17475ea6051415a29de79b2b
Reviewed-on: https://chromium-review.googlesource.com/683226
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504721}
[modify] https://crrev.com/5acee2d0d934f66d3c5f8c13cc7eac7c501b9ea4/chrome/browser/ssl/security_state_tab_helper.cc
[modify] https://crrev.com/5acee2d0d934f66d3c5f8c13cc7eac7c501b9ea4/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
[modify] https://crrev.com/5acee2d0d934f66d3c5f8c13cc7eac7c501b9ea4/tools/metrics/histograms/histograms.xml