Fix in-use updates for Chrome Frame.

chrome/installer/setup/install_worker.cc

Index: chrome/installer/setup/install_worker.cc
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc
index 7164d5032219a4ce284d8ddacfc94e5e2959f836..3e26b5c1836552a24ca8765d0ae59b55e0d9c2e1 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -606,6 +606,9 @@ bool AppendPostInstallTasks(const InstallerState& installer_state,
       // update upgrade_utils::SwapNewChromeExeIfPresent.
     }
 
+    MaybeAddCopyIELowRightsPolicyWorkItems(installer_state,
+                                           in_use_update_work_items.get());
+
     post_install_task_list->AddWorkItem(in_use_update_work_items.release());
   }
 
@@ -626,6 +629,9 @@ bool AppendPostInstallTasks(const InstallerState& installer_state,
                                             google_update::kRegRenameCmdField);
     }
 
+    MaybeAddDeleteOldIELowRightsPolicyWorkItems(
+        installer_state, regular_update_work_items.get());
+
     post_install_task_list->AddWorkItem(regular_update_work_items.release());
   }
 
@@ -969,62 +975,84 @@ void AddChromeFrameWorkItems(const InstallationState& original_state,
   }
 }
 
-void AddElevationPolicyWorkItems(const InstallationState& original_state,
-                                 const InstallerState& installer_state,
-                                 const Version& new_version,
-                                 WorkItemList* install_list) {
-  if (!installer_state.is_multi_install()) {
-    VLOG(1) << "Not adding elevation policy for single installs";
-    return;
+namespace {
+
+enum ElevationPolicyId {
+  CURRENT_ELEVATION_POLICY,
+  OLD_ELEVATION_POLICY,
+};
+
+// Historically, the UUID of the ChromeFrame class has been used for this value.
+// There is no need for the elevation policy GUID to match the ChromeFrame
+// class's GUID, so I am choosing to hardcode the GUID for the policy here
+// rather than do something like StringFromGUID2(__uuidof(ChromeFrame), ...).

[robertshield, 2011/09/22 20:34:10]

If there is nothing that constrains the GUID from matching the Chrome Frame one,
why not keep it as it was, and reuse the GUID? 

There are some maintenance advantages since code in chrome_tab.cc still refers
to the elevation policy as well.

[grt (UTC plus 2), 2011/09/23 04:16:49]

Because the reader could be confused into thinking that the "old" one won't work
since it doesn't match the class GUID, and because I have to make a function
call to get it.  Maybe those aren't very good reasons, though.
I'm not so sure that the maintenance advantages you wish for are actually there.
 :-)

The references in chrome_tab.cc did not refer to the GUID in any way;
chrome_frame_elevation.rgs hard-coded the GUID.  Had the ChromeFrame class GUID
changed for some reason, DLL registration would have used the old value while
the previous code here that did the StringFromGUID2 thing would have used the
new value.  This would be bad, as they'd be two trains passing in the night.

IMO, having two baked-in constants is a moderate step up from that.  I agree,
however, that that sucks, too.  Thanks for calling me on it.

I've changed it so that both places now derive a string from a constant out of
the .idl file.  Let me know what you think.

[robertshield, 2011/09/23 14:22:18]

SGTM.

+const wchar_t kIELowRightsPolicyGuid[] =

[robertshield, 2011/09/22 20:34:10]

Sometimes we call things ElevationPolicy and others LowRightsPolicy, I think
this CL should standardize on ElevationPolicy, so kIELowRightsPolicyGuid ->
kIEElevationPolicyGuid, etc.

[grt (UTC plus 2), 2011/09/23 04:16:49]

GCF requires some entries in two subkeys of IE's "Low Rights" key.  One of those
is called "ElevationPolicy", and the other is called "DragDrop".  I thought that
referring to these two things as "ElevationPolicy" wasn't ideal, so I started
collectively referring to them as "Low Rights policies".  For the in-use update
case, we only need to copy the ElevationPolicy since the DragDrop entry points
to (unversioned) chrome.

My preference is for the GUIDs to be labeled at the level of "Low Rights"
thingamaboobs.

[robertshield, 2011/09/23 14:22:18]

Ok, I don't feel strongly, as long as you admit that your bikeshed is a lesser
shade of burgundy than mine.

+    L"{E0A900DF-9611-4446-86BD-4B1D47E7DB2A}";
+const wchar_t kIELowRightsPolicyOldGuid[] =
+    L"{6C288DD7-76FB-4721-B628-56FAC252E199}";
+
+const wchar_t kElevationPolicyKeyPath[] =
+    L"SOFTWARE\\Microsoft\\Internet Explorer\\Low Rights\\ElevationPolicy\\";
+
+void GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,

[robertshield, 2011/09/22 20:34:10]

Drop the "LowRights" bit of the name.

[grt (UTC plus 2), 2011/09/23 04:16:49]

This function returns the path to the "ElevationPolicy" subkey of GCF's two "Low
Rights" keys.  I prefer to leave it as-is.

[robertshield, 2011/09/23 14:22:18]

As you will.

+                                          std::wstring* key_path) {
+  DCHECK(policy == CURRENT_ELEVATION_POLICY || policy == OLD_ELEVATION_POLICY);

[robertshield, 2011/09/22 20:34:10]

Do we commonly dcheck that enum values are one of the defined values?

[grt (UTC plus 2), 2011/09/23 04:16:49]

I'm not sure.  I put this here so that if a third one appears in the future (you
know, sent from the past by one of these on a mission to kill us all), the func
will blow up if it hasn't been adjusted accordingly.  Do you think this is being
overly paranoid?

[robertshield, 2011/09/23 14:22:18]

Yes, but just because you're paranoid doesn't mean they aren't out to get you. I
wouldn't say that in general using an enum value as a parameter implies that a
function has to handle ALL values of that enum, but the DCHECK here is fine.

+
+  key_path->assign(kElevationPolicyKeyPath,
+                   arraysize(kElevationPolicyKeyPath) - 1);
+  if (policy == CURRENT_ELEVATION_POLICY) {
+    key_path->append(kIELowRightsPolicyGuid,
+                     arraysize(kIELowRightsPolicyGuid) - 1);
   } else {

[robertshield, 2011/09/23 14:22:18]

perhaps change this to 

} else if (policy == OLD_ELEVATION_POLICY) {

to guard against future malicious enum changes.

[grt (UTC plus 2), 2011/09/23 18:21:07]

My hope was that the DCHECK is sufficient.  How strongly do you feel?

[robertshield, 2011/09/23 18:34:24]

lg.

-    const ProductState* cf_state =
-        original_state.GetProductState(installer_state.system_install(),
-                                       BrowserDistribution::CHROME_FRAME);
-    if (cf_state && !cf_state->is_multi_install()) {
-      LOG(WARNING) << "Not adding elevation policy since a single install "
-                      "of CF exists";
-      return;
-    }
+    key_path->append(kIELowRightsPolicyOldGuid,
+                     arraysize(kIELowRightsPolicyOldGuid)- 1);
   }
+}
 
