Make sure GoogleUpdateSettings::GetChromeChannel does not read

from the wrong section of registry when a per-user install has left
some registrations behind.
Fix the odd race and other bugs I encountered along the way.

BUG=40994
TEST=Unittests in this change. Open about box and look at the channel string.

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

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

5 files changed, 102 insertions, 16 deletions

diff --git a/chrome/installer/util/browser_distribution.cc b/chrome/installer/util/browser_distribution.cc
index 7eed257..53bddaf 100644
--- a/chrome/installer/util/browser_distribution.cc
+++ b/chrome/installer/util/browser_distribution.cc
@@ -12,6 +12,8 @@
 #include "base/command_line.h"
 #include "base/lock.h"
 #include "base/registry.h"
+#include "base/scoped_ptr.h"
+#include "base/singleton.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/installer/util/chrome_frame_distribution.h"
@@ -26,28 +28,32 @@ BrowserDistribution* BrowserDistribution::GetDistribution() {
   return GetDistribution(InstallUtil::IsChromeFrameProcess());
 }
 
+namespace {
+Singleton<Lock> g_dist_lock;
+scoped_ptr<BrowserDistribution> g_dist;
+};
+
 BrowserDistribution* BrowserDistribution::GetDistribution(bool chrome_frame) {
-  static BrowserDistribution* dist = NULL;
-  static Lock dist_lock;
-  AutoLock lock(dist_lock);
-  if (dist == NULL) {
+  AutoLock lock(*g_dist_lock.get());
+
+  if (g_dist == NULL) {
     if (chrome_frame) {
       // TODO(robertshield): Make one of these for Google Chrome vs
       // non Google Chrome builds?
-      dist = new ChromeFrameDistribution();
+      g_dist.reset(new ChromeFrameDistribution());
     } else {
 #if defined(GOOGLE_CHROME_BUILD)
       if (InstallUtil::IsChromeSxSProcess()) {
-        dist = new GoogleChromeSxSDistribution();
+        g_dist.reset(new GoogleChromeSxSDistribution());
       } else {
-        dist = new GoogleChromeDistribution();
+        g_dist.reset(new GoogleChromeDistribution());
       }
 #else
-      dist = new BrowserDistribution();
+      g_dist.reset(new BrowserDistribution());
 #endif
     }
   }
-  return dist;
+  return g_dist.get();
 }
 
 void BrowserDistribution::DoPostUninstallOperations(
diff --git a/chrome/installer/util/google_update_settings.cc b/chrome/installer/util/google_update_settings.cc
index 2dabe50..b23e717 100644
--- a/chrome/installer/util/google_update_settings.cc
+++ b/chrome/installer/util/google_update_settings.cc
@@ -4,14 +4,52 @@
 
 #include "chrome/installer/util/google_update_settings.h"
 
+#include "base/file_path.h"
+#include "base/path_service.h"
 #include "base/registry.h"
 #include "base/string_util.h"
+#include "base/singleton.h"
 #include "base/time.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/google_update_constants.h"
+#include "chrome/installer/util/install_util.h"
 
 namespace {
 
+// A helper class to initialize and keep the per-system install state.
+class SystemInstallKeeper {
+ public:
+  SystemInstallKeeper();
+
+  bool is_system_install() const { return is_system_install_; }
+  void set_is_system_install(bool is_system_install) {
+    is_system_install_ = is_system_install;
+  }
+
+ private:
+  bool is_system_install_;
+};
+
+SystemInstallKeeper::SystemInstallKeeper() {
+  FilePath module_path;
+  PathService::Get(base::FILE_MODULE, &module_path);
+  is_system_install_ =
+      !InstallUtil::IsPerUserInstall(module_path.value().c_str());
+}
+
+// We cache the system/user install state here so that it can be overridden
+// for unit testing, and also because deriving this bit of information is
+// expensive.
+Singleton<SystemInstallKeeper> g_system_install_keeper;
+
+// Returns HCKU or HKLM for per-user or per-system installs, resprectively.
+HKEY GetRootKeyForInstallMode() {
+  if (g_system_install_keeper->is_system_install())
+    return HKEY_LOCAL_MACHINE;
+  else
+    return HKEY_CURRENT_USER;
+}
+
 bool ReadGoogleUpdateStrKey(const wchar_t* const name, std::wstring* value) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
@@ -52,6 +90,14 @@ bool RemoveGoogleUpdateStrKey(const wchar_t* const name) {
 
 }  // namespace.
 
+void GoogleUpdateSettings::OverrideIsSystemInstall(bool is_system_install) {
+  g_system_install_keeper->set_is_system_install(is_system_install);
+}
+
+bool GoogleUpdateSettings::IsSystemInstall() {
+  return g_system_install_keeper->is_system_install();
+}
+
 bool GoogleUpdateSettings::GetCollectStatsConsent() {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring reg_path = dist->GetStateKey();
@@ -144,8 +190,12 @@ bool GoogleUpdateSettings::ClearReferral() {
 }
 
 bool GoogleUpdateSettings::GetChromeChannel(std::wstring* channel) {
+  HKEY root_key = GetRootKeyForInstallMode();
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  std::wstring reg_path = dist->GetStateKey();
+  RegKey key(root_key, reg_path.c_str(), KEY_READ);
   std::wstring update_branch;
-  if (!ReadGoogleUpdateStrKey(google_update::kRegApField, &update_branch)) {
+  if (!key.ReadValue(google_update::kRegApField, &update_branch)) {
     *channel = L"unknown";
     return false;
   }
@@ -166,5 +216,3 @@ bool GoogleUpdateSettings::GetChromeChannel(std::wstring* channel) {
 
   return true;
 }
-
-
diff --git a/chrome/installer/util/google_update_settings.h b/chrome/installer/util/google_update_settings.h
index 3edfedf..1a9c67d 100644
--- a/chrome/installer/util/google_update_settings.h
+++ b/chrome/installer/util/google_update_settings.h
@@ -10,10 +10,20 @@
 #include "base/basictypes.h"
 
 // This class provides accessors to the Google Update 'ClientState' information
-// that recorded when the user downloads the chrome installer. It is
-// google_update.exe responsability to write the initial values.
+// that recorded when the user downloads the chrome installer. It is the
+// responsibility of google_update.exe to write the initial values.
+// Note: this class has internal state that notes whether it should refer
+//    to a per-user or a per-machine install. This state is initially inferred
+//    from the path to this executable or DLL, as per the IsPerUserInstall
+//    function in InstallUtil.
 class GoogleUpdateSettings {
  public:
+  // This function overrides the internal per-system setting, used for testing.
+  static void OverrideIsSystemInstall(bool is_system_install);
+
+  // Returns true iff this executable is a per system install.
+  static bool IsSystemInstall();
+
   // Returns whether the user has given consent to collect UMA data and send
   // crash dumps to Google. This information is collected by the web server
   // used to download the chrome installer.
diff --git a/chrome/installer/util/google_update_settings_unittest.cc b/chrome/installer/util/google_update_settings_unittest.cc
index bb2e46c..4b245e0 100644
--- a/chrome/installer/util/google_update_settings_unittest.cc
+++ b/chrome/installer/util/google_update_settings_unittest.cc
@@ -38,6 +38,9 @@ class GoogleUpdateSettingsTest: public testing::Test {
               ::RegOverridePredefKey(HKEY_CURRENT_USER, hkcu_.Handle()));
     ASSERT_EQ(ERROR_SUCCESS,
               ::RegOverridePredefKey(HKEY_LOCAL_MACHINE, hklm_.Handle()));
+
+    // Normalize the GoogleUpdateSettings to per-system for all tests.
+    GoogleUpdateSettings::OverrideIsSystemInstall(true);
   }
 
   virtual void TearDown() {
@@ -122,11 +125,18 @@ class GoogleUpdateSettingsTest: public testing::Test {
 
 }  // namespace
 
-// Verify that we return failure on no registration.
+// Verify that we return failure on no registration,
+// whether per-system or per-user install.
 TEST_F(GoogleUpdateSettingsTest, CurrentChromeChannelAbsent) {
+  // Per-system first.
   std::wstring channel;
   EXPECT_FALSE(GoogleUpdateSettings::GetChromeChannel(&channel));
   EXPECT_STREQ(L"unknown", channel.c_str());
+
+  // Then per-user.
+  GoogleUpdateSettings::OverrideIsSystemInstall(false);
+  EXPECT_FALSE(GoogleUpdateSettings::GetChromeChannel(&channel));
+  EXPECT_STREQ(L"unknown", channel.c_str());
 }
 
 // Test an empty Ap key for system and user.
@@ -135,11 +145,20 @@ TEST_F(GoogleUpdateSettingsTest, CurrentChromeChannelEmptySystem) {
   std::wstring channel;
   EXPECT_TRUE(GoogleUpdateSettings::GetChromeChannel(&channel));
   EXPECT_STREQ(L"", channel.c_str());
+
+  // Per-user lookups should fail.
+  GoogleUpdateSettings::OverrideIsSystemInstall(false);
+  EXPECT_FALSE(GoogleUpdateSettings::GetChromeChannel(&channel));
 }
 
 TEST_F(GoogleUpdateSettingsTest, CurrentChromeChannelEmptyUser) {
   SetApField(USER_INSTALL, L"");
+  // Per-system lookup should fail.
   std::wstring channel;
+  EXPECT_FALSE(GoogleUpdateSettings::GetChromeChannel(&channel));
+
+  // Per-user lookup should succeed.
+  GoogleUpdateSettings::OverrideIsSystemInstall(false);
   EXPECT_TRUE(GoogleUpdateSettings::GetChromeChannel(&channel));
   EXPECT_STREQ(L"", channel.c_str());
 }
@@ -149,5 +168,6 @@ TEST_F(GoogleUpdateSettingsTest, CurrentChromeChannelVariousApValuesSystem) {
 }
 
 TEST_F(GoogleUpdateSettingsTest, CurrentChromeChannelVariousApValuesUser) {
+  GoogleUpdateSettings::OverrideIsSystemInstall(false);
   TestCurrentChromeChannelWithVariousApValues(USER_INSTALL);
 }
diff --git a/chrome/installer/util/install_util.cc b/chrome/installer/util/install_util.cc
index a3f32d1..1f22cd6 100644
--- a/chrome/installer/util/install_util.cc
+++ b/chrome/installer/util/install_util.cc
@@ -124,7 +124,9 @@ bool InstallUtil::IsPerUserInstall(const wchar_t* const exe_path) {
   wchar_t program_files_path[MAX_PATH] = {0};
   if (SUCCEEDED(SHGetFolderPath(NULL, CSIDL_PROGRAM_FILES, NULL,
                                 SHGFP_TYPE_CURRENT, program_files_path))) {
-    return !StartsWith(exe_path, program_files_path, false);
+    std::wstring program_files(program_files_path);
+    std::wstring exe_path_truncated(exe_path, program_files.size());
+    return !FilePath::CompareEqualIgnoreCase(program_files, exe_path_truncated);
   } else {
     NOTREACHED();
   }