Fix multi-install update regression.

chrome/installer/setup/setup_main.cc

 // Copyright (c) 2012 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 <windows.h>
 #include <msi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 
 #include "base/at_exit.h"
@@ skipping 199 matching lines @@
   // Delete an elevation policy associated with the old version, should one
   // exist.
   if (installer_state->FindProduct(BrowserDistribution::CHROME_FRAME)) {
     installer::AddDeleteOldIELowRightsPolicyWorkItems(*installer_state,
                                                       install_list.get());
   }
   // old_chrome.exe is still in use in most cases, so ignore failures here.
   install_list->AddDeleteTreeWorkItem(chrome_old_exe, temp_path.path())
       ->set_ignore_failure(true);
 
-  // Collect the set of distributions we need to update, which is the
-  // multi-install binaries (if this is a multi-install operation) and all
-  // products we're operating on.
+  // Collect the set of distributions we need to update, which is all
+  // products we're operating on (this includes the multi-install binaries).
   BrowserDistribution* dists[BrowserDistribution::NUM_TYPES];
   int num_dists = 0;
-  // First, add the multi-install binaries, if relevant.
-  if (installer_state->is_multi_install())
-    dists[num_dists++] = installer_state->multi_package_binaries_distribution();
-  // Next, add all products we're operating on.  std::transform can handily do
-  // this for us, but this is discouraged as being too tricky.
+  // std::transform can handily do this for us, but this is discouraged as
+  // being too tricky.

[robertshield, 2012/07/24 17:16:50]

Grumbling noted, but the above comment doesn't make this code clearer. Please
remove.

[grt (UTC plus 2), 2012/07/24 20:31:29]

I briefly changed the comment to "I hate robertshields's aversion to concise
code", but then removed it.  Done.

[robertshield, 2012/07/24 21:03:14]

concise != readable

[grt (UTC plus 2), 2012/07/24 21:06:00]

LOL, BFF

   const Products& products = installer_state->products();