-  FilePath binary_dir(
-      GetChromeInstallPath(installer_state.system_install(),
-          BrowserDistribution::GetSpecificDistribution(
-              BrowserDistribution::CHROME_BINARIES)));
-
-  struct {
-    const wchar_t* sub_key;
-    const wchar_t* executable;
-    const FilePath exe_dir;
-  } low_rights_entries[] = {
-    { L"ElevationPolicy\\", kChromeLauncherExe,
-      binary_dir.Append(ASCIIToWide(new_version.GetString())) },
-    { L"DragDrop\\", chrome::kBrowserProcessExecutableName, binary_dir },
-  };
-
-  bool uninstall = (installer_state.operation() == InstallerState::UNINSTALL);
-  HKEY root = installer_state.root_key();
-  const wchar_t kLowRightsKeyPath[] =
-      L"SOFTWARE\\Microsoft\\Internet Explorer\\Low Rights\\";
-  std::wstring key_path(kLowRightsKeyPath);
+// Returns true if this installation run owns the old IE low rights policies.
+bool InstallerOwnsOldLowRightsPolicies(const InstallerState& installer_state) {

[robertshield, 2011/09/22 20:34:10]

InstallerOwnsOldLowRightsPolicies -> InstallerOwnsOldElevationPolicies

[robertshield, 2011/09/23 14:22:18]

I take it that you disagree? Fine :-)

