Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10806086:
Fix multi-install update regression. (Closed)
|
Created:
8 years, 5 months ago by grt (UTC plus 2) Modified:
8 years, 5 months ago CC:
chromium-reviews, grt+watch_chromium.org, erikwright (departed), gab Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix multi-install update regression.
- adjusted some code to handle the fact that the binaries are in the set of products now
- added new process exit codes for some situations that should never happen (only if we mess up invoking setup.exe, nothing for a user to do, so no new strings)
BUG=138658
TEST=install beta channel chrome then update with only "--multi-install --do-not-launch-chrome --verbose-logging" on the command line. also test single to multi migration of chrome, multi-installs with chrome frame, etc.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148261
Patch Set 1 #
Total comments: 32
Patch Set 2 : addressed review comments #
Messages
Total messages: 10 (0 generated)
please see my own comments to explain the changes. thanks. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (left): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:651: if (current_version != NULL && installer_state.is_multi_install()) { the binaries are in the set of products now, so this extra handling is no longer needed. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:724: if (installer_state.is_multi_install()) { the binaries are in the set of products now, so this extra handling is no longer needed. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:385: if (products[i]->is_chrome_binaries()) values are copied from products into the binaries, so we skip over the binaries here. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (left): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/setup_main.cc:443: // TODO(grt): Add support for migration of Chrome Frame from single- to we've decided not to do this, i believe. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/setup_main.cc:460: if (products.size() != 1) 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. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/setup_main.cc:885: if (installer_state->is_multi_install()) { this isn't needed anymore https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/setup_main.cc:495: LOG(DFATAL) << "Application Host requires multi install"; 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. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1491: for (size_t i = 0; i < products.size(); ++i) { this counting is needed now since the binaries may be in the set of products. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... File chrome/installer/util/chrome_binaries_operations.cc (right): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... chrome/installer/util/chrome_binaries_operations.cc:22: options->insert(kOptionMultiInstall); the binaries are inherently multi-install, so it's wrong to check the prefs. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... chrome/installer/util/chrome_binaries_operations.cc:29: options->insert(kOptionMultiInstall); the binaries are inherently multi-install, so it's wrong to check the cmd line, which won't contain any flags for an update from an install that predates this change. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... File chrome/installer/util/util_constants.h (left): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... chrome/installer/util/util_constants.h:73: REQUIRES_MULTI_INSTALL, // 41. --multi-install was missing from the this value was never used (really), so i'm repurposing it here.
Drive-by comment. https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... File chrome/installer/util/util_constants.h (right): https://chromiumcodereview.appspot.com/10806086/diff/1/chrome/installer/util/... chrome/installer/util/util_constants.h:85: COMPILE_ASSERT(installer::INCONSISTENT_UPDATE_POLICY == 43, Change this to COMPILE_ASSERT(installer::APP_HOST_REQUIRES_BINARIES == 45, ...); I also feel the comment on this assert is unclear (i.e. what's the "contract" between the 2 setups?)... I feel it's more: "The order of the enum above cannot change. All new statuses should be added at the end."
One functional note in setup_main.cc:378. Otherwise LGTM w/ nits. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (left): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:460: if (products.size() != 1) On 2012/07/24 15:02:03, grt wrote: > 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. 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. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:225: // being too tricky. Grumbling noted, but the above comment doesn't make this code clearer. Please remove. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:229: dists[num_dists++] = products[i]->distribution(); 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(). http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:378: BrowserDistribution::CHROME_BROWSER) || 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? http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:396: // --multi-install is given with no products. see nit: simultaneous -> simultaneously, see -> See http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:426: // Chrome Frame or the Application Host is being installed in "Chrome Frame or the Application Host" -> "A product other than Chrome" http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:534: AddExistingMultiInstalls(original_state, installer_state); Out of curiosity, how did this work before with CheckMultiInstallConditions being called BEFORE AddExistingMultiInstalls?
Thanks for the comments, guys. I'll send this to the CQ shortly unless one or both of you spots a new flaw I've added. :-) http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (left): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:460: if (products.size() != 1) On 2012/07/24 17:16:50, robertshield wrote: > On 2012/07/24 15:02:03, grt wrote: > > 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. > > I believe you, but that isn't obvious from reading the code even with the > comment. What part of "It isn't possible" do you not understand. ;-) > 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. I'll remove it since it'll never happen. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:225: // being too tricky. On 2012/07/24 17:16:50, robertshield wrote: > Grumbling noted, but the above comment doesn't make this code clearer. Please > remove. I briefly changed the comment to "I hate robertshields's aversion to concise code", but then removed it. Done. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:229: dists[num_dists++] = products[i]->distribution(); On 2012/07/24 17:16:50, robertshield wrote: > 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(). What?!?? There's a forest around these trees? Done, and then some. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:378: BrowserDistribution::CHROME_BROWSER) || On 2012/07/24 17:16:50, robertshield wrote: > 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? 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. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:396: // --multi-install is given with no products. see On 2012/07/24 17:16:50, robertshield wrote: > nit: simultaneous -> simultaneously, see -> See Done. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:426: // Chrome Frame or the Application Host is being installed in On 2012/07/24 17:16:50, robertshield wrote: > "Chrome Frame or the Application Host" -> "A product other than Chrome" Done. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:534: AddExistingMultiInstalls(original_state, installer_state); On 2012/07/24 17:16:50, robertshield wrote: > Out of curiosity, how did this work before with CheckMultiInstallConditions > being called BEFORE AddExistingMultiInstalls? 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. http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... File chrome/installer/util/util_constants.h (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... chrome/installer/util/util_constants.h:85: COMPILE_ASSERT(installer::INCONSISTENT_UPDATE_POLICY == 43, On 2012/07/24 16:57:10, gab wrote: > Change this to COMPILE_ASSERT(installer::APP_HOST_REQUIRES_BINARIES == 45, ...); Nice catch, thanks. > I also feel the comment on this assert is unclear (i.e. what's the "contract" > between the 2 setups?)... I feel it's more: > "The order of the enum above cannot change. All new statuses should be added at > the end." Good point. Let me know what you think of my change.
http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... File chrome/installer/util/util_constants.h (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... chrome/installer/util/util_constants.h:85: COMPILE_ASSERT(installer::INCONSISTENT_UPDATE_POLICY == 43, On 2012/07/24 20:31:29, grt wrote: > On 2012/07/24 16:57:10, gab wrote: > > Change this to COMPILE_ASSERT(installer::APP_HOST_REQUIRES_BINARIES == 45, > ...); > > Nice catch, thanks. > > > I also feel the comment on this assert is unclear (i.e. what's the "contract" > > between the 2 setups?)... I feel it's more: > > "The order of the enum above cannot change. All new statuses should be added > at > > the end." > > Good point. Let me know what you think of my change. lgtm
On 2012/07/24 20:34:49, gab wrote: > http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... > File chrome/installer/util/util_constants.h (right): > > http://codereview.chromium.org/10806086/diff/1/chrome/installer/util/util_con... > chrome/installer/util/util_constants.h:85: > COMPILE_ASSERT(installer::INCONSISTENT_UPDATE_POLICY == 43, > On 2012/07/24 20:31:29, grt wrote: > > On 2012/07/24 16:57:10, gab wrote: > > > Change this to COMPILE_ASSERT(installer::APP_HOST_REQUIRES_BINARIES == 45, > > ...); > > > > Nice catch, thanks. > > > > > I also feel the comment on this assert is unclear (i.e. what's the > "contract" > > > between the 2 setups?)... I feel it's more: > > > "The order of the enum above cannot change. All new statuses should be added > > at > > > the end." > > > > Good point. Let me know what you think of my change. > > lgtm Thanks!
lgtm http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (left): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:460: if (products.size() != 1) On 2012/07/24 20:31:29, grt wrote: > On 2012/07/24 17:16:50, robertshield wrote: > > On 2012/07/24 15:02:03, grt wrote: > > > 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. > > > > I believe you, but that isn't obvious from reading the code even with the > > comment. > > What part of "It isn't possible" do you not understand. ;-) I typically come across comments stating that "X is not possible" while debugging a crash caused by X happening :-) > > > 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. > > I'll remove it since it'll never happen. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:225: // being too tricky. On 2012/07/24 20:31:29, grt wrote: > On 2012/07/24 17:16:50, robertshield wrote: > > Grumbling noted, but the above comment doesn't make this code clearer. Please > > remove. > > I briefly changed the comment to "I hate robertshields's aversion to concise > code", but then removed it. Done. concise != readable
http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (left): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:460: if (products.size() != 1) On 2012/07/24 21:03:14, robertshield wrote: > On 2012/07/24 20:31:29, grt wrote: > > On 2012/07/24 17:16:50, robertshield wrote: > > > On 2012/07/24 15:02:03, grt wrote: > > > > 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. > > > > > > I believe you, but that isn't obvious from reading the code even with the > > > comment. > > > > What part of "It isn't possible" do you not understand. ;-) > > I typically come across comments stating that "X is not possible" while > debugging a crash caused by X happening :-) remind me to work on shutdown next. http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10806086/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:225: // being too tricky. On 2012/07/24 21:03:14, robertshield wrote: > concise != readable LOL, BFF
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grt@chromium.org/10806086/8001
Change committed as 148261 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
