Fix multi-install update regression.

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
index eb59d175a630b57a8153b5bbdcb399353f9375f4..82582f11a73aaa6188fb0dfb72e48a0484f37f64 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -217,20 +217,16 @@ installer::InstallStatus RenameChromeExecutables(
   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.
@@ -383,14 +379,23 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
             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;
       }
     }
 
@@ -417,10 +422,11 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
           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)));
@@ -440,8 +446,6 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
 
     // 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.";
@@ -457,8 +461,6 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
     // 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(
@@ -475,25 +477,36 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
         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) {
-  if (!installer_state->FindProduct(BrowserDistribution::CHROME_APP_HOST))
-    return true;
+                               InstallerState* installer_state,
+                               installer::InstallStatus* status) {
+  if (installer_state->FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
 
-  if (!installer_state->is_multi_install()) {
-    VLOG(1) << "Application Host may only be installed in multi-install mode.";
-    return false;
-  }
+    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;
+    }
 
-  if (installer_state->system_install()) {
-    VLOG(1) << "Application Host may only be installed at user-level.";
-    return false;
   }
 
   return true;
@@ -511,11 +524,19 @@ bool CheckAppHostPreconditions(const InstallationState& original_state,
 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;
+  }
+
+  // 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))
+  if (!CheckMultiInstallConditions(original_state, installer_state, status)) {
+    DCHECK_NE(*status, installer::UNKNOWN_STATUS);
     return false;
+  }
 
   const Products& products = installer_state->products();
   if (products.empty()) {
@@ -528,16 +549,18 @@ bool CheckPreInstallConditions(const InstallationState& original_state,
     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 =
@@ -873,6 +896,9 @@ installer::InstallStatus InstallProducts(
     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();
@@ -882,10 +908,6 @@ installer::InstallStatus InstallProducts(
     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;
@@ -1464,7 +1486,13 @@ int WINAPI wWinMain(HINSTANCE instance, HINSTANCE prev_instance,
     } 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(),