Add additional UMA stats for remembering certificate decisions.

chrome/browser/ssl/ssl_blocking_page.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 
 #include "base/build_time.h"
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
@@ skipping 84 matching lines @@
   CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE,
   CAPTIVE_PORTAL_PROBE_COMPLETED,
   CAPTIVE_PORTAL_PROBE_COMPLETED_OVERRIDABLE,
   CAPTIVE_PORTAL_NO_RESPONSE,
   CAPTIVE_PORTAL_NO_RESPONSE_OVERRIDABLE,
   CAPTIVE_PORTAL_DETECTED,
   CAPTIVE_PORTAL_DETECTED_OVERRIDABLE,
   UNUSED_BLOCKING_PAGE_EVENT,
 };
 
+enum SSLIsExpiredAndDecision {

[felt, 2014/08/08 00:17:06]

sort of a weird name for an enum, the "Is" makes it sound like it's a bool.
SSLDesicionAfterExpiration? SSLExpirationAndDecision?

[felt, 2014/08/08 00:17:06]

Add a note warning people that this is a UMA histogram, e.g.,
// Events for UMA. Do not reorder or change!

[jww, 2014/08/08 15:55:45]

Done.

[jww, 2014/08/08 15:55:45]

Done.

+  EXPIRED_AND_PROCEED,
+  EXPIRED_AND_DO_NOT_PROCEED,
+  NOT_EXPIRED_AND_PROCEED,
+  NOT_EXPIRED_AND_DO_NOT_PROCEED,
+  END_OF_SSL_IS_EXPIRED_AND_DECISION,
+};
+
 void RecordSSLBlockingPageEventStats(SSLBlockingPageEvent event) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl",
                             event,
                             UNUSED_BLOCKING_PAGE_EVENT);
 }
 
-void RecordSSLBlockingPageDetailedStats(
-    bool proceed,
-    int cert_error,
-    bool overridable,
-    bool internal,
-    int num_visits,
-    bool captive_portal_detection_enabled,
-    bool captive_portal_probe_completed,
-    bool captive_portal_no_response,
-    bool captive_portal_detected) {
+void RecordSSLIsExpiredPageEventState(bool expired_but_previously_allowed,

[felt, 2014/08/08 00:17:06]

it seems like this will count all non-overridable ones as
NOT_EXPRED_AND_DO_NOT_PROCEED? perhaps you ought to handle non-overridable ones
with a separate entry (or entries), or don't include them in the histogram at
all.

[jww, 2014/08/08 15:55:45]

Good call. I'll split them into
"interstitial.ssl.expired_and_decision.overridable" and
"interstitial.ssl.expired_and_decision.nonoverridable". I'd like to see if
there's a difference between the two.

+                                      bool proceed) {
+  SSLIsExpiredAndDecision event;
+  if (expired_but_previously_allowed && proceed)
+    event = EXPIRED_AND_PROCEED;
+  else if (expired_but_previously_allowed && !proceed)
+    event = EXPIRED_AND_DO_NOT_PROCEED;
+  else if (!expired_but_previously_allowed && proceed)
+    event = NOT_EXPIRED_AND_PROCEED;
+  else
+    event = NOT_EXPIRED_AND_DO_NOT_PROCEED;
+
+  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.is_expired_and_decision",
+                            event,
+                            END_OF_SSL_IS_EXPIRED_AND_DECISION);
+}
+
+void RecordSSLBlockingPageDetailedStats(bool proceed,
+                                        int cert_error,
+                                        bool overridable,
+                                        bool internal,
+                                        int num_visits,
+                                        bool captive_portal_detection_enabled,
+                                        bool captive_portal_probe_completed,
+                                        bool captive_portal_no_response,
+                                        bool captive_portal_detected,
+                                        bool expired_but_previously_allowed) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_type",
       SSLErrorInfo::NetErrorToErrorType(cert_error), SSLErrorInfo::END_OF_ENUM);
+  RecordSSLIsExpiredPageEventState(expired_but_previously_allowed, proceed);
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
   if (captive_portal_detection_enabled)
     RecordSSLBlockingPageEventStats(
         overridable ?
         CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE :
         CAPTIVE_PORTAL_DETECTION_ENABLED);
   if (captive_portal_probe_completed)
     RecordSSLBlockingPageEventStats(
         overridable ?
         CAPTIVE_PORTAL_PROBE_COMPLETED_OVERRIDABLE :
@@ skipping 129 matching lines @@
   options.allow_new_privs = true;
 #endif
   base::LaunchProcess(command, options, NULL);
 #endif
 }
 
 }  // namespace
 
 // Note that we always create a navigation entry with SSL errors.
 // No error happening loading a sub-resource triggers an interstitial so far.
