Clean up experiment logic in ChromeSSLHostStateDelegate.

This deletes code that references
kRevertCertificateErrorDecisionsFieldTrialName, the experiment for
clearing certificate exception decisions at the end of a session. This
includes deleting all code for FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END
since we decided to launch REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA.

Bug: 840045
Change-Id: I6ea2606a64c907e52215e20dc820b481ef28c114
Reviewed-on: https://chromium-review.googlesource.com/1142567
Commit-Queue: Livvie Lin <livvielin@chromium.org>
Reviewed-by: Adrienne Porter Felt <felt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576374}

chrome/browser/ssl/chrome_ssl_host_state_delegate.cc

@@ -14,7 +14,6 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
-#include "base/guid.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_params.h"
@@ -70,17 +69,11 @@ constexpr int kRecurrentInterstitialDefaultResetTime =
 // overidden by a field trial group.  See https://crbug.com/487270.
 const uint64_t kDeltaDefaultExpirationInSeconds = UINT64_C(604800);
 
-// Field trial information
-const char kRevertCertificateErrorDecisionsFieldTrialName[] =
-    "RevertCertificateErrorDecisions";
-const char kForgetAtSessionEndGroup[] = "Session";
-
 // Keys for the per-site error + certificate finger to judgment content
 // settings map.
 const char kSSLCertDecisionCertErrorMapKey[] = "cert_exceptions_map";
 const char kSSLCertDecisionExpirationTimeKey[] = "decision_expiration_time";
 const char kSSLCertDecisionVersionKey[] = "version";
-const char kSSLCertDecisionGUIDKey[] = "guid";
 
 const int kDefaultSSLCertDecisionVersion = 1;
 
@@ -181,17 +174,6 @@ GURL GetSecureGURLForHost(const std::string& host) {
   return GURL(url);
 }
 