+  // The installer owns the old policies only when GCF is being operated on.
+  return installer_state.FindProduct(BrowserDistribution::CHROME_FRAME) != NULL;
+}
 
-  wchar_t cf_classid[64] = {0};
-  StringFromGUID2(__uuidof(ChromeFrame), cf_classid, arraysize(cf_classid));
+}  // namespace
 
-  for (size_t i = 0; i < arraysize(low_rights_entries); ++i) {
-    key_path.append(low_rights_entries[i].sub_key).append(cf_classid);
-    if (uninstall) {
-      install_list->AddDeleteRegKeyWorkItem(root, key_path);
-    } else {
-      install_list->AddCreateRegKeyWorkItem(root, key_path);
-      install_list->AddSetRegValueWorkItem(root, key_path, L"Policy",
-          static_cast<DWORD>(3), true);
-      install_list->AddSetRegValueWorkItem(root, key_path, L"AppName",
-          low_rights_entries[i].executable, true);
-      install_list->AddSetRegValueWorkItem(root, key_path, L"AppPath",
-          low_rights_entries[i].exe_dir.value(), true);
+void MaybeAddDeleteOldIELowRightsPolicyWorkItems(
+    const InstallerState& installer_state,
+    WorkItemList* install_list) {
+  DCHECK(install_list);
+
+  if (InstallerOwnsOldLowRightsPolicies(installer_state)) {
+    std::wstring key_path;
+    GetIELowRightsElevationPolicyKeyPath(OLD_ELEVATION_POLICY, &key_path);
+    install_list->AddDeleteRegKeyWorkItem(installer_state.root_key(), key_path);
+  }
+}
+
+// Conditionally adds work items to copy the chrome_launcher IE low rights
+// elevation policy from the primary policy GUID to the "old" policy GUID.  Take
+// care not to perform the copy if there is already an old policy present, as
+// the ones under the main kElevationPolicyGuid would then correspond to an
+// intermediate version (current_version < pv < new_version).
+void MaybeAddCopyIELowRightsPolicyWorkItems(

[robertshield, 2011/09/22 20:34:10]

what about instead of having these MaybeFoo methods, we dropped the "Maybe" part
and called them from AddChromeFrameWorkItems above? That would let us get rid of
the InstallerOwnsOldLowRightsPolicies function.

[grt (UTC plus 2), 2011/09/23 04:16:49]

The Copy is only called for an in-use update, while the delete is called during
a non-in-use update (to clean up a stale value).  I think moving them into
AddCFWI would mean duplicating a bunch of code to add the conditional work item
lists corresponding to the two cases there.  Do you see a better way?

[robertshield, 2011/09/23 14:22:18]

How about just wrapping the existing call sites in 
if (installer_state.chrome_frame()) ?

It keeps policy decisions about whether to have these methods add workitems in
one place which makes the code easier to read imo. 

[grt (UTC plus 2), 2011/09/23 18:21:07]

Done.

+    const InstallerState& installer_state,
+    WorkItemList* install_list) {
+  DCHECK(install_list);
+
+  // Proceed if we own the old policies and if old policies are not already
+  // present (we don't want to clobber them).
+  if (InstallerOwnsOldLowRightsPolicies(installer_state)) {
+    const HKEY root = installer_state.root_key();
+    std::wstring old_key_path;
+
+    GetIELowRightsElevationPolicyKeyPath(OLD_ELEVATION_POLICY, &old_key_path);
+    if (!RegKey(root, old_key_path.c_str(), KEY_QUERY_VALUE).Valid()) {
+      std::wstring current_key_path;
+
+      GetIELowRightsElevationPolicyKeyPath(CURRENT_ELEVATION_POLICY,
+                                           &current_key_path);
+      install_list->AddCopyRegKeyWorkItem(root, current_key_path, old_key_path);
     }
-    key_path.resize(arraysize(kLowRightsKeyPath) - 1);
   }
 }