diff options
author | grt@chromium.org <grt@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-05 15:23:58 +0000 |
---|---|---|
committer | grt@chromium.org <grt@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-05 15:23:58 +0000 |
commit | ca1abf67762946dca6b8988a16d027b650c9042e (patch) | |
tree | 87880e8e87c7ece51a08649c9c7d14b498335549 | |
parent | 3daf923017ac77b354513b53259e80b4fb2a609d (diff) | |
download | chromium_src-ca1abf67762946dca6b8988a16d027b650c9042e.zip chromium_src-ca1abf67762946dca6b8988a16d027b650c9042e.tar.gz chromium_src-ca1abf67762946dca6b8988a16d027b650c9042e.tar.bz2 |
Control crash reporting and UMA collection in multi-install via a value in the multi-install binaries' ClientState{Medium} key.
BUG=none
TEST=Confirm no substantive change in single-install Chrome. In multi-install, usagestats should be in the ClientState\{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D} and/or ClientStateMedium\{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D} key. Enabling/disabling usage stats for multi-install Chrome (via Under the Hood) should control crash reports and UMA for multi-install Chrome Frame.
Review URL: http://codereview.chromium.org/6936001
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84250 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/installer/setup/install_worker.cc | 60 | ||||
-rw-r--r-- | chrome/installer/setup/install_worker.h | 18 | ||||
-rw-r--r-- | chrome/installer/setup/install_worker_unittest.cc | 78 | ||||
-rw-r--r-- | chrome/installer/util/google_update_settings.cc | 96 | ||||
-rw-r--r-- | chrome/installer/util/installation_state.cc | 37 | ||||
-rw-r--r-- | chrome/installer/util/installation_state.h | 8 |
6 files changed, 261 insertions, 36 deletions
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc index da91368..b14880f 100644 --- a/chrome/installer/setup/install_worker.cc +++ b/chrome/installer/setup/install_worker.cc @@ -248,12 +248,7 @@ void AddProductSpecificWorkItems(const InstallationState& original_state, } } -// Adds work items that make registry adjustments for Google Update. When a -// product is installed (including overinstall), Google Update will write the -// channel ("ap") value into either Chrome or Chrome Frame's ClientState key. -// In the multi-install case, this value is used as the basis upon which the -// package's channel value is built (by adding the ordered list of installed -// products and their options). +// Adds work items that make registry adjustments for Google Update. void AddGoogleUpdateWorkItems(const InstallationState& original_state, const InstallerState& installer_state, WorkItemList* install_list) { @@ -289,10 +284,62 @@ void AddGoogleUpdateWorkItems(const InstallationState& original_state, } } + AddUsageStatsWorkItems(original_state, installer_state, install_list); + // TODO(grt): check for other keys/values we should put in the package's // ClientState and/or Clients key. } +void AddUsageStatsWorkItems(const InstallationState& original_state, + const InstallerState& installer_state, + WorkItemList* install_list) { + DCHECK(installer_state.operation() == InstallerState::MULTI_INSTALL || + installer_state.operation() == InstallerState::MULTI_UPDATE); + + HKEY root_key = installer_state.root_key(); + bool value_found = false; + DWORD usagestats = 0; + const Products& products = installer_state.products(); + + // Search for an existing usagestats value for any product. + for (Products::const_iterator scan = products.begin(), end = products.end(); + !value_found && scan != end; ++scan) { + BrowserDistribution* dist = (*scan)->distribution(); + const ProductState* product_state = + original_state.GetNonVersionedProductState( + installer_state.system_install(), dist->GetType()); + value_found = product_state->GetUsageStats(&usagestats); + } + + // If a value was found, write it in the appropriate location for the + // binaries and remove all values from the products. + if (value_found) { + std::wstring state_key( + installer_state.multi_package_binaries_distribution()->GetStateKey()); + install_list->AddCreateRegKeyWorkItem(root_key, state_key); + install_list->AddSetRegValueWorkItem(root_key, state_key, + google_update::kRegUsageStatsField, + usagestats, false); + + for (Products::const_iterator scan = products.begin(), end = products.end(); + scan != end; ++scan) { + BrowserDistribution* dist = (*scan)->distribution(); + if (installer_state.system_install()) { + install_list->AddDeleteRegValueWorkItem( + root_key, dist->GetStateMediumKey(), + google_update::kRegUsageStatsField); + // Previous versions of Chrome also wrote a value in HKCU even for + // system-level installs, so clean that up. + install_list->AddDeleteRegValueWorkItem( + HKEY_CURRENT_USER, dist->GetStateKey(), + google_update::kRegUsageStatsField); + } + install_list->AddDeleteRegValueWorkItem(root_key, dist->GetStateKey(), + google_update::kRegUsageStatsField); + } + } +} + // This is called when an MSI installation is run. It may be that a user is // attempting to install the MSI on top of a non-MSI managed installation. // If so, try and remove any existing uninstallation shortcuts, as we want the @@ -570,6 +617,7 @@ void AddInstallWorkItems(const InstallationState& original_state, AddProductSpecificWorkItems(original_state, installer_state, setup_path, new_version, install_list); + // Copy over brand, usagestats, and other values. AddGoogleUpdateWorkItems(original_state, installer_state, install_list); AddQuickEnableWorkItems(installer_state, original_state, &setup_path, diff --git a/chrome/installer/setup/install_worker.h b/chrome/installer/setup/install_worker.h index c04433c..7e4e19f 100644 --- a/chrome/installer/setup/install_worker.h +++ b/chrome/installer/setup/install_worker.h @@ -28,16 +28,22 @@ class InstallerState; class Package; class Product; -// Adds work items that make registry adjustments for Google Update. When a -// product is installed (including overinstall), Google Update will write the -// channel ("ap") value into either Chrome or Chrome Frame's ClientState key. -// In the multi-install case, this value is used as the basis upon which the -// package's channel value is built (by adding the ordered list of installed -// products and their options). +// Adds work items that make registry adjustments for Google Update; namely, +// copy a brand value and move a usagestats value. void AddGoogleUpdateWorkItems(const InstallationState& original_state, const InstallerState& installer_state, WorkItemList* install_list); +// Adds work items that make registry adjustments for stats and crash +// collection. When a product is installed, Google Update may write a +// "usagestats" value to Chrome or Chrome Frame's ClientState key. In the +// multi-install case, both products will consult/modify stats for the binaries' +// app guid. Consequently, during install and update we will move a +// product-specific value into the binaries' ClientState key. +void AddUsageStatsWorkItems(const InstallationState& original_state, + const InstallerState& installer_state, + WorkItemList* install_list); + // After a successful copying of all the files, this function is called to // do a few post install tasks: // - Handle the case of in-use-update by updating "opv" (old version) key or diff --git a/chrome/installer/setup/install_worker_unittest.cc b/chrome/installer/setup/install_worker_unittest.cc index 5cdb3d6..4a99174 100644 --- a/chrome/installer/setup/install_worker_unittest.cc +++ b/chrome/installer/setup/install_worker_unittest.cc @@ -50,10 +50,9 @@ class MockWorkItemList : public WorkItemList { MOCK_METHOD1(AddCreateDirWorkItem, WorkItem* (const FilePath&)); MOCK_METHOD2(AddCreateRegKeyWorkItem, WorkItem* (HKEY, const std::wstring&)); MOCK_METHOD2(AddDeleteRegKeyWorkItem, WorkItem* (HKEY, const std::wstring&)); - MOCK_METHOD4(AddDeleteRegValueWorkItem, WorkItem* (HKEY, + MOCK_METHOD3(AddDeleteRegValueWorkItem, WorkItem* (HKEY, const std::wstring&, - const std::wstring&, - bool)); + const std::wstring&)); MOCK_METHOD2(AddDeleteTreeWorkItem, WorkItem* (const FilePath&, const std::vector<FilePath>&)); MOCK_METHOD1(AddDeleteTreeWorkItem, WorkItem* (const FilePath&)); @@ -94,6 +93,11 @@ class MockProductState : public ProductState { void set_version(Version* version) { version_.reset(version); } void set_multi_install(bool multi) { multi_install_ = multi; } void set_brand(const std::wstring& brand) { brand_ = brand; } + void set_usagestats(DWORD usagestats) { + has_usagestats_ = true; + usagestats_ = usagestats; + } + void clear_usagestats() { has_usagestats_ = false; } void SetUninstallProgram(const FilePath& setup_exe) { uninstall_command_ = CommandLine(setup_exe); } @@ -265,7 +269,8 @@ class InstallWorkerTest : public testing::Test { const ProductState* chrome = machine_state.GetProductState(installer_state->system_install(), BrowserDistribution::CHROME_BROWSER); - if (chrome != NULL) { + if (chrome != NULL && + chrome->is_multi_install() == installer_state->is_multi_install()) { installer_state->AddProductFromState(BrowserDistribution::CHROME_BROWSER, *chrome); } else { @@ -561,6 +566,71 @@ TEST_F(InstallWorkerTest, GoogleUpdateWorkItemsTest) { &work_item_list); } +// Test that usagestats values are migrated properly. +TEST_F(InstallWorkerTest, AddUsageStatsWorkItems) { + const bool system_level = true; + const bool multi_install = true; + MockWorkItemList work_item_list; + + scoped_ptr<MockInstallationState> installation_state( + BuildChromeInstallationState(system_level, multi_install)); + + MockProductState chrome_state; + chrome_state.set_version(current_version_->Clone()); + chrome_state.set_multi_install(false); + chrome_state.set_usagestats(1); + + installation_state->SetProductState(system_level, + BrowserDistribution::CHROME_BROWSER, chrome_state); + + scoped_ptr<MockInstallerState> installer_state( + BuildChromeInstallerState(system_level, multi_install, + *installation_state, + InstallerState::MULTI_INSTALL)); + + // Expect the multi Client State key to be created. + BrowserDistribution* multi_dist = + BrowserDistribution::GetSpecificDistribution( + BrowserDistribution::CHROME_BINARIES); + std::wstring multi_app_guid(multi_dist->GetAppGuid()); + EXPECT_CALL(work_item_list, + AddCreateRegKeyWorkItem(_, HasSubstr(multi_app_guid))).Times(1); + + // Expect to see a set value for the usagestats in the multi Client State key. + EXPECT_CALL(work_item_list, + AddSetRegDwordValueWorkItem( + _, + HasSubstr(multi_app_guid), + StrEq(google_update::kRegUsageStatsField), + Eq(static_cast<DWORD>(1)), + _)).Times(1); + + // Expect to see some values cleaned up from Chrome's keys. + BrowserDistribution* chrome_dist = + BrowserDistribution::GetSpecificDistribution( + BrowserDistribution::CHROME_BROWSER); + if (system_level) { + EXPECT_CALL(work_item_list, + AddDeleteRegValueWorkItem( + _, + StrEq(chrome_dist->GetStateMediumKey()), + StrEq(google_update::kRegUsageStatsField))).Times(1); + EXPECT_CALL(work_item_list, + AddDeleteRegValueWorkItem( + Eq(HKEY_CURRENT_USER), + StrEq(chrome_dist->GetStateKey()), + StrEq(google_update::kRegUsageStatsField))).Times(1); + } + EXPECT_CALL(work_item_list, + AddDeleteRegValueWorkItem( + Eq(installer_state->root_key()), + StrEq(chrome_dist->GetStateKey()), + StrEq(google_update::kRegUsageStatsField))).Times(1); + + AddUsageStatsWorkItems(*installation_state.get(), + *installer_state.get(), + &work_item_list);} + // The Quick Enable tests only make sense for the Google Chrome build as it // interacts with registry values that are specific to Google Update. #if defined(GOOGLE_CHROME_BUILD) diff --git a/chrome/installer/util/google_update_settings.cc b/chrome/installer/util/google_update_settings.cc index 2b6b8ea..702eabb 100644 --- a/chrome/installer/util/google_update_settings.cc +++ b/chrome/installer/util/google_update_settings.cc @@ -7,6 +7,7 @@ #include <algorithm> #include "base/command_line.h" +#include "base/path_service.h" #include "base/string_number_conversions.h" #include "base/string_util.h" #include "base/threading/thread_restrictions.h" @@ -123,31 +124,90 @@ bool GetChromeChannelInternal(bool system_install, } // namespace +// Older versions of Chrome unconditionally read from HKCU\...\ClientState\... +// and then HKLM\...\ClientState\.... This means that system-level Chrome +// never checked ClientStateMedium (which has priority according to Google +// Update) and gave preference to a value in HKCU (which was never checked by +// Google Update). From now on, Chrome follows Google Update's policy. bool GoogleUpdateSettings::GetCollectStatsConsent() { + // Determine whether this is a system-level or a user-level install. + bool system_install = false; + FilePath module_dir; + if (!PathService::Get(base::DIR_MODULE, &module_dir)) { + LOG(WARNING) + << "Failed to get directory of module; assuming per-user install."; + } else { + system_install = !InstallUtil::IsPerUserInstall(module_dir.value().c_str()); + } BrowserDistribution* dist = BrowserDistribution::GetDistribution(); - std::wstring reg_path = dist->GetStateKey(); - RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ); - DWORD value = 0; - if (key.ReadValueDW(google_update::kRegUsageStatsField, &value) != - ERROR_SUCCESS) { - key.Open(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ); - key.ReadValueDW(google_update::kRegUsageStatsField, &value); + + // Consent applies to all products in a multi-install package. + if (InstallUtil::IsMultiInstall(dist, system_install)) { + dist = BrowserDistribution::GetSpecificDistribution( + BrowserDistribution::CHROME_BINARIES); } - return (1 == value); + + RegKey key; + DWORD value = 0; + bool have_value = false; + + // For system-level installs, try ClientStateMedium first. + have_value = + system_install && + key.Open(HKEY_LOCAL_MACHINE, dist->GetStateMediumKey().c_str(), + KEY_QUERY_VALUE) == ERROR_SUCCESS && + key.ReadValueDW(google_update::kRegUsageStatsField, + &value) == ERROR_SUCCESS; + + // Otherwise, try ClientState. + have_value = + !have_value && + key.Open(system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, + dist->GetStateKey().c_str(), KEY_QUERY_VALUE) == ERROR_SUCCESS && + key.ReadValueDW(google_update::kRegUsageStatsField, + &value) == ERROR_SUCCESS; + + // Google Update specifically checks that the value is 1, so we do the same. + return have_value && value == 1; } bool GoogleUpdateSettings::SetCollectStatsConsent(bool consented) { - DWORD value = consented? 1 : 0; - // Writing to HKLM is only a best effort deal. + // Google Update writes and expects 1 for true, 0 for false. + DWORD value = consented ? 1 : 0; + + // Determine whether this is a system-level or a user-level install. + bool system_install = false; + FilePath module_dir; + if (!PathService::Get(base::DIR_MODULE, &module_dir)) { + LOG(WARNING) + << "Failed to get directory of module; assuming per-user install."; + } else { + system_install = !InstallUtil::IsPerUserInstall(module_dir.value().c_str()); + } BrowserDistribution* dist = BrowserDistribution::GetDistribution(); - std::wstring reg_path = dist->GetStateMediumKey(); - RegKey key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ | KEY_WRITE); - key.WriteValue(google_update::kRegUsageStatsField, value); - // Writing to HKCU is used both by chrome and by the crash reporter. - reg_path = dist->GetStateKey(); - key.Create(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE); - return (key.WriteValue(google_update::kRegUsageStatsField, value) == - ERROR_SUCCESS); + + // Consent applies to all products in a multi-install package. + if (InstallUtil::IsMultiInstall(dist, system_install)) { + dist = BrowserDistribution::GetSpecificDistribution( + BrowserDistribution::CHROME_BINARIES); + } + + // Write to ClientStateMedium for system-level; ClientState otherwise. + HKEY root_key = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; + std::wstring reg_path = + system_install ? dist->GetStateMediumKey() : dist->GetStateKey(); + RegKey key; + LONG result = key.Create(root_key, reg_path.c_str(), KEY_SET_VALUE); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed opening key " << reg_path << " to set " + << google_update::kRegUsageStatsField << "; result: " << result; + } else { + result = key.WriteValue(google_update::kRegUsageStatsField, value); + LOG_IF(ERROR, result != ERROR_SUCCESS) << "Failed setting " + << google_update::kRegUsageStatsField << " in key " << reg_path + << "; result: " << result; + } + return (result == ERROR_SUCCESS); } bool GoogleUpdateSettings::GetMetricsId(std::wstring* metrics_id) { diff --git a/chrome/installer/util/installation_state.cc b/chrome/installer/util/installation_state.cc index 1ba74a2..ff46253 100644 --- a/chrome/installer/util/installation_state.cc +++ b/chrome/installer/util/installation_state.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -15,8 +15,10 @@ namespace installer { ProductState::ProductState() : uninstall_command_(CommandLine::NO_PROGRAM), + usagestats_(0), msi_(false), - multi_install_(false) { + multi_install_(false), + has_usagestats_(false) { } bool ProductState::Initialize(bool system_install, @@ -90,6 +92,11 @@ bool ProductState::Initialize(bool system_install, InstallUtil::MakeUninstallCommand(setup_path, uninstall_arguments, &uninstall_command_); + // "usagestats" may be absent, 0 (false), or 1 (true). On the chance that + // different values are permitted in the future, we'll simply hold whatever + // we find. + has_usagestats_ = (key.ReadValueDW(google_update::kRegUsageStatsField, + &usagestats_) == ERROR_SUCCESS); // "msi" may be absent, 0 or 1 DWORD dw_value = 0; msi_ = (key.ReadValueDW(google_update::kRegMSIField, @@ -101,6 +108,20 @@ bool ProductState::Initialize(bool system_install, multi_install_ = uninstall_command_.HasSwitch(switches::kMultiInstall); } + // Read from the ClientStateMedium key. + if (system_install && + key.Open(root_key, distribution->GetStateMediumKey().c_str(), + KEY_QUERY_VALUE) == ERROR_SUCCESS) { + DWORD usagestats = 0; + + // A usagestats value in ClientStateMedium overrides that in ClientState. + if (key.ReadValueDW(google_update::kRegUsageStatsField, + &usagestats) == ERROR_SUCCESS) { + has_usagestats_ = true; + usagestats_ = usagestats; + } + } + return version_.get() != NULL; } @@ -122,8 +143,10 @@ ProductState& ProductState::CopyFrom(const ProductState& other) { rename_cmd_ = other.rename_cmd_; uninstall_command_ = other.uninstall_command_; commands_.CopyFrom(other.commands_); + usagestats_ = other.usagestats_; msi_ = other.msi_; multi_install_ = other.multi_install_; + has_usagestats_ = other.has_usagestats_; return *this; } @@ -136,8 +159,18 @@ void ProductState::Clear() { rename_cmd_.clear(); uninstall_command_ = CommandLine(CommandLine::NO_PROGRAM); commands_.Clear(); + usagestats_ = 0; msi_ = false; multi_install_ = false; + has_usagestats_ = false; +} + +bool ProductState::GetUsageStats(DWORD* usagestats) const { + DCHECK(usagestats); + if (!has_usagestats_) + return false; + *usagestats = usagestats_; + return true; } InstallationState::InstallationState() { diff --git a/chrome/installer/util/installation_state.h b/chrome/installer/util/installation_state.h index ed58a8a..1ced729 100644 --- a/chrome/installer/util/installation_state.h +++ b/chrome/installer/util/installation_state.h @@ -65,6 +65,12 @@ class ProductState { // awaiting update; may be empty. const std::wstring& rename_cmd() const { return rename_cmd_; } + // Returns true and populates |usagestats| if the product has such a value; + // otherwise, returns false and does not modify |usagestats|. Expected values + // are 0 (false) and 1 (true), although |usagestats| is given whatever is + // found. + bool GetUsageStats(DWORD* usagestats) const; + // True if the "msi" value in the ClientState key is present and non-zero. bool is_msi() const { return msi_; } @@ -94,8 +100,10 @@ class ProductState { std::wstring rename_cmd_; CommandLine uninstall_command_; AppCommands commands_; + DWORD usagestats_; bool msi_; bool multi_install_; + bool has_usagestats_; private: friend class InstallationState; |