-  for (Products::size_type i = 0; i < products.size(); ++i) {
+  CHECK_LE(products.size(), arraysize(dists));
+  for (size_t i = 0; i < products.size(); ++i)
     dists[num_dists++] = products[i]->distribution();

[robertshield, 2012/07/24 17:16:50]

nit: It seems to me that num_dists could be done away with altogether, in this
loop i could be used and the other usage could be replaced with products.size().

[grt (UTC plus 2), 2012/07/24 20:31:29]

What?!??  There's a forest around these trees?   Done, and then some.

-  }
 
   // Add work items to delete the "opv", "cpv", and "cmd" values from all
   // distributions.
   HKEY reg_root = installer_state->root_key();
   string16 version_key;
   for (int i = 0; i < num_dists; ++i) {
     version_key = dists[i]->GetVersionKey();
     install_list->AddDeleteRegValueWorkItem(
         reg_root, version_key, google_update::kRegOldVersionField);
     install_list->AddDeleteRegValueWorkItem(
@@ skipping 128 matching lines @@
     const ProductState* chrome_state =
         original_state.GetProductState(system_level,
                                        BrowserDistribution::CHROME_BROWSER);
 
     if (!binaries) {
       if (app_host && !chrome && !chrome_frame && !cf_state && !chrome_state) {
         DCHECK(!system_level);
         // App Host may use Chrome/Chrome binaries at system-level.
         if (original_state.GetProductState(
                 true,  // system
                 BrowserDistribution::CHROME_BROWSER) ||

[robertshield, 2012/07/24 17:16:50]

Shouldn't this only be checking for the "Chrome binaries" bit or are we okay
with the App Host working with single-install Chrome? Wouldn't that be prone to
breakage if the user uninstalled single-install Chrome?

[grt (UTC plus 2), 2012/07/24 20:31:29]

I discussed this with Erik, and I believe that this is correct: the app host at
user level is compatible with a single-install or a multi-install Chrome at
system-level.  Issues around the system-level Chrome/binaries being uninstalled
are orthogonal to whether it's single-install or multi-install.

             original_state.GetProductState(
                 true,  // system
                 BrowserDistribution::CHROME_BINARIES)) {
+          VLOG(1) << "Installing/updating Application Host without binaries.";
           return true;
         } else {
+          // Somehow the binaries were present when the quick-enable app host
+          // command was run, but now they appear to be missing.
+          // TODO(erikwright): should the binaries be implicitly added?
+          LOG(ERROR) << "Cannot install Application Host without binaries.";
+          *status = installer::APP_HOST_REQUIRES_BINARIES;
+          installer_state->WriteInstallerResult(*status, 0, NULL);
           return false;
         }
       } else {
         // Every other scenario requires the binaries to be installed/updated
-        // simultaneous to the main product.
-        return false;
+        // simultaneous to the main product.  This will only be hit if
+        // --multi-install is given with no products.  see

[robertshield, 2012/07/24 17:16:50]

nit: simultaneous -> simultaneously, see -> See

[grt (UTC plus 2), 2012/07/24 20:31:29]

Done.

+        // CheckPreInstallConditions for handling of this case.
+        return true;
       }
     }
 
     if (chrome) {
       if (chrome_frame &&
           chrome_frame->HasOption(installer::kOptionReadyMode)) {
         // We're being asked to install Chrome with Chrome Frame in ready-mode.
         // This is an optimistic operation: if a SxS install of Chrome Frame
         // is already present, don't touch it; if a multi-install of Chrome
         // Frame is present, preserve its settings (ready-mode).
         if (cf_state != NULL) {
           installer_state->RemoveProduct(chrome_frame);
           chrome_frame = NULL;
           if (cf_state->is_multi_install()) {
             chrome_frame = installer_state->AddProductFromState(
                 BrowserDistribution::CHROME_FRAME, *cf_state);
             VLOG(1) << "Upgrading existing multi-install Chrome Frame rather "
                        "than installing in ready-mode.";
           } else {
             VLOG(1) << "Skipping upgrade of single-install Chrome Frame rather "
                        "than installing in ready-mode.";
           }
         } else {
           VLOG(1) << "Performing initial install of Chrome Frame ready-mode.";
         }
       }
-    } else if (chrome_state != NULL) {
-      // Chrome Frame is being installed in multi-install mode, and Chrome is
-      // already present. Add Chrome to the set of products (making it
-      // multi-install in the process) so that it is updated, too.
+    } else if (chrome_state) {
+      // Chrome Frame or the Application Host is being installed in

[robertshield, 2012/07/24 17:16:50]

"Chrome Frame or the Application Host" -> "A product other than Chrome"

[grt (UTC plus 2), 2012/07/24 20:31:29]

Done.

+      // multi-install mode, and Chrome is already present. Add Chrome to the
+      // set of products (making it multi-install in the process) so that it is
+      // updated, too.
       scoped_ptr<Product> multi_chrome(new Product(
           BrowserDistribution::GetSpecificDistribution(
               BrowserDistribution::CHROME_BROWSER)));
       multi_chrome->SetOption(installer::kOptionMultiInstall, true);
       chrome = installer_state->AddProduct(&multi_chrome);
       VLOG(1) << "Upgrading existing Chrome browser in multi-install mode.";
     } else if (chrome_frame &&
                chrome_frame->HasOption(installer::kOptionReadyMode)) {
       // Chrome Frame with ready-mode is to be installed, yet Chrome is
       // neither installed nor being installed.  Fail.
       LOG(ERROR) << "Cannot install Chrome Frame in ready mode without Chrome.";
       *status = installer::READY_MODE_REQUIRES_CHROME;
       installer_state->WriteInstallerResult(
           *status, IDS_INSTALL_READY_MODE_REQUIRES_CHROME_BASE, NULL);
       return false;
     }
 
     // Fail if we're installing Chrome Frame when a single-install of it is
     // already installed.
-    // TODO(grt): Add support for migration of Chrome Frame from single- to

[grt (UTC plus 2), 2012/07/24 15:02:03]

we've decided not to do this, i believe.

-    // multi-install.
     if (chrome_frame && cf_state && !cf_state->is_multi_install()) {
       LOG(ERROR) << "Cannot migrate existing Chrome Frame installation to "
                  << "multi-install.";
       *status = installer::NON_MULTI_INSTALLATION_EXISTS;
       installer_state->WriteInstallerResult(*status,
           IDS_INSTALL_NON_MULTI_INSTALLATION_EXISTS_BASE, NULL);
       return false;
     }
   } else {
     // This is a non-multi installation.
 
     // It isn't possible to stuff two products into a single-install
     // InstallerState.  Abort the process here in debug builds just in case
     // someone finds a way.
     DCHECK_EQ(1U, products.size());
-    if (products.size() != 1)

[grt (UTC plus 2), 2012/07/24 15:02:03]

erik added this, but i don't think it's necessary.  InstallerState simply won't
allow it to happen.  the dcheck is over-paranoia in case someone someday changes
InstallerState.

[robertshield, 2012/07/24 17:16:50]

I believe you, but that isn't obvious from reading the code even with the
comment.

I'm generally supportive of the pattern of having a DCHECK for an invalid state
paired with sane release mode handling of that invalid state since humans almost
never run Debug builds.

IMHO if we remove the release mode check, we should remove the DCHECK. Or keep
both.

[grt (UTC plus 2), 2012/07/24 20:31:29]

What part of "It isn't possible" do you not understand.  ;-)
I'll remove it since it'll never happen.

[robertshield, 2012/07/24 21:03:14]

I typically come across comments stating that "X is not possible" while
debugging a crash caused by X happening :-)

