Re-enable PrefHashBrowserTest with a few more mitigations and cleanups.

chrome/browser/prefs/tracked/pref_hash_browsertest.cc

 // Copyright 2014 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 <memory>
 #include <string>
 #include <utility>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
@@ skipping 26 matching lines @@
 #include "components/search_engines/template_url_data.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/common/extension.h"
 #include "services/preferences/public/cpp/tracked/tracked_preference_histogram_names.h"
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/chromeos_switches.h"
 #endif
 
 #if defined(OS_WIN)
-#include "base/test/test_reg_util_win.h"
+#include "base/win/registry.h"
+#include "chrome/install_static/install_util.h"
 #endif
 
 namespace {
 
 // Extension ID of chrome/test/data/extensions/good.crx
 const char kGoodCrxId[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
 
 // Explicit expectations from the caller of GetTrackedPrefHistogramCount(). This
 // enables detailed reporting of the culprit on failure.
 enum AllowedBuckets {
   // Allow no samples in any buckets.
   ALLOW_NONE = -1,
   // Any integer between BEGIN_ALLOW_SINGLE_BUCKET and END_ALLOW_SINGLE_BUCKET
   // indicates that only this specific bucket is allowed to have a sample.
   BEGIN_ALLOW_SINGLE_BUCKET = 0,
   END_ALLOW_SINGLE_BUCKET = 100,
   // Allow any buckets (no extra verifications performed).
   ALLOW_ANY
 };
 
 #if defined(OS_WIN)
 base::string16 GetRegistryPathForTestProfile() {
+  // Cleanup follow-up to http://crbug.com/721245 for the previous location of
+  // this test key which had similar problems (to a lesser extent). It's
+  // redundant but harmless to have multiple callers hit this on the same
+  // machine. TODO(gab): remove this mid-june 2017.
+  base::win::RegKey key;
+  if (key.Open(HKEY_CURRENT_USER, L"SOFTWARE\\Chromium\\PrefHashBrowserTest",
+               KEY_SET_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
+    LONG result = key.DeleteKey(L"");
+    EXPECT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
+  }
+
   base::FilePath profile_dir;
   EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
-  return L"SOFTWARE\\Chromium\\PrefHashBrowserTest\\" +
+
+  // Use a location under the real PreferenceMACs path so that the backup
+  // cleanup logic in ChromeTestLauncherDelegate::PreSharding() for interrupted
+  // tests covers this test key as well.
+  return install_static::GetRegistryPath() +
+         L"\\PreferenceMACs\\PrefHashBrowserTest\\" +
          profile_dir.BaseName().value();
 }
 #endif
 
 // Returns the number of times |histogram_name| was reported so far; adding the
 // results of the first 100 buckets (there are only ~19 reporting IDs as of this
 // writing; varies depending on the platform). |allowed_buckets| hints at extra
 // requirements verified in this method (see AllowedBuckets for details).
 int GetTrackedPrefHistogramCount(const char* histogram_name,
                                  const char* histogram_suffix,
@@ skipping 207 matching lines @@
         LONG result = key.DeleteKey(L"");
         ASSERT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
       }
     }
 #endif
   }
 
   void TearDown() override {
 #if defined(OS_WIN)
     // When done, delete the Registry key to avoid polluting the registry.
-    // TODO(proberge): it would be nice to delete keys from interrupted tests
-    // as well.
     if (!IsPRETest()) {
       base::string16 registry_key = GetRegistryPathForTestProfile();
       base::win::RegKey key;
       if (key.Open(HKEY_CURRENT_USER, registry_key.c_str(),
                    KEY_SET_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
         LONG result = key.DeleteKey(L"");
         ASSERT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
       }
     }
 #endif
@@ skipping 200 matching lines @@
                     ? num_tracked_prefs()
                     : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramUnchanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     ALLOW_ANY));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUnchangedDefault,
-                       DISABLED_UnchangedDefault);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUnchangedDefault, UnchangedDefault);
 
 // Augments PrefHashBrowserTestUnchangedDefault to confirm that nothing is reset
 // when nothing is tampered with, even if Chrome itself wrote custom prefs in
 // its last run.
 class PrefHashBrowserTestUnchangedCustom
     : public PrefHashBrowserTestUnchangedDefault {
  public:
   void SetupPreferences() override {
     profile()->GetPrefs()->SetString(prefs::kHomePage, "http://example.com");
 
     InstallExtensionWithUIAutoConfirm(
         test_data_dir_.AppendASCII("good.crx"), 1, browser());
   }
 
   void VerifyReactionToPrefAttack() override {
     // Make sure the settings written in the last run stuck.
     EXPECT_EQ("http://example.com",
               profile()->GetPrefs()->GetString(prefs::kHomePage));
 
     EXPECT_TRUE(extension_service()->GetExtensionById(kGoodCrxId, false));
 
     // Reaction should be identical to unattacked default prefs.
     PrefHashBrowserTestUnchangedDefault::VerifyReactionToPrefAttack();
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUnchangedCustom,
-                       DISABLED_UnchangedCustom);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUnchangedCustom, UnchangedCustom);
 
 // Verifies that cleared prefs are reported.
 class PrefHashBrowserTestClearedAtomic : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     profile()->GetPrefs()->SetString(prefs::kHomePage, "http://example.com");
   }
 
   void AttackPreferencesOnDisk(
       base::DictionaryValue* unprotected_preferences,
@@ skipping 51 matching lines @@
       // Expect homepage clearance to have been noticed by registry validation.
       EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramCleared,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 2));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestClearedAtomic,
