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

chrome/browser/ssl/chrome_security_state_model_client.cc

 // Copyright 2015 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/chrome_security_state_model_client.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #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"
 #include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/navigation_entry.h"
@@ skipping 335 matching lines @@
   if (logged_http_warning_on_current_navigation_)
     return;
 
   security_state::SecurityStateModel::SecurityInfo security_info;
   GetSecurityInfo(&security_info);
   if (!security_info.displayed_password_field_on_http &&
       !security_info.displayed_credit_card_field_on_http) {
     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) {
     case security_state::SecurityStateModel::HTTP_SHOW_WARNING:
       warning =
           "This page includes a password or credit card input in a non-secure "
           "context. A warning has been added to the URL bar. For more "
           "information, see https://goo.gl/zmWq3m.";
       warning_is_user_visible = true;
       break;
@@ skipping 17 matching lines @@
         "Security.HTTPBad.UserWarnedAboutSensitiveInput.CreditCard",
         warning_is_user_visible);
   }
   if (security_info.displayed_password_field_on_http) {
     UMA_HISTOGRAM_BOOLEAN(
         "Security.HTTPBad.UserWarnedAboutSensitiveInput.Password",
         warning_is_user_visible);
   }
 }
 
+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()) {
     // Only reset the console message flag for main-frame navigations,
     // and not for same-page navigations like reference fragments and pushState.
     logged_http_warning_on_current_navigation_ = false;
   }
 }
 
+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 =
       policy::PolicyCertServiceFactory::GetForProfile(
           Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
   if (service && service->UsedPolicyCertificates())
     return true;
 #endif
   return false;
 }
@@ skipping 43 matching lines @@
       !!(ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
   state->displayed_password_field_on_http =
       !!(ssl.content_status &
          content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP);
   state->displayed_credit_card_field_on_http =
       !!(ssl.content_status &
          content::SSLStatus::DISPLAYED_CREDIT_CARD_FIELD_ON_HTTP);
 
   CheckSafeBrowsingStatus(entry, web_contents_, state);
 }