Temporary workaround to get tests that reference singletons either directly or indirectly to pass.

With the recent Singleton refactoring, these tests started exposing some inherent problems with singletons, namely that one test can reference a state set by a different test, which can cause all sorts of flakiness.
Furthermore, some of these tests could reference the registry which may have some user specific settings that could cause the tests to fail

To work around these issues, I'm adding a few utility classes for tests (registry virtualization) and exposing a method to reset singletons before a test is run.

TEST=Fixes flakiness and a few tests that could fail on the waterfall.
BUG=none

Review URL: http://codereview.chromium.org/5564009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@68701 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 138 insertions, 88 deletions

diff --git a/chrome_frame/chrome_frame_unittest_main.cc b/chrome_frame/chrome_frame_unittest_main.cc
index 70157a4..4050f9b 100644
--- a/chrome_frame/chrome_frame_unittest_main.cc
+++ b/chrome_frame/chrome_frame_unittest_main.cc
@@ -1,31 +1,6 @@
-// Copyright 2009, Google Inc.
-// All rights reserved.
-//
-// Redistribution and use in source and binary forms, with or without
-// modification, are permitted provided that the following conditions are
-// met:
-//
-//    * Redistributions of source code must retain the above copyright
-// notice, this list of conditions and the following disclaimer.
-//    * Redistributions in binary form must reproduce the above
-// copyright notice, this list of conditions and the following disclaimer
-// in the documentation and/or other materials provided with the
-// distribution.
-//    * Neither the name of Google Inc. nor the names of its
-// contributors may be used to endorse or promote products derived from
-// this software without specific prior written permission.
-//
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+// Copyright (c) 2010 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 <atlbase.h>
 #include <atlcom.h>
@@ -38,11 +13,22 @@ class ObligatoryModule: public CAtlExeModuleT<ObligatoryModule> {
 
 ObligatoryModule g_obligatory_atl_module;
 
+static base::AtExitManager* g_at_exit_manager = NULL;
+
+void DeleteAllSingletons() {
+  if (g_at_exit_manager) {
+    g_at_exit_manager->ProcessCallbacksNow();
+  }
+}
+
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
 
   base::AtExitManager at_exit_manager;
+  g_at_exit_manager = &at_exit_manager;
   CommandLine::Init(argc, argv);
 
   RUN_ALL_TESTS();
+
+  g_at_exit_manager = NULL;
 }
diff --git a/chrome_frame/test/chrome_frame_test_utils.cc b/chrome_frame/test/chrome_frame_test_utils.cc
index f44f22c..49c18c6 100644
--- a/chrome_frame/test/chrome_frame_test_utils.cc
+++ b/chrome_frame/test/chrome_frame_test_utils.cc
@@ -29,6 +29,7 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome_frame/utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace chrome_frame_test {
 
@@ -52,6 +53,9 @@ const wchar_t kIEProfileName[] = L"iexplore";
 const wchar_t kChromeLauncher[] = L"chrome_launcher.exe";
 const int kChromeFrameLongNavigationTimeoutInSeconds = 10;
 
+const wchar_t TempRegKeyOverride::kTempTestKeyPath[] =
+    L"Software\\Chromium\\TempTestKeys";
+
 // Callback function for EnumThreadWindows.
 BOOL CALLBACK CloseWindowsThreadCallback(HWND hwnd, LPARAM param) {
   int& count = *reinterpret_cast<int*>(param);
@@ -624,4 +628,40 @@ base::ProcessHandle StartCrashService() {
   }
 }
 
+TempRegKeyOverride::TempRegKeyOverride(HKEY override, const wchar_t* temp_name)
+    : override_(override), temp_name_(temp_name) {
+  DCHECK(temp_name && lstrlenW(temp_name));
+  std::wstring key_path(kTempTestKeyPath);
+  key_path += L"\\" + temp_name_;
+  EXPECT_TRUE(temp_key_.Create(HKEY_CURRENT_USER, key_path.c_str(),
+                               KEY_ALL_ACCESS));
+  EXPECT_EQ(ERROR_SUCCESS,
+            ::RegOverridePredefKey(override_, temp_key_.Handle()));
+}
+
+TempRegKeyOverride::~TempRegKeyOverride() {
+  ::RegOverridePredefKey(override_, NULL);
+  // The temp key will be deleted via a call to DeleteAllTempKeys().
+}
+
+// static
+void TempRegKeyOverride::DeleteAllTempKeys() {
+  base::win::RegKey key;
+  if (key.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS)) {
+    key.DeleteKey(kTempTestKeyPath);
+  }
+}
+
+ScopedVirtualizeHklmAndHkcu::ScopedVirtualizeHklmAndHkcu() {
+  TempRegKeyOverride::DeleteAllTempKeys();
+  hklm_.reset(new TempRegKeyOverride(HKEY_LOCAL_MACHINE, L"hklm_fake"));
+  hkcu_.reset(new TempRegKeyOverride(HKEY_CURRENT_USER, L"hkcu_fake"));
+}
+
+ScopedVirtualizeHklmAndHkcu::~ScopedVirtualizeHklmAndHkcu() {
+  hkcu_.reset(NULL);
+  hklm_.reset(NULL);
+  TempRegKeyOverride::DeleteAllTempKeys();
+}
+
 }  // namespace chrome_frame_test