[grt (UTC plus 2), 2012/07/24 21:06:00]

remind me to work on shutdown next.

-      return false;
 
     // Check for an existing installation of the product.
     const ProductState* product_state = original_state.GetProductState(
         system_level, products[0]->distribution()->GetType());
     if (product_state != NULL) {
       // Block downgrades from multi-install to single-install.
       if (product_state->is_multi_install()) {
         LOG(ERROR) << "Multi-install "
                    << products[0]->distribution()->GetAppShortCutName()
                    << " exists; aborting single install.";
         *status = installer::MULTI_INSTALLATION_EXISTS;
         installer_state->WriteInstallerResult(*status,
             IDS_INSTALL_MULTI_INSTALLATION_EXISTS_BASE, NULL);
         return false;
       }
     }
+  }
+
+  return true;
+}
+
+// Checks app host pre-install conditions, specifically that this is a
+// user-level multi-install.  When the pre-install conditions are not
+// satisfied, the result is written to the registry (via WriteInstallerResult),
+// |status| is set appropriately, and false is returned.
+bool CheckAppHostPreconditions(const InstallationState& original_state,
+                               InstallerState* installer_state,
+                               installer::InstallStatus* status) {
+  if (installer_state->FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
+
+    if (!installer_state->is_multi_install()) {
+      LOG(DFATAL) << "Application Host requires multi install";

[grt (UTC plus 2), 2012/07/24 15:02:03]

this means we're invoking ourselves wrong, which is a pretty bad failure.  it's
nothing that a user can do anything about, hence the lack of a ui string.

+      *status = installer::APP_HOST_REQUIRES_MULTI_INSTALL;
+      // No message string since there is nothing a user can do.
+      installer_state->WriteInstallerResult(*status, 0, NULL);
+      return false;
+    }
+
+    if (installer_state->system_install()) {
+      LOG(DFATAL) << "Application Host may only be installed at user-level.";
+      *status = installer::APP_HOST_REQUIRES_USER_LEVEL;
+      // No message string since there is nothing a user can do.
+      installer_state->WriteInstallerResult(*status, 0, NULL);
+      return false;
+    }
 
   }
 
   return true;
-}
-
-bool CheckAppHostPreconditions(const InstallationState& original_state,
-                               InstallerState* installer_state) {
-  if (!installer_state->FindProduct(BrowserDistribution::CHROME_APP_HOST))
-    return true;
-
-  if (!installer_state->is_multi_install()) {
-    VLOG(1) << "Application Host may only be installed in multi-install mode.";
-    return false;
-  }
-
-  if (installer_state->system_install()) {
-    VLOG(1) << "Application Host may only be installed at user-level.";
-    return false;
-  }
-
-  return true;
 }
 
 // Checks for compatibility between the current state of the system and the
 // desired operation.  Also applies policy that mutates the desired operation;
 // specifically, the |installer_state| object.
 // Also blocks simultaneous user-level and system-level installs.  In the case
 // of trying to install user-level Chrome when system-level exists, the
 // existing system-level Chrome is launched.
 // When the pre-install conditions are not satisfied, the result is written to
 // the registry (via WriteInstallerResult), |status| is set appropriately, and
 // false is returned.
 bool CheckPreInstallConditions(const InstallationState& original_state,
                                InstallerState* installer_state,
                                installer::InstallStatus* status) {
-  if (!CheckAppHostPreconditions(original_state, installer_state))
+  if (!CheckAppHostPreconditions(original_state, installer_state, status)) {
+    DCHECK_NE(*status, installer::UNKNOWN_STATUS);
     return false;
+  }
 
-  if (!CheckMultiInstallConditions(original_state, installer_state, status))
+  // See what products are already installed in multi mode.  When we do multi
+  // installs, we must upgrade all installations since they share the binaries.
+  AddExistingMultiInstalls(original_state, installer_state);

[robertshield, 2012/07/24 17:16:50]

Out of curiosity, how did this work before with CheckMultiInstallConditions
being called BEFORE AddExistingMultiInstalls?

[grt (UTC plus 2), 2012/07/24 20:31:29]

It didn't -- this was the change that broke dev channel.  Prior to r147696,
AddExisting was called before CheckMulti.  r147696 flipped the order.  This
happened while I was away, so I'm not sure why they were switched.  The world
was broken since our update scenario (nothing but --multi-install on the command
line to update all products) depends on existing products being added based on
their presence on the system rather than on the command line.

+
+  if (!CheckMultiInstallConditions(original_state, installer_state, status)) {
+    DCHECK_NE(*status, installer::UNKNOWN_STATUS);
     return false;
+  }
 
   const Products& products = installer_state->products();
   if (products.empty()) {
     // We haven't been given any products on which to operate.
     LOG(ERROR)
         << "Not given any products to install and no products found to update.";
     *status = installer::CHROME_NOT_INSTALLED;
     installer_state->WriteInstallerResult(*status,
         IDS_INSTALL_NO_PRODUCTS_TO_UPDATE_BASE, NULL);
     return false;
   }
 
-  // See what products are already installed in multi mode.  When we do multi
-  // installs, we must upgrade all installations since they share the binaries.
-  AddExistingMultiInstalls(original_state, installer_state);
-
   if (!installer_state->system_install()) {
     // This is a user-level installation. Make sure that we are not installing
     // on top of an existing system-level installation.
     for (size_t i = 0; i < products.size(); ++i) {
       const Product* product = products[i];
       BrowserDistribution* browser_dist = product->distribution();
+
+      // Skip over the binaries, as it's okay for them to be at both levels
+      // for different products.
+      if (browser_dist->GetType() == BrowserDistribution::CHROME_BINARIES)
+        continue;
+
       const ProductState* user_level_product_state =
           original_state.GetProductState(false, browser_dist->GetType());
       const ProductState* system_level_product_state =
           original_state.GetProductState(true, browser_dist->GetType());
 
       // Allow upgrades to proceed so that out-of-date versions are not left
       // around.
       if (user_level_product_state)
         continue;
 
@@ skipping 315 matching lines @@
   installer_state->UpdateStage(installer::PRECONDITIONS);
   // The stage provides more fine-grained information than -multifail, so remove
   // the -multifail suffix from the Google Update "ap" value.
   BrowserDistribution::GetSpecificDistribution(installer_state->state_type())
       ->UpdateInstallStatus(system_install, archive_type, install_status);
   if (CheckPreInstallConditions(original_state, installer_state,
                                 &install_status)) {
     VLOG(1) << "Installing to " << installer_state->target_path().value();
     install_status = InstallProductsHelper(
         original_state, cmd_line, prefs, *installer_state, &archive_type);
+  } else {
+    // CheckPreInstallConditions must set the status on failure.
+    DCHECK_NE(install_status, installer::UNKNOWN_STATUS);
   }
 
   const Products& products = installer_state->products();
 
   for (size_t i = 0; i < products.size(); ++i) {
     const Product* product = products[i];
     product->distribution()->UpdateInstallStatus(
         system_install, archive_type, install_status);
   }
-  if (installer_state->is_multi_install()) {

[grt (UTC plus 2), 2012/07/24 15:02:03]

this isn't needed anymore

-    installer_state->multi_package_binaries_distribution()->UpdateInstallStatus(
-        system_install, archive_type, install_status);
-  }
 
   installer_state->UpdateStage(installer::NO_STAGE);
   return install_status;
 }
 
 installer::InstallStatus UninstallProduct(
     const InstallationState& original_state,
     const InstallerState& installer_state,
     const CommandLine& cmd_line,
     bool remove_all,
@@ skipping 558 matching lines @@
   const Product* cf_install =
       installer_state.FindProduct(BrowserDistribution::CHROME_FRAME);
 
   if (cf_install &&
       !cmd_line.HasSwitch(installer::switches::kForceUninstall)) {
     if (install_status == installer::UNINSTALL_REQUIRES_REBOOT) {
       ShowRebootDialog();
     } else if (is_uninstall) {
       // Only show the message box if Chrome Frame was the only product being
       // uninstalled.
-      if (installer_state.products().size() == 1U) {
+      const Products& products = installer_state.products();
+      int num_products = 0;
+      for (size_t i = 0; i < products.size(); ++i) {

[grt (UTC plus 2), 2012/07/24 15:02:03]

this counting is needed now since the binaries may be in the set of products.

+        if (!products[i]->is_chrome_binaries())
+          ++num_products;
+      }
+      if (num_products == 1U) {
         ::MessageBoxW(NULL,
                       installer::GetLocalizedString(
                           IDS_UNINSTALL_COMPLETE_BASE).c_str(),
                       cf_install->distribution()->GetAppShortCutName().c_str(),
                       MB_OK);
       }
     }
   }
 
   int return_code = 0;
   // MSI demands that custom actions always return 0 (ERROR_SUCCESS) or it will
   // rollback the action. If we're uninstalling we want to avoid this, so always
   // report success, squashing any more informative return codes.
   if (!(installer_state.is_msi() && is_uninstall))
     // Note that we allow the status installer::UNINSTALL_REQUIRES_REBOOT
     // to pass through, since this is only returned on uninstall which is
     // never invoked directly by Google Update.
     return_code = InstallUtil::GetInstallReturnCode(install_status);
 
   VLOG(1) << "Installation complete, returning: " << return_code;
 
   return return_code;
 }