-// By default, certificate exception decisions are remembered for one week.
-// However, there is a field trial group for the "old" style of certificate
-// decision memory that expires decisions at session end. ExpireAtSessionEnd()
-// returns |true| if and only if the user is in that field trial group.
-bool ExpireAtSessionEnd() {
-  std::string group_name = base::FieldTrialList::FindFullName(
-      kRevertCertificateErrorDecisionsFieldTrialName);
-  return !group_name.empty() &&
-         group_name.compare(kForgetAtSessionEndGroup) == 0;
-}
-
 std::string GetKey(const net::X509Certificate& cert, int error) {
   // Since a security decision will be made based on the fingerprint, Chrome
   // should use the SHA-256 fingerprint for the certificate.
@@ -264,22 +246,10 @@ bool HostFilterToPatternFilter(
 const base::Feature kRecurrentInterstitialFeature{
     "RecurrentInterstitialFeature", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// If |should_remember_ssl_decisions_| is
-// FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END, that means that all invalid
-// certificate proceed decisions should be forgotten when the session ends. To
-// simulate that, Chrome keeps track of a guid to represent the current browser
-// session and stores it in decision entries. See the comment for
-// |current_expiration_guid_| for more information.
 ChromeSSLHostStateDelegate::ChromeSSLHostStateDelegate(Profile* profile)
     : clock_(new base::DefaultClock()),
-      profile_(profile),
-      current_expiration_guid_(base::GenerateGUID()) {
+      profile_(profile) {
   MigrateOldSettings(HostContentSettingsMapFactory::GetForProfile(profile));
-  if (ExpireAtSessionEnd())
-    should_remember_ssl_decisions_ =
-        FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END;
-  else
-    should_remember_ssl_decisions_ = REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA;
 }
 
 ChromeSSLHostStateDelegate::~ChromeSSLHostStateDelegate() {
@@ -626,9 +596,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
   // NULL.
   // - Expired and |create_entries| is CREATE_DICTIONARY_ENTRIES, update the
   // expiration time.
-  if (should_remember_ssl_decisions_ !=
-          FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END &&
-      decision_expiration.ToInternalValue() <= now.ToInternalValue()) {
+  if (decision_expiration.ToInternalValue() <= now.ToInternalValue()) {
     *expired_previous_decision = true;
 
     if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES)
@@ -642,20 +610,8 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
     // better to store the value as a string.
     dict->SetString(kSSLCertDecisionExpirationTimeKey,
                     base::Int64ToString(expiration_time.ToInternalValue()));
-  } else if (should_remember_ssl_decisions_ ==
-             FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END) {
-    if (dict->HasKey(kSSLCertDecisionGUIDKey)) {
-      std::string old_expiration_guid;
-      success = dict->GetString(kSSLCertDecisionGUIDKey, &old_expiration_guid);
-      if (old_expiration_guid.compare(current_expiration_guid_) != 0) {
-        *expired_previous_decision = true;
-        expired = true;
-      }
-    }
   }
 
-  dict->SetString(kSSLCertDecisionGUIDKey, current_expiration_guid_);
-
   // Extract the map of certificate fingerprints to errors from the setting.
   base::DictionaryValue* cert_error_dict = NULL;  // Will be owned by dict
   if (expired ||

chrome/browser/ssl/chrome_ssl_host_state_delegate.h

@@ -92,16 +92,6 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
     DO_NOT_CREATE_DICTIONARY_ENTRIES
   };
 
-  // Specifies whether user SSL error decisions should be forgetten at the end
-  // of this current session (the old style of remembering decisions), or
-  // whether they should be remembered across session restarts for a specified
-  // length of time, deteremined by
-  // |default_ssl_cert_decision_expiration_delta_|.
-  enum RememberSSLExceptionDecisionsDisposition {
-    FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END,
-    REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA
-  };
-
   // Returns a dictionary of certificate fingerprints and errors that have been
   // allowed as exceptions by the user.
   //
@@ -122,7 +112,6 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
       bool* expired_previous_decision);
 
   std::unique_ptr<base::Clock> clock_;
-  RememberSSLExceptionDecisionsDisposition should_remember_ssl_decisions_;
   Profile* profile_;
 
   // A BrokenHostEntry is a pair of (host, child_id) that indicates the host
@@ -138,29 +127,6 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
   // the specific process.
   std::set<BrokenHostEntry> ran_content_with_cert_errors_hosts_;
 
-  // This is a GUID to mark this unique session. Whenever a certificate decision
-  // expiration is set, the GUID is saved as well so Chrome can tell if it was
-  // last set during the current session. This is used by the
-  // FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END experimental group to
-  // determine if the expired_previous_decision bit should be set on queries.
-  //
-  // Why not just iterate over the set of current extensions and mark them all
-  // as expired when the session starts, rather than storing a GUID for the
-  // current session? Glad you asked! Unfortunately, content settings does not
-  // currently support iterating over all current *compound* content setting
-  // values (iteration only works for simple content settings). While this could
-  // be added, it would be a fair amount of work for what amounts to a temporary
-  // measurement problem, so it's not worth the complexity.
-  //
-  // TODO(jww): This is only used by the default and disable groups of the
-  // certificate memory decisions experiment to tell if a decision has expired
-  // since the last session. Since this is only used for UMA purposes, this
-  // should be removed after the experiment has finished, and a call to Clear()
-  // should be added to the constructor and destructor for members of the
-  // FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END groups. See
-  // https://crbug.com/418631 for more details.
-  const std::string current_expiration_guid_;
-
   // Tracks how many times an error page has been shown for a given error, up
   // to a certain threshold value.
   std::map<int /* error code */, int /* count */> recurrent_errors_;

chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc

@@ -48,12 +48,6 @@ scoped_refptr<net::X509Certificate> GetOkCert() {
   return net::ImportCertFromFile(net::GetTestCertsDirectory(), kOkCertFile);
 }
 
-// Helper function for setting Finch options
-void SetFinchConfig(base::CommandLine* command_line, const std::string& group) {
-  command_line->AppendSwitchASCII("--force-fieldtrials",
-                                  "RevertCertificateErrorDecisions/" + group);
-}
-
 bool CStrStringMatcher(const char* a, const std::string& b) {
   return a == b;
 }
@@ -491,83 +485,9 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest,
       net::ERR_CERTIFICATE_TRANSPARENCY_REQUIRED));
 }
 
-class ForgetAtSessionEndSSLHostStateDelegateTest
-    : public ChromeSSLHostStateDelegateTest {
- protected:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    ChromeSSLHostStateDelegateTest::SetUpCommandLine(command_line);
-    SetFinchConfig(command_line, "Session");
-  }
-};
-
-// QueryPolicyExpired unit tests to make sure that if a certificate decision has
-// expired, the return value from QueryPolicy returns the correct vaule.
-IN_PROC_BROWSER_TEST_F(ForgetAtSessionEndSSLHostStateDelegateTest,
-                       PRE_QueryPolicyExpired) {
-  scoped_refptr<net::X509Certificate> cert = GetOkCert();
-  content::WebContents* tab =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
-  content::SSLHostStateDelegate* state = profile->GetSSLHostStateDelegate();
-  bool expired_previous_decision;
-
-  // The certificate has never been seen before, so it should be UNKNOWN and
-  // should also indicate that it hasn't expired.
-  EXPECT_EQ(
-      content::SSLHostStateDelegate::DENIED,
-      state->QueryPolicy(kWWWGoogleHost, *cert, net::ERR_CERT_DATE_INVALID,
-                         &expired_previous_decision));
-  EXPECT_FALSE(expired_previous_decision);
-
-  // After allowing the certificate, a query should say that it is allowed and
-  // also specify that it hasn't expired.
-  state->AllowCert(kWWWGoogleHost, *cert, net::ERR_CERT_DATE_INVALID);
-  EXPECT_EQ(
-      content::SSLHostStateDelegate::ALLOWED,
-      state->QueryPolicy(kWWWGoogleHost, *cert, net::ERR_CERT_DATE_INVALID,
-                         &expired_previous_decision));
-  EXPECT_FALSE(expired_previous_decision);
-}
-
-// Since this is being checked on a browser instance that expires security
-// decisions after restart, the test needs to  wait until after a restart to
-// verify that the expiration state is correct.
-IN_PROC_BROWSER_TEST_F(ForgetAtSessionEndSSLHostStateDelegateTest,
-                       QueryPolicyExpired) {
-  scoped_refptr<net::X509Certificate> cert = GetOkCert();
-  content::WebContents* tab =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
-  content::SSLHostStateDelegate* state = profile->GetSSLHostStateDelegate();
-  bool expired_previous_decision;
-
-  // The browser content has restart thus expiring the user decision made above,
-  // so it should indicate that the certificate and error are DENIED but also
-  // that they expired since the last query.
-  EXPECT_EQ(
-      content::SSLHostStateDelegate::DENIED,
-      state->QueryPolicy(kWWWGoogleHost, *cert, net::ERR_CERT_DATE_INVALID,
-                         &expired_previous_decision));
-  EXPECT_TRUE(expired_previous_decision);
-
-  // However, with a new query, it should indicate that no new expiration has
-  // occurred.
-  EXPECT_EQ(
-      content::SSLHostStateDelegate::DENIED,
-      state->QueryPolicy(kWWWGoogleHost, *cert, net::ERR_CERT_DATE_INVALID,
-                         &expired_previous_decision));
-  EXPECT_FALSE(expired_previous_decision);
-}
-
 // Tests the basic behavior of cert memory in incognito.
 class IncognitoSSLHostStateDelegateTest
-    : public ChromeSSLHostStateDelegateTest {
- protected:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    ChromeSSLHostStateDelegateTest::SetUpCommandLine(command_line);
-    SetFinchConfig(command_line, "OneWeek");
-  }
-};
+    : public ChromeSSLHostStateDelegateTest {};
 
 IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, PRE_AfterRestart) {
   scoped_refptr<net::X509Certificate> cert = GetOkCert();