diff --git a/chrome_frame/test/chrome_frame_test_utils.h b/chrome_frame/test/chrome_frame_test_utils.h
index 82febb2..5d20e8a 100644
--- a/chrome_frame/test/chrome_frame_test_utils.h
+++ b/chrome_frame/test/chrome_frame_test_utils.h
@@ -21,6 +21,8 @@
 #include "base/message_loop.h"
 #include "base/process_util.h"
 #include "base/scoped_comptr_win.h"
+#include "base/scoped_ptr.h"
+#include "base/win/registry.h"
 
 #include "chrome_frame/test_utils.h"
 #include "chrome_frame/test/simulate_input.h"
@@ -177,6 +179,41 @@ class CloseIeAtEndOfScope {
 // during test runs.
 base::ProcessHandle StartCrashService();
 
+class TempRegKeyOverride {
+ public:
+  static const wchar_t kTempTestKeyPath[];
+
+  TempRegKeyOverride(HKEY override, const wchar_t* temp_name);
+  ~TempRegKeyOverride();
+
+  static void DeleteAllTempKeys();
+
+ protected:
+  HKEY override_;
+  base::win::RegKey temp_key_;
+  std::wstring temp_name_;
+};
+
+// Used in tests where we reference the registry and don't want to run into
+// problems where existing registry settings might conflict with the
+// expectations of the test.
+class ScopedVirtualizeHklmAndHkcu {
+ public:
+  ScopedVirtualizeHklmAndHkcu();
+  ~ScopedVirtualizeHklmAndHkcu();
+
+ protected:
+  scoped_ptr<TempRegKeyOverride> hklm_;
+  scoped_ptr<TempRegKeyOverride> hkcu_;
+};
+
 }  // namespace chrome_frame_test
 
+// TODO(tommi): This is a temporary workaround while we're getting our
+// Singleton story straight.  Ideally each test should clear up any singletons
+// it might have created, but test cases do not implicitly have their own
+// AtExitManager, so we have this workaround method for tests that depend on
+// "fresh" singletons.  The implementation is in chrome_frame_unittest_main.cc.
+void DeleteAllSingletons();
+
 #endif  // CHROME_FRAME_TEST_CHROME_FRAME_TEST_UTILS_H_
diff --git a/chrome_frame/test/policy_settings_unittest.cc b/chrome_frame/test/policy_settings_unittest.cc
index 53c4df0..771762f 100644
--- a/chrome_frame/test/policy_settings_unittest.cc
+++ b/chrome_frame/test/policy_settings_unittest.cc
@@ -11,9 +11,12 @@
 #include "base/win/registry.h"
 #include "chrome/common/policy_constants.h"
 #include "chrome_frame/policy_settings.h"
