Force restoration of Chrome's shortcuts when Active Setup kicks in in response to a major OS upgrad…

chrome/installer/util/install_util.cc

Index: chrome/installer/util/install_util.cc
diff --git a/chrome/installer/util/install_util.cc b/chrome/installer/util/install_util.cc
index f7dccea263c8bc2f0c43b040dd191b70324d294e..32a660e1727a5086a777c5dd32ff33a23190b191 100644
--- a/chrome/installer/util/install_util.cc
+++ b/chrome/installer/util/install_util.cc
@@ -19,6 +19,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
@@ -26,14 +28,17 @@
 #include "base/version.h"
 #include "base/win/metro.h"
 #include "base/win/registry.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/installer/util/app_registration_data.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/l10n_string_util.h"
+#include "chrome/installer/util/update_active_setup_version_work_item.h"
 #include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item_list.h"
 
@@ -162,6 +167,90 @@ void InstallUtil::TriggerActiveSetupCommand() {
     PLOG(ERROR) << cmd.GetCommandLineString();
 }
 
+bool InstallUtil::UpdateLastOSUpgradeHandledByActiveSetup(

[gab, 2015/07/09 20:54:44]

@grt: This duplicates a bit of logic from a few places, but I couldn't quite see
where to put it to justify re-use, I think in this special case a little bit of
duplication saves a lot of complexity.

Duplication is pretty much:
 - Usage of GetStateMediumKey() and GetUserSidString() a la
google_update_settings.cc and beacons.cc
 - Knowledge of the "Version" entry of Active Setup a la
update_active_setup_version_work_item.cc

Let me know if you have a better idea.

+    BrowserDistribution* dist) {
+  static const wchar_t kLastOSUpgradeHandledRegName[] = L"LastOSUpgradeHandled";
+
+  // FIRST: Find out the value of the last OS upgrade handled, defaults to 0 if
+  // none was ever handled.
+  DWORD last_os_upgrade_handled = 0;
+
+  base::string16 last_upgrade_handled_key_path =
+      dist->GetAppRegistrationData().GetStateMediumKey();
+  last_upgrade_handled_key_path.push_back(L'\\');
+  last_upgrade_handled_key_path.append(kLastOSUpgradeHandledRegName);
+
+  base::string16 user_specific_value;
+  // This should never fail. If it does, the beacon will be written in the key's
+  // default value, which is okay since the majority case is likely a machine
+  // with a single user.
+  if (!base::win::GetUserSidString(&user_specific_value))
+    NOTREACHED();
+
+  base::win::RegKey last_upgrade_key;
+  if (last_upgrade_key.Open(
+          HKEY_LOCAL_MACHINE, last_upgrade_handled_key_path.c_str(),
+          KEY_WOW64_32KEY | KEY_QUERY_VALUE) == ERROR_SUCCESS) {
+    last_upgrade_key.ReadValueDW(user_specific_value.c_str(),
+                                 &last_os_upgrade_handled);
+  }
+
+  // SECOND: Find out the value of the latest OS upgrade registered in the
+  // Active Setup version (bumped on every major OS upgrade), defaults to 0 if
+  // no OS upgrade was ever encountered by this install.
+  DWORD latest_os_upgrade = 0;

[grt (UTC plus 2), 2015/07/10 18:15:24]

does this method always return false without updating the registry if
latest_os_upgrade == 0? if so, it looks like you could get rid of a potential
failure mode by doing this read first, and then Create last_upgrade_key for what
is currently part one (but would become part two) with KEY_WOW64_32KEY |
KEY_QUERY_VALUE | KEY_SET_VALUE. this way you never fail down in part four as a
result of not being able to create the key with write perms.

[gab, 2015/07/13 18:19:01]

Tweaked the order and made sure it only ever returns true if it successfully
saved its state (i.e. make the writes an explicit failure rather than a best
effort). In case of failure this will make sure the OS upgrade is never handled
more than once, though it may still be handled at a much later time, when it
eventually succeeds in an infrequent and potentially unrelated follow-up Active
Setup run.

Doing it as a best effort had the slight advantage of doing it when its right on
top of doing it again later when it will appear random (we can't avoid doing it
later if the initial write fails; unless we write a timestamp on-os-upgrade and
never do it X days after the OS upgrade, but this is more complexity for a
theoretical use case).

Since doing it later will look like a bug regardless and doing it initially is
always right, shall we go back to best-effort write?

+
+  const base::string16 active_setup_key_path(GetActiveSetupPath(dist));
+
+  base::win::RegKey active_setup_key;
+  if (active_setup_key.Open(HKEY_LOCAL_MACHINE, active_setup_key_path.c_str(),
+                            KEY_QUERY_VALUE) == ERROR_SUCCESS) {
+    base::string16 existing_version;
+    if (active_setup_key.ReadValue(L"Version",
+                                   &existing_version) == ERROR_SUCCESS) {
+      std::vector<base::string16> version_components =
+          base::SplitString(existing_version, L",", base::TRIM_WHITESPACE,
+                            base::SPLIT_WANT_NONEMPTY);
+      unsigned latest_os_upgrade_uint = 0;
+      if (version_components.size() == 4U &&
+          base::StringToUint(
+              version_components[UpdateActiveSetupVersionWorkItem::
+                                     VersionComponent::OS_UPGRADES],
+              &latest_os_upgrade_uint)) {
+        latest_os_upgrade = static_cast<DWORD>(latest_os_upgrade_uint);
+      } else {
+        LOG(ERROR) << "Failed to parse OS_UPGRADES component of "
+                   << existing_version;
+      }
+    }
+  }
+
+  // THIRD: Figure out whether the latest OS upgrade has been handled already.
+
+  if (last_os_upgrade_handled >= latest_os_upgrade) {
+    LOG_IF(ERROR, last_os_upgrade_handled > latest_os_upgrade)
+        << "Last OS upgrade handled is somehow ahead of the latest OS upgrade?";
+    VLOG_IF(1, last_os_upgrade_handled == latest_os_upgrade)
+        << "Latest OS upgrade already handled.";
+    return false;
+  }
+
+  // At this point |last_os_upgrade_handled < latest_os_upgrade| so,
+  // FOURTH: store the fact that the latest OS upgrade has been handled and
+  // return true for the caller to act accordingly.
+
+  if (last_upgrade_key.Create(
+          HKEY_LOCAL_MACHINE, last_upgrade_handled_key_path.c_str(),
+          KEY_WOW64_32KEY | KEY_SET_VALUE) != ERROR_SUCCESS ||
+      last_upgrade_key.WriteValue(user_specific_value.c_str(),
+                                  latest_os_upgrade) != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed to save latest_os_upgrade value ("
+               << latest_os_upgrade << ") to " << last_upgrade_handled_key_path;
+  }
+
+  return true;
+}
+
 bool InstallUtil::ExecuteExeAsAdmin(const base::CommandLine& cmd,
                                     DWORD* exit_code) {
   base::FilePath::StringType program(cmd.GetProgram().value());