-                       DISABLED_ClearedAtomic);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestClearedAtomic, ClearedAtomic);
 
 // Verifies that clearing the MACs results in untrusted Initialized pings for
 // non-null protected prefs.
 class PrefHashBrowserTestUntrustedInitialized : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     // Explicitly set the DSE (it's otherwise NULL by default, preventing
     // thorough testing of the PROTECTION_ENABLED_DSE level).
     DefaultSearchManager default_search_manager(
         profile()->GetPrefs(), DefaultSearchManager::ObserverCallback());
@@ skipping 113 matching lines @@
                     ? num_tracked_prefs()
                     : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramUnchanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     ALLOW_ANY));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedInitialized,
-                       DISABLED_UntrustedInitialized);
+                       UntrustedInitialized);
 
 // Verifies that changing an atomic pref results in it being reported (and reset
 // if the protection level allows it).
 class PrefHashBrowserTestChangedAtomic : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     profile()->GetPrefs()->SetInteger(prefs::kRestoreOnStartup,
                                       SessionStartupPref::URLS);
 
     ListPrefUpdate update(profile()->GetPrefs(),
@@ skipping 77 matching lines @@
       // Expect a single Changed event for tracked pref #4 (startup URLs).
       EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramChanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 4));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedAtomic,
-                       DISABLED_ChangedAtomic);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedAtomic, ChangedAtomic);
 
 // Verifies that changing or adding an entry in a split pref results in both
 // items being reported (and remove if the protection level allows it).
 class PrefHashBrowserTestChangedSplitPref : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     InstallExtensionWithUIAutoConfirm(
         test_data_dir_.AppendASCII("good.crx"), 1, browser());
   }
 
@@ skipping 86 matching lines @@
       // pref #5 (extensions).
       EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramChanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 5));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedSplitPref,
-                       DISABLED_ChangedSplitPref);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedSplitPref, ChangedSplitPref);
 
 // Verifies that adding a value to unprotected preferences for a key which is
 // still using the default (i.e. has no value stored in protected preferences)
 // doesn't allow that value to slip in with no valid MAC (regression test for
 // http://crbug.com/414554)
 class PrefHashBrowserTestUntrustedAdditionToPrefs
     : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     // Ensure there is no user-selected value for kRestoreOnStartup.
@@ skipping 63 matching lines @@
                     ? changed_expected
                     : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramChanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 3));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedAdditionToPrefs,
-                       DISABLED_UntrustedAdditionToPrefs);
+                       UntrustedAdditionToPrefs);
 
 // Verifies that adding a value to unprotected preferences while wiping a
 // user-selected value from protected preferences doesn't allow that value to
 // slip in with no valid MAC (regression test for http://crbug.com/414554).
 class PrefHashBrowserTestUntrustedAdditionToPrefsAfterWipe
     : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     profile()->GetPrefs()->SetString(prefs::kHomePage, "http://example.com");
   }
@@ skipping 65 matching lines @@
                     BEGIN_ALLOW_SINGLE_BUCKET + 2));
       EXPECT_EQ(cleared_expected,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramCleared,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 2));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedAdditionToPrefsAfterWipe,
-                       DISABLED_UntrustedAdditionToPrefsAfterWipe);
+                       UntrustedAdditionToPrefsAfterWipe);
 
 #if defined(OS_WIN)
 class PrefHashBrowserTestRegistryValidationFailure
     : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     profile()->GetPrefs()->SetString(prefs::kHomePage, "http://example.com");
   }
 
   void AttackPreferencesOnDisk(
@@ skipping 22 matching lines @@
       // (homepage).
       EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
                 GetTrackedPrefHistogramCount(
                     user_prefs::tracked::kTrackedPrefHistogramChanged,
                     user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
                     BEGIN_ALLOW_SINGLE_BUCKET + 2));
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestRegistryValidationFailure,
-                       DISABLED_RegistryValidationFailure);
+                       RegistryValidationFailure);
 #endif
 
 // Verifies that all preferences related to choice of default search engine are
 // protected.
 class PrefHashBrowserTestDefaultSearch : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
     // Set user selected default search engine.
     DefaultSearchManager default_search_manager(
         profile()->GetPrefs(), DefaultSearchManager::ObserverCallback());
@@ skipping 84 matching lines @@
       // Attack fails.
       EXPECT_EQ(DefaultSearchManager::FROM_FALLBACK, dse_source);
       EXPECT_NE(current_dse->keyword(), base::UTF8ToUTF16("badkeyword"));
       EXPECT_NE(current_dse->short_name(), base::UTF8ToUTF16("badname"));
       EXPECT_NE(current_dse->url(),
                 "http://bad_default_engine/search?q=dirty_user_query");
     }
   }
 };
 
-// Test is flaky. crbug.com/723639
-PREF_HASH_BROWSER_TEST(PrefHashBrowserTestDefaultSearch,
-                       DISABLED_SearchProtected);
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestDefaultSearch, SearchProtected);