GCAPI should append to the existing experiment_labels instead of clobbering them.

As described on http://crbug.com/266955#c7

Also adding AtExitManager to gcapi_test.exe; this is required to support MasterPreferences's LazyInstance used by BrowserDistribution, used by google_update's ReadExperimentLabels().

Introducing GCAPITestRegistryOverrider as a class to be added as a member to GCAPI test fixtures that require registry overriding; extracted from the existing GCAPIReactivationTest fixture.

Move Windows-specific variations_util.cc code to experiment_labels_win.cc

BUG=266955
TEST=gcapi_test.exe

Review URL: https://codereview.chromium.org/23579003

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

7 files changed, 316 insertions, 40 deletions

diff --git a/chrome/installer/gcapi/gcapi_omaha_experiment.cc b/chrome/installer/gcapi/gcapi_omaha_experiment.cc
index ac5899f..d45067e 100644
--- a/chrome/installer/gcapi/gcapi_omaha_experiment.cc
+++ b/chrome/installer/gcapi/gcapi_omaha_experiment.cc
@@ -4,55 +4,102 @@
 
 #include "chrome/installer/gcapi/gcapi_omaha_experiment.h"
 
-#include "base/strings/string16.h"
+#include "base/basictypes.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "chrome/installer/gcapi/gcapi.h"
+#include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/google_update_experiment_util.h"
 #include "chrome/installer/util/google_update_settings.h"
 