+#include "chrome_frame/test/chrome_frame_test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::win::RegKey;
+using chrome_frame_test::ScopedVirtualizeHklmAndHkcu;
+using chrome_frame_test::TempRegKeyOverride;
 
 namespace {
 
@@ -30,42 +33,6 @@ void DeleteChromeFramePolicyEntries(HKEY root) {
   }
 }
 
-class TempRegKeyOverride {
- public:
-  static const wchar_t kTempTestKeyPath[];
-
-  TempRegKeyOverride(HKEY override, const wchar_t* temp_name)
-      : override_(override), temp_name_(temp_name) {
-    DCHECK(temp_name && lstrlenW(temp_name));
-    std::wstring key_path(kTempTestKeyPath);
-    key_path += L"\\" + temp_name_;
-    EXPECT_TRUE(temp_key_.Create(HKEY_CURRENT_USER, key_path.c_str(),
-                                 KEY_ALL_ACCESS));
-    EXPECT_EQ(ERROR_SUCCESS,
-              ::RegOverridePredefKey(override_, temp_key_.Handle()));
-  }
-
-  ~TempRegKeyOverride() {
-    ::RegOverridePredefKey(override_, NULL);
-    // The temp key will be deleted via a call to DeleteAllTempKeys().
-  }
-
-  static void DeleteAllTempKeys() {
-    RegKey key;
-    if (key.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS)) {
-      key.DeleteKey(kTempTestKeyPath);
-    }
-  }
-
- protected:
-  HKEY override_;
-  RegKey temp_key_;
-  std::wstring temp_name_;
-};
-
-const wchar_t TempRegKeyOverride::kTempTestKeyPath[] =
-    L"Software\\Chromium\\TempTestKeys";
-
 bool InitializePolicyKey(HKEY policy_root, RegKey* policy_key) {
   EXPECT_TRUE(policy_key->Create(policy_root, policy::kRegistrySubKey,
                                  KEY_ALL_ACCESS));
@@ -136,28 +103,21 @@ bool SetChromeApplicationLocale(HKEY policy_root, const wchar_t* locale) {
 class PolicySettingsTest : public testing::Test {
  protected:
   void SetUp() {
-    TempRegKeyOverride::DeleteAllTempKeys();
-
-    hklm_pol_.reset(new TempRegKeyOverride(HKEY_LOCAL_MACHINE, L"hklm_pol"));
-    hkcu_pol_.reset(new TempRegKeyOverride(HKEY_CURRENT_USER, L"hkcu_pol"));
-
     ResetPolicySettings();
   }
 
   void TearDown() {
-    hkcu_pol_.reset(NULL);
-    hklm_pol_.reset(NULL);
-    TempRegKeyOverride::DeleteAllTempKeys();
   }
 
   void ResetPolicySettings() {
-    at_exit_manager_.ProcessCallbacksNow();
+    //at_exit_manager_.ProcessCallbacksNow();
+    DeleteAllSingletons();
   }
 
   // This is used to manage life cycle of PolicySettings singleton.
-  base::ShadowingAtExitManager at_exit_manager_;
-  scoped_ptr<TempRegKeyOverride> hklm_pol_;
-  scoped_ptr<TempRegKeyOverride> hkcu_pol_;
+  // base::ShadowingAtExitManager at_exit_manager_;
+
+  ScopedVirtualizeHklmAndHkcu registry_virtualization_;
 };
 
 TEST_F(PolicySettingsTest, RendererForUrl) {
diff --git a/chrome_frame/test/urlmon_moniker_unittest.cc b/chrome_frame/test/urlmon_moniker_unittest.cc
index 1d889bf..d644be7 100644
--- a/chrome_frame/test/urlmon_moniker_unittest.cc
+++ b/chrome_frame/test/urlmon_moniker_unittest.cc
@@ -9,8 +9,10 @@
 #include "base/path_service.h"
 #include "base/scoped_comptr_win.h"
 #include "chrome_frame/urlmon_bind_status_callback.h"
+#include "chrome_frame/test/chrome_frame_test_utils.h"
 #include "chrome_frame/test/urlmon_moniker_tests.h"
 
+using chrome_frame_test::ScopedVirtualizeHklmAndHkcu;
 using testing::Return;
 using testing::Eq;
 
@@ -20,6 +22,7 @@ class MonikerPatchTest : public testing::Test {
   }
 
   virtual void SetUp() {
+    DeleteAllSingletons();
     PathService::Get(base::DIR_SOURCE_ROOT, &test_file_path_);
     test_file_path_ = test_file_path_.Append(FILE_PATH_LITERAL("chrome_frame"))
         .Append(FILE_PATH_LITERAL("test"))
@@ -58,6 +61,7 @@ class MonikerPatchTest : public testing::Test {
   }
 
   FilePath test_file_path_;
+  ScopedVirtualizeHklmAndHkcu virtualized_registry_;
 };
 
 // Tests the CacheStream class by writing content into a stream object
diff --git a/chrome_frame/test/util_unittests.cc b/chrome_frame/test/util_unittests.cc
index 6581184..5e4e524 100644
--- a/chrome_frame/test/util_unittests.cc
+++ b/chrome_frame/test/util_unittests.cc
@@ -6,17 +6,41 @@
 #include "base/file_version_info.h"
 #include "base/file_version_info_win.h"
 #include "base/win/registry.h"
+#include "chrome_frame/test/chrome_frame_test_utils.h"
 #include "chrome_frame/utils.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using base::win::RegKey;
+using chrome_frame_test::TempRegKeyOverride;
 
 const wchar_t kChannelName[] = L"-dev";
 const wchar_t kSuffix[] = L"-fix";
 
-TEST(UtilTests, GetModuleVersionTest) {
+class UtilTests : public testing::Test {
+ protected:
+  void SetUp() {
+    TempRegKeyOverride::DeleteAllTempKeys();
+    DeleteAllSingletons();
+
+    hklm_pol_.reset(new TempRegKeyOverride(HKEY_LOCAL_MACHINE, L"hklm_fake"));
+    hkcu_pol_.reset(new TempRegKeyOverride(HKEY_CURRENT_USER, L"hkcu_fake"));
+  }
+
+  void TearDown() {
+    hkcu_pol_.reset(NULL);
+    hklm_pol_.reset(NULL);
+    TempRegKeyOverride::DeleteAllTempKeys();
+  }
+
+  // This is used to manage life cycle of PolicySettings singleton.
+  // base::ShadowingAtExitManager at_exit_manager_;
+  scoped_ptr<TempRegKeyOverride> hklm_pol_;
+  scoped_ptr<TempRegKeyOverride> hkcu_pol_;
+};
+
+TEST_F(UtilTests, GetModuleVersionTest) {
   HMODULE mod = GetModuleHandle(L"kernel32.dll");
   EXPECT_NE(mod, static_cast<HMODULE>(NULL));
   wchar_t path[MAX_PATH] = {0};
@@ -43,7 +67,7 @@ TEST(UtilTests, GetModuleVersionTest) {
   EXPECT_EQ(fixed_info->dwFileVersionLS, static_cast<DWORD>(low));
 }
 
-TEST(UtilTests, HaveSameOrigin) {
+TEST_F(UtilTests, HaveSameOrigin) {
   struct OriginCompare {
     const char* a;
     const char* b;
@@ -68,7 +92,7 @@ TEST(UtilTests, HaveSameOrigin) {
   }
 }
 
-TEST(UtilTests, IsValidUrlScheme) {
+TEST_F(UtilTests, IsValidUrlScheme) {
   struct Cases {
     const wchar_t* url;
     bool is_privileged;
@@ -102,7 +126,7 @@ TEST(UtilTests, IsValidUrlScheme) {
   }
 }
 
-TEST(UtilTests, GuidToString) {
+TEST_F(UtilTests, GuidToString) {
   // {3C5E2125-35BA-48df-A841-5F669B9D69FC}
   const GUID test_guid = { 0x3c5e2125, 0x35ba, 0x48df,
       { 0xa8, 0x41, 0x5f, 0x66, 0x9b, 0x9d, 0x69, 0xfc } };
@@ -115,12 +139,12 @@ TEST(UtilTests, GuidToString) {
   EXPECT_EQ(static_cast<size_t>(lstrlenW(compare)), str_guid.length());
 }
 
-TEST(UtilTests, GetTempInternetFiles) {
+TEST_F(UtilTests, GetTempInternetFiles) {
   FilePath path = GetIETemporaryFilesFolder();
   EXPECT_FALSE(path.empty());
 }
 
-TEST(UtilTests, ParseAttachTabUrlTest) {
+TEST_F(UtilTests, ParseAttachTabUrlTest) {
   ChromeFrameUrl cf_url;
 
   static const std::string kProfileName("iexplore");
@@ -202,7 +226,7 @@ class MockIInternetSecurityManager : public IInternetSecurityManager {
       HRESULT(DWORD zone, IEnumString** enum_string, DWORD flags));
 };
 
-TEST(UtilTests, CanNavigateTest) {
+TEST_F(UtilTests, CanNavigateTest) {
   MockIInternetSecurityManager mock;
 
   struct Zones {
@@ -293,7 +317,7 @@ TEST(UtilTests, CanNavigateTest) {
   SetConfigBool(kAllowUnsafeURLs, enable_gcf);
 }
 
-TEST(UtilTests, IsDefaultRendererTest) {
+TEST_F(UtilTests, IsDefaultRendererTest) {
   RegKey config_key(HKEY_CURRENT_USER, kChromeFrameConfigKey, KEY_ALL_ACCESS);
   EXPECT_TRUE(config_key.Valid());
 
@@ -312,7 +336,7 @@ TEST(UtilTests, IsDefaultRendererTest) {
   config_key.WriteValue(kEnableGCFRendererByDefault, saved_default_renderer);
 }
 
-TEST(UtilTests, RendererTypeForUrlTest) {
+TEST_F(UtilTests, RendererTypeForUrlTest) {
   // Open all the keys we need.
   RegKey config_key(HKEY_CURRENT_USER, kChromeFrameConfigKey, KEY_ALL_ACCESS);
   EXPECT_TRUE(config_key.Valid());
@@ -356,7 +380,7 @@ TEST(UtilTests, RendererTypeForUrlTest) {
   config_key.WriteValue(kEnableGCFRendererByDefault, saved_default_renderer);
 }
 
-TEST(UtilTests, XUaCompatibleDirectiveTest) {
+TEST_F(UtilTests, XUaCompatibleDirectiveTest) {
   int all_versions[] = {0, 1, 2, 5, 6, 7, 8, 9, 10, 11, 99, 100, 101, 1000};
 
   struct Cases {
diff --git a/chrome_frame/urlmon_bind_status_callback.cc b/chrome_frame/urlmon_bind_status_callback.cc
index 3d03542..474f8f3 100644
--- a/chrome_frame/urlmon_bind_status_callback.cc
+++ b/chrome_frame/urlmon_bind_status_callback.cc
@@ -171,7 +171,7 @@ void SniffData::DetermineRendererType(bool last_chance) {
   if (is_undetermined()) {
     if (last_chance)
       renderer_type_ = OTHER;
-    if (IsChrome(RendererTypeForUrl(url_.c_str()))) {
+    if (IsChrome(RendererTypeForUrl(url_))) {
       renderer_type_ = CHROME;
     } else {
       if (is_cache_valid() && cache_) {
@@ -428,4 +428,3 @@ bool BSCBStorageBind::ShouldCacheProgress(unsigned long status_code) const {
 
   return false;
 }
-