-SSLBlockingPage::SSLBlockingPage(
-    content::WebContents* web_contents,
-    int cert_error,
-    const net::SSLInfo& ssl_info,
-    const GURL& request_url,
-    bool overridable,
-    bool strict_enforcement,
-    const base::Callback<void(bool)>& callback)
+SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents,
+                                 int cert_error,
+                                 const net::SSLInfo& ssl_info,
+                                 const GURL& request_url,
+                                 bool overridable,
+                                 bool strict_enforcement,
+                                 bool expired_but_previously_allowed,
+                                 const base::Callback<void(bool)>& callback)
     : callback_(callback),
       web_contents_(web_contents),
       cert_error_(cert_error),
       ssl_info_(ssl_info),
       request_url_(request_url),
       overridable_(overridable),
       strict_enforcement_(strict_enforcement),
       interstitial_page_(NULL),
       internal_(false),
       num_visits_(-1),
       captive_portal_detection_enabled_(false),
       captive_portal_probe_completed_(false),
       captive_portal_no_response_(false),
-      captive_portal_detected_(false) {
+      captive_portal_detected_(false),
+      expired_but_previously_allowed_(expired_but_previously_allowed) {
   Profile* profile = Profile::FromBrowserContext(
       web_contents->GetBrowserContext());
   // For UMA stats.
   if (net::IsHostnameNonUnique(request_url_.HostNoBrackets()))
     internal_ = true;
   RecordSSLBlockingPageEventStats(SHOW_ALL);
   if (overridable_ && !strict_enforcement_) {
     RecordSSLBlockingPageEventStats(SHOW_OVERRIDABLE);
     if (internal_)
       RecordSSLBlockingPageEventStats(SHOW_INTERNAL_HOSTNAME);
@@ skipping 30 matching lines @@
 SSLBlockingPage::~SSLBlockingPage() {
   if (!callback_.is_null()) {
     RecordSSLBlockingPageDetailedStats(false,
                                        cert_error_,
                                        overridable_ && !strict_enforcement_,
                                        internal_,
                                        num_visits_,
                                        captive_portal_detection_enabled_,
                                        captive_portal_probe_completed_,
                                        captive_portal_no_response_,
-                                       captive_portal_detected_);
+                                       captive_portal_detected_,
+                                       expired_but_previously_allowed_);
     // The page is closed without the user having chosen what to do, default to
     // deny.
     NotifyDenyCertificate();
   }
 }
 
 void SSLBlockingPage::Show() {
   DCHECK(!interstitial_page_);
   interstitial_page_ = InterstitialPage::Create(
       web_contents_, true, request_url_, this);
@@ skipping 161 matching lines @@
 
 void SSLBlockingPage::OnProceed() {
   RecordSSLBlockingPageDetailedStats(true,
                                      cert_error_,
                                      overridable_ && !strict_enforcement_,
                                      internal_,
                                      num_visits_,
                                      captive_portal_detection_enabled_,
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     captive_portal_detected_,
+                                     expired_but_previously_allowed_);
   // Accepting the certificate resumes the loading of the page.
   NotifyAllowCertificate();
 }
 
 void SSLBlockingPage::OnDontProceed() {
   RecordSSLBlockingPageDetailedStats(false,
                                      cert_error_,
                                      overridable_ && !strict_enforcement_,
                                      internal_,
                                      num_visits_,
                                      captive_portal_detection_enabled_,
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     captive_portal_detected_,
+                                     expired_but_previously_allowed_);
   NotifyDenyCertificate();
 }
 
 void SSLBlockingPage::NotifyDenyCertificate() {
   // It's possible that callback_ may not exist if the user clicks "Proceed"
   // followed by pressing the back button before the interstitial is hidden.
   // In that case the certificate will still be treated as allowed.
   if (callback_.is_null())
     return;
 
@@ skipping 55 matching lines @@
     // sure we don't clear the captive portal flag, since the interstitial was
     // potentially caused by the captive portal.
     captive_portal_detected_ = captive_portal_detected_ ||
         (results->result == captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL);
     // Also keep track of non-HTTP portals and error cases.
     captive_portal_no_response_ = captive_portal_no_response_ ||
         (results->result == captive_portal::RESULT_NO_RESPONSE);
   }
 #endif
 }