-using base::Time;
-using base::TimeDelta;
-
 namespace {
 
 // Returns the number of weeks since 2/3/2003.
-int GetCurrentRlzWeek() {
-  Time::Exploded february_third_2003_exploded = {2003, 2, 1, 3, 0, 0, 0, 0};
-  Time f = Time::FromUTCExploded(february_third_2003_exploded);
-  TimeDelta delta = Time::Now() - f;
+int GetCurrentRlzWeek(const base::Time& current_time) {
+  base::Time::Exploded february_third_2003_exploded =
+      {2003, 2, 1, 3, 0, 0, 0, 0};
+  base::Time f = base::Time::FromUTCExploded(february_third_2003_exploded);
+  base::TimeDelta delta = current_time - f;
   return delta.InDays() / 7;
 }
 
-bool SetLabel(const wchar_t* brand_code, const wchar_t* label, int shell_mode) {
+bool SetExperimentLabel(const wchar_t* brand_code,
+                        const string16& label,
+                        int shell_mode) {
   if (!brand_code) {
     return false;
   }
 
-  int week_number = GetCurrentRlzWeek();
-  if (week_number < 0 || week_number > 999)
-    week_number = 999;
+  const bool system_level = shell_mode == GCAPI_INVOKED_UAC_ELEVATION;
 
-  string16 experiment_labels;
-  base::SStringPrintf(&experiment_labels,
-                      L"%ls=%ls_%d|%ls",
-                      label,
-                      brand_code,
-                      week_number,
-                      installer::BuildExperimentDateString().c_str());
+  string16 original_labels;
+  if (!GoogleUpdateSettings::ReadExperimentLabels(system_level,
+                                                  &original_labels)) {
+    return false;
+  }
+
+  // Split the original labels by the label separator.
+  std::vector<string16> entries;
+  base::SplitStringUsingSubstr(
+      original_labels, google_update::kExperimentLabelSep, &entries);
+
+  // Keep all labels, but the one we want to add/replace.
+  string16 new_labels;
+  for (std::vector<string16>::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {
+    if (!it->empty() && !StartsWith(*it, label + L"=", true)) {
+      new_labels += *it;
+      new_labels += google_update::kExperimentLabelSep;
+    }
+  }
+
+  new_labels.append(
+      gcapi_internals::GetGCAPIExperimentLabel(brand_code, label));
 
-  return GoogleUpdateSettings::SetExperimentLabels(
-      shell_mode == GCAPI_INVOKED_UAC_ELEVATION,
-      experiment_labels);
+  return GoogleUpdateSettings::SetExperimentLabels(system_level,
+                                                   new_labels);
 }
 
 }  // namespace
 
+namespace gcapi_internals {
+
+const wchar_t kReactivationLabel[] = L"reacbrand";
+const wchar_t kRelaunchLabel[] = L"relaunchbrand";
+
+string16 GetGCAPIExperimentLabel(const wchar_t* brand_code,
+                                 const string16& label) {
+  // Keeps a fixed time state for this GCAPI instance; this makes tests reliable
+  // when crossing time boundaries on the system clock and doesn't otherwise
+  // affect results of this short lived binary.
+  static time_t instance_time_value = 0;
+  if (instance_time_value == 0)
+    instance_time_value = base::Time::Now().ToTimeT();
+
+  base::Time instance_time = base::Time::FromTimeT(instance_time_value);
+
+  string16 gcapi_experiment_label;
+  base::SStringPrintf(&gcapi_experiment_label,
+                      L"%ls=%ls_%d|%ls",
+                      label.c_str(),
+                      brand_code,
+                      GetCurrentRlzWeek(instance_time),
+                      installer::BuildExperimentDateString(
+                          instance_time).c_str());
+  return gcapi_experiment_label;
+}
+
+}  // namespace gcapi_internals
+
 bool SetReactivationExperimentLabels(const wchar_t* brand_code,
                                      int shell_mode) {
-  return SetLabel(brand_code, L"reacbrand", shell_mode);
+  return SetExperimentLabel(brand_code, gcapi_internals::kReactivationLabel,
+                            shell_mode);
 }
 
 bool SetRelaunchExperimentLabels(const wchar_t* brand_code, int shell_mode) {
-  return SetLabel(brand_code, L"relaunchbrand", shell_mode);
+  return SetExperimentLabel(brand_code, gcapi_internals::kRelaunchLabel,
+                            shell_mode);
 }
diff --git a/chrome/installer/gcapi/gcapi_omaha_experiment.h b/chrome/installer/gcapi/gcapi_omaha_experiment.h
index babe41e..d64bc4b 100644
--- a/chrome/installer/gcapi/gcapi_omaha_experiment.h
+++ b/chrome/installer/gcapi/gcapi_omaha_experiment.h
@@ -5,6 +5,20 @@
 #ifndef CHROME_INSTALLER_GCAPI_GCAPI_OMAHA_EXPERIMENT_H_
 #define CHROME_INSTALLER_GCAPI_GCAPI_OMAHA_EXPERIMENT_H_
 
+#include "base/strings/string16.h"
+
+namespace gcapi_internals {
+
+extern const wchar_t kReactivationLabel[];
+extern const wchar_t kRelaunchLabel[];
+
+// Returns the full experiment label to be used by |label| (which is one of the
+// labels declared above) for |brand_code|.
+string16 GetGCAPIExperimentLabel(const wchar_t* brand_code,
+                                 const string16& label);
+
+}  // namespace gcapi_internals
+
 // Writes a reactivation brand code experiment label in the Chrome product and
 // binaries registry keys for |brand_code|. This experiment label will have a
 // expiration date of now plus one year. If |shell_mode| is set to
diff --git a/chrome/installer/gcapi/gcapi_omaha_experiment_test.cc b/chrome/installer/gcapi/gcapi_omaha_experiment_test.cc
new file mode 100644
index 0000000..132cd1f
--- /dev/null
+++ b/chrome/installer/gcapi/gcapi_omaha_experiment_test.cc
@@ -0,0 +1,180 @@
+// Copyright 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/installer/gcapi/gcapi_omaha_experiment.h"
+
+#include "chrome/installer/gcapi/gcapi.h"
+#include "chrome/installer/gcapi/gcapi_test_registry_overrider.h"
+#include "chrome/installer/util/google_update_constants.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const wchar_t kBrand[] = L"ABCD";
+const uint16 kUserLevel = GCAPI_INVOKED_STANDARD_SHELL;
+
+const wchar_t kSomeExperiments[] = L"myexp=1|Aug 2;yourexp=2|Sep 5";
+const wchar_t kSomeOtherExperiments[] = L"anotherexp=joe|Jun 7 2008";
+const wchar_t kSomeMoreExperiments[] = L"moreexp=foo|Jul 31 1999";
+
+class GCAPIOmahaExperimentTest : public ::testing::Test {
+ protected:
+  GCAPIOmahaExperimentTest()
+      : brand_(kBrand),
+        reactivation_label_(gcapi_internals::GetGCAPIExperimentLabel(
+            kBrand, gcapi_internals::kReactivationLabel)),
+        relaunch_label_(gcapi_internals::GetGCAPIExperimentLabel(
+            kBrand, gcapi_internals::kRelaunchLabel)) {
+  }
+
+  void VerifyExperimentLabels(const string16& expected_labels) {
+    string16 actual_labels;
+    EXPECT_TRUE(GoogleUpdateSettings::ReadExperimentLabels(false,
+                                                           &actual_labels));
+    EXPECT_EQ(expected_labels, actual_labels);
+  }
+
+  string16 brand_;
+  string16 reactivation_label_;
+  string16 relaunch_label_;
+
+  const GCAPITestRegistryOverrider gcapi_test_registry_overrider_;
+};
+
+TEST_F(GCAPIOmahaExperimentTest, SetReactivationLabelFromEmptyExperiments) {
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+  VerifyExperimentLabels(reactivation_label_);
+}
+
+// Test the relaunch label once; all other tests go more in depth, but since
+// both labels use the same logic underneath there is no need to test both in
+// depth.
+TEST_F(GCAPIOmahaExperimentTest, SetRelaunchLabelFromEmptyExperiments) {
+  ASSERT_TRUE(SetRelaunchExperimentLabels(kBrand, kUserLevel));
+  VerifyExperimentLabels(relaunch_label_);
+}
+
+TEST_F(GCAPIOmahaExperimentTest, SetReactivationLabelWithExistingExperiments) {
+  GoogleUpdateSettings::SetExperimentLabels(false, kSomeExperiments);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+TEST_F(GCAPIOmahaExperimentTest,
+       SetReactivationLabelWithExistingIdenticalExperiment) {
+  string16 previous_labels(kSomeExperiments);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(reactivation_label_);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(kSomeOtherExperiments);
+  GoogleUpdateSettings::SetExperimentLabels(false, previous_labels);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(kSomeOtherExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+TEST_F(GCAPIOmahaExperimentTest,
+       SetReactivationLabelWithExistingIdenticalAtBeginning) {
+  string16 previous_labels(reactivation_label_);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(kSomeExperiments);
+  GoogleUpdateSettings::SetExperimentLabels(false, previous_labels);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+TEST_F(GCAPIOmahaExperimentTest,
+       SetReactivationLabelWithFakeMatchInAnExperiment) {
+  string16 previous_labels(kSomeExperiments);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(L"blah_");
+  // Shouldn't match deletion criteria.
+  previous_labels.append(reactivation_label_);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(kSomeOtherExperiments);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  // Should match the deletion criteria.
+  previous_labels.append(reactivation_label_);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(kSomeMoreExperiments);
+  GoogleUpdateSettings::SetExperimentLabels(false, previous_labels);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(L"blah_");
+  expected_labels.append(reactivation_label_);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(kSomeOtherExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(kSomeMoreExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+TEST_F(GCAPIOmahaExperimentTest,
+       SetReactivationLabelWithFakeMatchInAnExperimentAndNoRealMatch) {
+  string16 previous_labels(kSomeExperiments);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(L"blah_");
+  // Shouldn't match deletion criteria.
+  previous_labels.append(reactivation_label_);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  previous_labels.append(kSomeOtherExperiments);
+  GoogleUpdateSettings::SetExperimentLabels(false, previous_labels);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(L"blah_");
+  expected_labels.append(reactivation_label_);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(kSomeOtherExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+TEST_F(GCAPIOmahaExperimentTest,
+       SetReactivationLabelWithExistingEntryWithLabelAsPrefix) {
+  string16 previous_labels(kSomeExperiments);
+  previous_labels.append(google_update::kExperimentLabelSep);
+  // Append prefix matching the label, but not followed by '='.
+  previous_labels.append(gcapi_internals::kReactivationLabel);
+  // Shouldn't match deletion criteria.
+  previous_labels.append(kSomeOtherExperiments);
+  GoogleUpdateSettings::SetExperimentLabels(false, previous_labels);
+
+  ASSERT_TRUE(SetReactivationExperimentLabels(kBrand, kUserLevel));
+
+  string16 expected_labels(kSomeExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(gcapi_internals::kReactivationLabel);
+  expected_labels.append(kSomeOtherExperiments);
+  expected_labels.append(google_update::kExperimentLabelSep);
+  expected_labels.append(reactivation_label_);
+  VerifyExperimentLabels(expected_labels);
+}
+
+}  // namespace
diff --git a/chrome/installer/gcapi/gcapi_reactivation_test.cc b/chrome/installer/gcapi/gcapi_reactivation_test.cc
index 92987a2..fb154e6a 100644
--- a/chrome/installer/gcapi/gcapi_reactivation_test.cc
+++ b/chrome/installer/gcapi/gcapi_reactivation_test.cc
@@ -5,16 +5,13 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/guid.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/strings/stringprintf.h"
-#include "base/strings/utf_string_conversions.h"
-#include "base/test/test_reg_util_win.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "chrome/installer/gcapi/gcapi.h"
 #include "chrome/installer/gcapi/gcapi_omaha_experiment.h"
 #include "chrome/installer/gcapi/gcapi_reactivation.h"
+#include "chrome/installer/gcapi/gcapi_test_registry_overrider.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -24,15 +21,7 @@ using base::win::RegKey;
 
 class GCAPIReactivationTest : public ::testing::Test {
  protected:
-  void SetUp() {
-    // Override keys - this is undone during destruction.
-    std::wstring hkcu_override = base::StringPrintf(
-        L"hkcu_override\\%ls", ASCIIToWide(base::GenerateGUID()));
-    override_manager_.OverrideRegistry(HKEY_CURRENT_USER, hkcu_override);
-    std::wstring hklm_override = base::StringPrintf(
-        L"hklm_override\\%ls", ASCIIToWide(base::GenerateGUID()));
-    override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE, hklm_override);
-  }
+  GCAPIReactivationTest() {}
 
   bool SetChromeInstallMarker(HKEY hive) {
     // Create the client state keys in the right places.
@@ -100,8 +89,7 @@ class GCAPIReactivationTest : public ::testing::Test {
     return L"ERROR";
   }
 
- private:
-  registry_util::RegistryOverrideManager override_manager_;
+  const GCAPITestRegistryOverrider gcapi_test_registry_overrider_;
 };
 
 TEST_F(GCAPIReactivationTest, CheckSetReactivationBrandCode) {
@@ -109,7 +97,6 @@ TEST_F(GCAPIReactivationTest, CheckSetReactivationBrandCode) {
   EXPECT_EQ(L"GAGA", GetReactivationString(HKEY_CURRENT_USER));
 
   EXPECT_TRUE(HasBeenReactivated());
-
 }
 
 TEST_F(GCAPIReactivationTest, CanOfferReactivation_Basic) {
diff --git a/chrome/installer/gcapi/gcapi_test.cc b/chrome/installer/gcapi/gcapi_test.cc
index 68633e0..477b15a 100644
--- a/chrome/installer/gcapi/gcapi_test.cc
+++ b/chrome/installer/gcapi/gcapi_test.cc
@@ -4,6 +4,7 @@
 
 #include <stdio.h>
 
+#include "base/at_exit.h"
 #include "base/command_line.h"
 #include "chrome/installer/gcapi/gcapi.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -60,6 +61,7 @@ void call_dynamically() {
 const char kManualLaunchTests[] = "launch-chrome";
 
 int main(int argc, char* argv[]) {
+  base::AtExitManager exit_manager;
   CommandLine::Init(argc, argv);
 
   testing::InitGoogleTest(&argc, argv);
diff --git a/chrome/installer/gcapi/gcapi_test_registry_overrider.cc b/chrome/installer/gcapi/gcapi_test_registry_overrider.cc
new file mode 100644
index 0000000..2df337f
--- /dev/null
+++ b/chrome/installer/gcapi/gcapi_test_registry_overrider.cc
@@ -0,0 +1,23 @@
+// Copyright 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/installer/gcapi/gcapi_test_registry_overrider.h"
+
+#include "base/guid.h"
+#include "base/strings/string16.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+
+GCAPITestRegistryOverrider::GCAPITestRegistryOverrider() {
+  // Override keys - this is undone during destruction.
+  string16 hkcu_override = base::StringPrintf(
+      L"hkcu_override\\%ls", ASCIIToWide(base::GenerateGUID()));
+  override_manager_.OverrideRegistry(HKEY_CURRENT_USER, hkcu_override);
+  string16 hklm_override = base::StringPrintf(
+      L"hklm_override\\%ls", ASCIIToWide(base::GenerateGUID()));
+  override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE, hklm_override);
+}
+
+GCAPITestRegistryOverrider::~GCAPITestRegistryOverrider() {
+}
diff --git a/chrome/installer/gcapi/gcapi_test_registry_overrider.h b/chrome/installer/gcapi/gcapi_test_registry_overrider.h
new file mode 100644
index 0000000..44dfed0
--- /dev/null
+++ b/chrome/installer/gcapi/gcapi_test_registry_overrider.h
@@ -0,0 +1,23 @@
+// Copyright 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.
+
+#ifndef CHROME_INSTALLER_GCAPI_GCAPI_TEST_REGISTRY_OVERRIDER_H_
+#define CHROME_INSTALLER_GCAPI_GCAPI_TEST_REGISTRY_OVERRIDER_H_
+
+#include "base/test/test_reg_util_win.h"
+
+// Overrides the registry throughout its lifetime; used by GCAPI tests
+// overriding the registry.
+class GCAPITestRegistryOverrider {
+ public:
+  GCAPITestRegistryOverrider();
+  ~GCAPITestRegistryOverrider();
+
+ private:
+  registry_util::RegistryOverrideManager override_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(GCAPITestRegistryOverrider);
+};
+
+#endif  // CHROME_INSTALLER_GCAPI_GCAPI_TEST_REGISTRY_OVERRIDER_H_