Record time to navigation/tab-closed after HTTP-bad warning

chrome/browser/ssl/chrome_security_state_model_client.cc

Index: chrome/browser/ssl/chrome_security_state_model_client.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client.cc b/chrome/browser/ssl/chrome_security_state_model_client.cc
index a5072e614f8558f91977d838cb5506b2e161eb7b..4ce2a90ad4aff39cf94f9346dfb9f842e3b80aa2 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client.cc
@@ -11,6 +11,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/policy/policy_cert_service.h"
@@ -366,6 +367,10 @@ void ChromeSecurityStateModelClient::VisibleSecurityStateChanged() {
     return;
   }
 
+  if (time_of_http_warning_on_current_navigation_.is_null()) {
+    time_of_http_warning_on_current_navigation_ = base::Time::Now();

[elawrence, 2016/11/15 16:29:52]

Do we really want to clear this? If a warning flashed in and then disappeared
(e.g. on page load) but then later reappeared due to user-action, it seems like
the more recent warning would be the more interesting one?

[estark, 2016/11/16 05:16:13]

First of all, I remember being very convinced that this null check on line 370
is the right thing to do, but I don't remember why I thought that. :-/ The way
the code is right now, whenever |logged_http_warning| is false,
|time_of_http_warning| is null, which means line 370 should really be a
DCHECK(time_of_http_warning_on_current_navigation.is_null()).

Anyway, to your actual point... to do what you're suggesting (recording the time
since the most recent HTTP-bad warning was shown), I think we'd have to keep
some extra state around, like the values of |displayed_password_field_on_http|
and |displayed_credit_card_field_on_http| from the most recent time
VisibleSecurityStateChanged was called, to detect when the warning is appearing.
Personally I'm having a hard enough time reasoning about when to set/clear the
time without having even more state to think about, but it's probably doable if
you feel strongly about it.

+  }
+
   std::string warning;
   bool warning_is_user_visible = false;
   switch (security_info.security_level) {
@@ -403,6 +408,23 @@ void ChromeSecurityStateModelClient::VisibleSecurityStateChanged() {
   }
 }
 
+void ChromeSecurityStateModelClient::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage()) {
+    if (time_of_http_warning_on_current_navigation_.is_null()) {
+      return;

[elawrence, 2016/11/15 16:29:52]

Is 

if (time_of_http_warning_on_current_navigation_.is_null() ||
    !navigation_handle->IsInMainFrame() ||
    navigation_handle->IsSamePage())
      return;

more readable?

[estark, 2016/11/16 05:16:13]

Done.

+    }
+    // Record the time delta between when an HTTP warning was shown and

[elawrence, 2016/11/15 16:29:52]

I love comments. I worry that explaining what's happening here is not needed,
but explaining WHY is. So maybe:

// Record how quickly a user leaves a site after seeing a HTTPBad warning

?

[estark, 2016/11/16 05:16:13]

Done.

+    // when a navigation began. A navigation here only counts if it is a
+    // main-frame, not-same-page navigation, since it aims to measure
+    // how quickly a user leaves a site after seeing the HTTP warning.
+    UMA_HISTOGRAM_LONG_TIMES(
+        "Security.HTTPBad.NavigationStartedAfterUserWarnedAboutSensitiveInput",
+        base::Time::Now() - time_of_http_warning_on_current_navigation_);
+    time_of_http_warning_on_current_navigation_ = base::Time();

[elawrence, 2016/11/15 16:29:52]

I don't understand why we reset this here?

[estark, 2016/11/16 05:16:13]

I find it easiest to reason about these histograms if we say that we record at
most once per main-frame navigation. So to implement that, I'm setting the time
at most once per navigation, recording the histogram only if the time is not
null, and setting it to null after recording so that a time histogram doesn't
get recorded again.

I added a comment to try to explain this, lmk if it makes sense.

+  }
+}
+
 void ChromeSecurityStateModelClient::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage()) {
@@ -412,6 +434,20 @@ void ChromeSecurityStateModelClient::DidFinishNavigation(
   }
 }
 
+void ChromeSecurityStateModelClient::WebContentsDestroyed() {

[elawrence, 2016/11/15 16:29:53]

This event only fires for the top-level frame, right?

[estark, 2016/11/16 05:16:13]

Yeah, a WebContents usually is 1:1 with a tab.

+  if (time_of_http_warning_on_current_navigation_.is_null()) {
+    return;
+  }
+  // Record the time delta between when an HTTP warning was shown and
+  // when the WebContents was destroyed. This histogram will only be
+  // recorded if the WebContents is destroyed before another
+  // navigation begins.
+  UMA_HISTOGRAM_LONG_TIMES(

[elawrence, 2016/11/15 16:29:52]

"This histogram will only be recorded if the WebContents is destroyed before
another navigation begins."

How is that implemented? Why wouldn't this get logged as a part of teardown of
the page after a navigation starts?

[estark, 2016/11/16 05:16:13]

If a navigation has begun before the WebContents is destroyed, then
|time_of_http_warning| will be null (having been cleared on line 424) and we'll
return on line 439.

+      "Security.HTTPBad.WebContentsDestroyedAfterUserWarnedAboutSensitiveInput",
+      base::Time::Now() - time_of_http_warning_on_current_navigation_);
+  time_of_http_warning_on_current_navigation_ = base::Time();

[elawrence, 2016/11/15 16:29:52]

I don't understand why we reset this here?

[estark, 2016/11/16 05:16:13]

I guess this is unnecessary, removed.

+}
+
 bool ChromeSecurityStateModelClient::UsedPolicyInstalledCertificate() {
 #if defined(OS_CHROMEOS)
   policy::PolicyCertService* service =