Adding the Finch code for the certificate error reporter experiment

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/bind.h"
 #include "base/bind_helpers.h"
 #include "base/build_time.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/process/launch.h"
+#include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/interstitials/security_interstitial_metrics_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/browser/ssl/ssl_error_classification.h"
 #include "chrome/browser/ssl/ssl_error_info.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/google/core/browser/google_util.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cert_store.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/interstitial_page_delegate.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ skipping 23 matching lines @@
 #include "chrome/common/url_constants.h"
 #endif
 
 using base::ASCIIToUTF16;
 using base::TimeTicks;
 using content::InterstitialPage;
 using content::InterstitialPageDelegate;
 using content::NavigationController;
 using content::NavigationEntry;
 
+// Constants for the HTTPSErrorReporter Finch experiment
+const char kHTTPSErrorReporterFinchExperimentName[] = "ReportCertificateErrors";
+const char kHTTPSErrorReporterFinchGroupShowPossiblySend[] =
+    "ShowAndPossiblySend";
+const char kHTTPSErrorReporterFinchParamName[] = "possibly_send";
+
 namespace {
 
 // URL for help page.
 const char kHelpURL[] = "https://support.google.com/chrome/answer/4454607";
 
 // Constants for the Experience Sampling instrumentation.
 const char kEventNameBase[] = "ssl_interstitial_";
 const char kEventNotOverridable[] = "notoverridable_";
 const char kEventOverridable[] = "overridable_";
 
 // Events for UMA. Do not reorder or change!
 enum SSLExpirationAndDecision {
   EXPIRED_AND_PROCEED,
   EXPIRED_AND_DO_NOT_PROCEED,
   NOT_EXPIRED_AND_PROCEED,
   NOT_EXPIRED_AND_DO_NOT_PROCEED,
   END_OF_SSL_EXPIRATION_AND_DECISION,
 };
 
 // Rappor prefix
 const char kSSLRapporPrefix[] = "ssl";
 
+// Check whether to show the certificate reporter checkbox
+bool ShouldShowCertificateReporterCheckbox(bool in_incognito) {
+  // Check Finch parameters
+  return base::FieldTrialList::FindFullName(
+             kHTTPSErrorReporterFinchExperimentName)

[felt, 2015/04/03 14:32:28]

nit: this indentation is very odd, is this what git cl format did?

[fahl, 2015/04/03 17:39:31]

Yep, I usually run git cl format before git cl upload

+                 .compare(kHTTPSErrorReporterFinchGroupShowPossiblySend) == 0 &&

[felt, 2015/04/03 14:32:29]

nit: you normally see this as FindFullName(name) == kGroup, instead of using
.compare; it's a little cleaner IMO

[fahl, 2015/04/03 17:39:31]

Done.

+         !in_incognito;
+}
+
+// Check whether to report certificate verification errors to Google
+bool ShouldReportCertificateErrors(bool in_incognito) {
+  DCHECK(ShouldShowCertificateReporterCheckbox(in_incognito));
+  // Check Finch parameters

[felt, 2015/04/03 14:32:29]

either leave off this comment (as it's obvious), or add more detail to it to
include something non-obvious; the latter might be worthwhile, as this is an
unusual use of finch params

[fahl, 2015/04/03 17:39:31]

Done.

+  const std::string param =
+      variations::GetVariationParamValue(kHTTPSErrorReporterFinchExperimentName,
+                                         kHTTPSErrorReporterFinchParamName);
+  if (param.compare("") != 0) {

[felt, 2015/04/03 14:32:29]

this is an awfully weird check too. if (!param.empty()) perhaps?

[fahl, 2015/04/03 17:39:31]

Done.

+    double possiblySend;

[felt, 2015/04/03 14:32:28]

nit: would "sendingThreshold" be a more descriptive variable name?

[fahl, 2015/04/03 17:39:31]

Done.

+    if (base::StringToDouble(param, &possiblySend))

[felt, 2015/04/03 14:32:29]

you'll need { } for the outermost one because its inner contents are more than 1
line long

[fahl, 2015/04/03 17:39:31]

Done.

+      if (possiblySend >= 0.0 && possiblySend <= 1.0)
+        return base::RandDouble() <= possiblySend;
+  }
+  return false;
+}
+
 void RecordSSLExpirationPageEventState(bool expired_but_previously_allowed,
                                        bool proceed,
                                        bool overridable) {
   SSLExpirationAndDecision 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;
@@ skipping 336 matching lines @@
       &encoded_chain);
   load_time_data->SetString(
       "pem", JoinString(encoded_chain, std::string()));
 
   PopulateExtendedReportingOption(load_time_data);
 }
 
 void SSLBlockingPage::PopulateExtendedReportingOption(
     base::DictionaryValue* load_time_data) {
   // Only show the checkbox if not off-the-record and if the
   // command-line option is set.

[felt, 2015/04/03 14:32:29]

does this comment need to be updated?

[fahl, 2015/04/03 17:39:31]

Done.

-  const bool show = !web_contents()->GetBrowserContext()->IsOffTheRecord() &&
-                    base::CommandLine::ForCurrentProcess()->HasSwitch(
-                        switches::kEnableInvalidCertCollection);
+  const bool show = ShouldShowCertificateReporterCheckbox(
+      web_contents()->GetBrowserContext()->IsOffTheRecord());
 
   load_time_data->SetBoolean(interstitials::kDisplayCheckBox, show);
   if (!show)
     return;
 
   load_time_data->SetBoolean(
       interstitials::kBoxChecked,
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled));
 
   const std::string privacy_link = base::StringPrintf(
@@ skipping 165 matching lines @@
   else
     event_name.append(kEventNotOverridable);
   event_name.append(net::ErrorToString(cert_error_));
   return event_name;
 }
 
 void SSLBlockingPage::FinishCertCollection() {
   base::ScopedClosureRunner scoped_callback(
       certificate_report_callback_for_testing_);
 
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableInvalidCertCollection) ||
-      web_contents()->GetBrowserContext()->IsOffTheRecord()) {
+  if (!ShouldShowCertificateReporterCheckbox(
+          web_contents()->GetBrowserContext()->IsOffTheRecord())) {
     return;
   }
 
   const bool enabled =

[felt, 2015/04/03 14:32:29]

should this pref check be moved into ShouldShowCertificateReporterCheckbox?

[fahl, 2015/04/03 17:39:31]

Currently, IsPrefEnabled is a protected member of SecurityInterstitialPage. We
either have to make ShouldShowCertificateReporterCheckbox a member of
SSLBlockingPage or leave the check as it is. What do you think?

[estark, 2015/04/03 17:44:36]

I don't think there's any problem with making
ShouldShowCertificateReporterCheckbox a member of SSLBlockingPage, but  I don't
think moving this check into that function is correct. The call on line 493
should return true even if the preference is false.

[felt, 2015/04/03 22:38:09]

you're right emily, nevermind sascha

       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
 
   if (!enabled)
     return;
 
   metrics_helper()->RecordUserInteraction(
       SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
 
-  if (certificate_report_callback_for_testing_.is_null())
-    scoped_callback.Reset(base::Bind(&base::DoNothing));
-
-  safe_browsing_ui_manager_->ReportInvalidCertificateChain(
-      request_url().host(), ssl_info_, scoped_callback.Release());
+  if (ShouldReportCertificateErrors(
+          web_contents()->GetBrowserContext()->IsOffTheRecord())) {
+    if (certificate_report_callback_for_testing_.is_null())
+      scoped_callback.Reset(base::Bind(&base::DoNothing));
+    safe_browsing_ui_manager_->ReportInvalidCertificateChain(
+        request_url().host(), ssl_info_, scoped_callback.Release());
+  }
 }
 
 // static
 bool SSLBlockingPage::IsOptionsOverridable(int options_mask) {
   return (options_mask & SSLBlockingPage::OVERRIDABLE) &&
          !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT);
 }