Introduce IEImporterTestRegistryOverrider to be able to override the registry for a test and flag t…

chrome/browser/importer/ie_importer_test_registry_overrider_win.cc

+// Copyright (c) 2013 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/importer/ie_importer_test_registry_overrider_win.h"
+
+#include <windows.h>
+
+#include <string>
+
+#include "base/environment.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/win/registry.h"
+
+namespace {
+
+const wchar_t kTestHKCUOverrideKey[] = L"SOFTWARE\\Chromium Unit Tests";
+const wchar_t kTestHKCUOverrideSubKey[] =
+    L"SOFTWARE\\Chromium Unit Tests\\HKCU Override";
+const char kTestHKCUOverrideEnvironmentVariable[] =
+    "IE_IMPORTER_TEST_OVERRIDE_HKCU";
+const char kTestHKCUOverrideEnvironmentVariableValue[] = "1";
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// IEImporterTestRegistryOverrider, public:
+
+IEImporterTestRegistryOverrider::IEImporterTestRegistryOverrider()
+    : in_process_override_started_by_this_(false),
+      override_set_by_this_(false) {
+}
+
+IEImporterTestRegistryOverrider::~IEImporterTestRegistryOverrider() {
+  if (in_process_override_started_by_this_) {

[robertshield, 2013/04/30 03:15:42]

shouldn't this set override_active_in_process_ to false (also holding the lock)?

[gab, 2013/04/30 21:44:52]

This is done via static initialization at the end of this file (I debated
putting static initializers at the top, but decided to follow declaration order
in the class, WDYT?).

[robertshield, 2013/04/30 22:04:05]

I think that this is the destructor and that after the object is destroyed that
override_active_in_process_ should be reset from true back to false. How does
static initialization help with this?

[gab, 2013/04/30 22:56:47]

Err my bad, I somehow read this comment and saw the constructor initializer list
above and thought this comment referred to initialization....

Yes, you're right, will do (in fact I'm crossing my fingers and hoping this is
why tests were failing on the try bots... i.e., if this variable still said the
override was there it would not set it in the next test)!

+    if (RegOverridePredefKey(HKEY_CURRENT_USER, NULL) != ERROR_SUCCESS)
+      LOG(ERROR) << "Failed to unset registry override";
+
+    if (override_set_by_this_) {
+      base::win::RegKey key(HKEY_CURRENT_USER, kTestHKCUOverrideKey,
+                            KEY_ALL_ACCESS);
+      key.DeleteKey(L"");
+
+      scoped_ptr<base::Environment> env(base::Environment::Create());
+      env->UnSetVar(kTestHKCUOverrideEnvironmentVariable);
+    }
+  }
+}
+
+bool IEImporterTestRegistryOverrider::SetRegistryOverride() {
+  DCHECK(!IsEnvironmentSet());
+  override_set_by_this_ = true;
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  return StartRegistryOverride() &&
+      env->SetVar(kTestHKCUOverrideEnvironmentVariable,

[robertshield, 2013/04/30 03:15:42]

should the setvar / unsetvar calls also be protected?

[gab, 2013/04/30 21:44:52]

They are protected by contract (i.e. this method should only be called once
across all processes).

The DCHECK also verifies the contract was honored (i.e. no one else set the
environment yet); although a race could technically bypass the DCHECK, it is not
really intended as thread-safe check since there should really only be one
caller of this method...

[robertshield, 2013/04/30 22:04:05]

I'm not sure that protected-by-contract is a feature :-) Generally, either a
class is thread safe, or it isn't. Having parts of it be makes callers lives
more difficult.

It feels like some of the state changes in this class are protected by a lock
and some aren't and it isn't clear to me yet why not. 

[gab, 2013/04/30 22:56:47]

I disagree, callers shouldn't be confused, they should only call this method if
the environment isn't set (the contract is actually even stricter and says this
should only be called once across all processes; in theory it would be ok to
call it again after the initial setter has been destructed, but this is
unnecessary for our use-case and making the interface simpler/stricter is fine
imo).
A lock wouldn't help either as it wouldn't prevent a child process from racing
to set the environment (if it tried, but it shouldn't!).
By contract only one of these objects should be responsible to set the
environment and other objects should be "readers" (i.e. check the environment
and react accordingly).
The DCHECK also helps enforce the contract if a developer was to make the
mistake to call SetRegistryOverride from 2 different places.

+                  kTestHKCUOverrideEnvironmentVariableValue);
+}
+
+bool IEImporterTestRegistryOverrider::StartRegistryOverrideIfNeeded() {
+  if (!IsEnvironmentSet())
+    return true;
+  return StartRegistryOverride();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// IEImporterTestRegistryOverrider, private:
+
+bool IEImporterTestRegistryOverrider::StartRegistryOverride() {
+  base::AutoLock auto_lock(lock_.Get());
+  if (override_active_in_process_)
+    return true;
+
+  base::win::RegKey temp_hkcu_hive_key;
+  DWORD result = temp_hkcu_hive_key.Create(HKEY_CURRENT_USER,
+                                           kTestHKCUOverrideSubKey,
+                                           KEY_ALL_ACCESS);
+  if (result != ERROR_SUCCESS || !temp_hkcu_hive_key.Valid()) {
+    DPLOG(ERROR) << result;
+    return false;
+  }
+
+  override_active_in_process_ = true;
+  in_process_override_started_by_this_ = true;
+  return RegOverridePredefKey(HKEY_CURRENT_USER,
+                              temp_hkcu_hive_key.Handle()) == ERROR_SUCCESS;
+}
+
+bool IEImporterTestRegistryOverrider::IsEnvironmentSet() {
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  std::string value;
+  return env->GetVar(kTestHKCUOverrideEnvironmentVariable, &value) &&
+      value == kTestHKCUOverrideEnvironmentVariableValue;
+}
+
+// static
+bool IEImporterTestRegistryOverrider::override_active_in_process_ = false;
+base::LazyInstance<base::Lock>::Leaky IEImporterTestRegistryOverrider::lock_ =
+    LAZY_INSTANCE_INITIALIZER;