Issue 11685006: [Fixit-Dec-2012] Auto-launch system-level Chrome post user-level Chrome self-destruction.

Issue 11685006: [Fixit-Dec-2012] Auto-launch system-level Chrome post user-level Chrome self-destruction. (Closed)

Created:
7 years, 11 months ago by gab

Modified:
7 years, 11 months ago

Reviewers:
brettw, robertshield, grt (UTC plus 2)

CC:
chromium-reviews, grt+watch_chromium.org, robertshield

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

[Fixit-Dec-2012] Auto-launch system-level Chrome post user-level Chrome self-destruction. Also changed self-destruct to launch setup.exe using ShellExecute instead of base::LaunchProcess to allow us to specify SEE_MASK_FLAG_LOG_USAGE to flip to the desktop as documented in "DevelopinganewexperienceDesktopBrowser". This only seems to work when ShellExecute is called from an immersive process and thus setup.exe must be launched with this flag (if it's not and setup later tries to ShellExecute the system-level Chrome with that flag, the desktop flip doesn't occur...). Also fixed InstallUtil::GetSentinelFilePath() which used to get DIR_EXE from the PathService to determine whether it was a user-level install and if so return DIR_EXE as the sentinel file path; this was wrong when called from setup.exe as DIR_EXE is not Application\ in that case, it's Application\<version>\Installer...! Thankfully this method was not used to set the sentinel (so that we don't have to keep checking for sentinel files in the wrong places); we could up until this CL however get constant false negatives when looking for sentinels with this method. This CL depends on https://codereview.chromium.org/11636031/. BUG=165048 TEST=Self-destruct with/without First Run having occured (prior to the system-level install) for the user-level Chrome; the self-destruct should occur and launch System-level Chrome with/without the First Run flow (i.e. with First Run only if the self-destructing Chrome itself hadn't undergone First Run already). On Win8, the above scenarios should work whether the user-level Chrome prefers Metro or not (if it does prefer Metro the immersive launch should flip back to the desktop for the self-destruct -- the self-destruct message dialog is still suppressed in that case however). Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175249

Patch Set 1 : . #

Total comments: 36

Patch Set 2 : +grt comments #

Total comments: 4

Patch Set 3 : ++grt #

Total comments: 4

Patch Set 4 : +++grt #

Created: 7 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+76 lines, -13 lines)			Patch
M	chrome/browser/chrome_browser_main_win.cc	View	1	1 chunk	+16 lines, -3 lines	0 comments	Download
M	chrome/installer/setup/setup_main.cc	View	1 2	3 chunks	+39 lines, -1 line	0 comments	Download
M	chrome/installer/setup/uninstall.cc	View	1	1 chunk	+0 lines, -8 lines	0 comments	Download
M	chrome/installer/util/install_util.cc	View	1 2 3	1 chunk	+16 lines, -1 line	0 comments	Download
M	chrome/installer/util/util_constants.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/installer/util/util_constants.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

grt (UTC plus 2)

+robert for my comment at chrome_browser_main_win.cc:378. https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc#newcode369 chrome/browser/chrome_browser_main_win.cc:369: SHELLEXECUTEINFO sei = ...

7 years, 11 months ago (2013-01-02 17:59:33 UTC) #2

robertshield

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc#newcode378 chrome/browser/chrome_browser_main_win.cc:378: if (!::ShellExecuteExW(&sei)) On 2013/01/02 17:59:33, grt wrote: > MSDN ...

7 years, 11 months ago (2013-01-02 19:36:06 UTC) #3

gab

Thanks, PTAL! https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc#newcode369 chrome/browser/chrome_browser_main_win.cc:369: SHELLEXECUTEINFO sei = { sizeof(sei) }; On ...

7 years, 11 months ago (2013-01-02 21:15:48 UTC) #4

Thanks, PTAL!

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:369: SHELLEXECUTEINFO sei = {
sizeof(sei) };
On 2013/01/02 17:59:33, grt wrote:
> For safety, I think you need:
>         sei.fMask = SEE_MASK_NOASYNC;
> see MSDN for details

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:372: sei.lpParameters =
params.c_str();
On 2013/01/02 17:59:33, grt wrote:
> MSDN says that quotes in lpParameters must be quoted by putting them in quotes
> (e.g., " -> """). This isn't the quoting strategy used by
> CommandLine::GetArgumentsString. I suspect we do this everywhere we call
> ShellExecute, so maybe there's nothing to be done except to be aware of it and
> hope that we never have a quoted string in the arguments.

Interesting, if this is ever a problem (and if it is, it was already one) it
should be fixed in command_line.cc, not here imo.

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:376: sei.fMask =
SEE_MASK_FLAG_LOG_USAGE;
On 2013/01/02 17:59:33, grt wrote:
> = -> |=

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:378: if (!::ShellExecuteExW(&sei))
On 2013/01/02 17:59:33, grt wrote:
> nit: please remove the trailing 'W'

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:378: if (!::ShellExecuteExW(&sei))
On 2013/01/02 19:36:06, robertshield wrote:
> On 2013/01/02 17:59:33, grt wrote:
> > MSDN suggests that COM should always be initialized, although perhaps it
isn't
> > needed in this case since lpFile is the executable itself. I don't know if
the
> > shell could decide to load up an extension for even this simple case. I'll
add
> > Robert since he has more in-the-shell knowledge than I do.
> 
> This should be fine.  

Ack.

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:379: NOTREACHED();
On 2013/01/02 17:59:33, grt wrote:
> DPCHECK(false); so that the last-error code is logged.

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1015: scoped_ptr<CommandLine>
system_level_cmd;
On 2013/01/02 17:59:33, grt wrote:
> It seems more straightforward to use:
>   CommandLine system_level_cmd(CommandLine::NO_PROGRAM);
> here,
>        SetProgram(system_exe_path);
> on line 1030, and
>      if (!system_level_cmd.GetProgram().empty())
> on line 1073, in which case scoped_ptr.h isn't needed.

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1024: if
(cmd_line.HasSwitch(installer::switches::kSelfDestruct)) {
On 2013/01/02 17:59:33, grt wrote:
> && !system_level() since this shouldn't be done if someone accidentally puts
> --self-destruct on a system-level uninstall command line?

Sure.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1034: chrome::kFirstRunSentinel, false,
products[0]->distribution(),
On 2013/01/02 17:59:33, grt wrote:
> isn't products[0]->distribution() the same as |dist|?

Indeed :)!

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1039: // through first run.
On 2013/01/02 17:59:33, grt wrote:
> is there a difference between using --no-first-run and simply creating the
> system-level beacon here?

Hmmm, well arguably dropping the beacon here could fail (i.e. write error) where
as passing --no-first-run can't really fail...

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1041:
system_level_cmd->AppendSwitch(::switches::kNoFirstRun);
On 2013/01/02 17:59:33, grt wrote:
> nit: remove the :: in front of "switches" unless it's actually necessary.

It is necessary since installer::switches also exists and this code lives in the
installer:: namespace.
kNoFirstRun is in switches:: not installer::switches::.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1070: if (trigger_active_setup)
On 2013/01/02 17:59:33, grt wrote:
> install_status may be any of: (CHROME_NOT_INSTALLED, UNINSTALL_SUCCESSFUL,
> UNINSTALL_FAILED, UNINSTALL_REQUIRES_REBOOT). should active setup and/or the
> handoff happen for all of those cases?

In fact I think it was wrong to base this off of "install_status !=
installer::UNINSTALL_CANCELLED" (had copied that logic from the for loop
condition -- and UNINSTALL_CANCELLED is only set by the user prompt which isn't
even shown with --force-uninstall anyways...).

I think that we should always launch Active Setup and the system-level Chrome no
matter the uninstall result.

i.e., I think it's a better user experience to keep getting the self-destruct
prompt in case of repetitive uninstall failures, but at least get system-level
Chrome to launch, then it is to only get the prompt with no Chrome launching...

I could wrap all of this in an if block that also checks for the presence of
kSelfDestruct; that would be redundant, but perhaps more readable..?

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1110: bool CreateEULASentinel(bool
system_install,
On 2013/01/02 17:59:33, grt wrote:
> The EULA only applies for system-level installs, so you can remove
> |system_install| and pass |true| directly to GetSentinelFilePath.

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:1284:
CreateEULASentinel(installer_state->system_install(),
On 2013/01/02 17:59:33, grt wrote:
> FYI: --system-level isn't passed to setup.exe in this case, so
> installer_state->system_install() cannot be used. this isn't a problem,
however;
> see comment on line 1110.

Done.

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:27: #include
"chrome/common/chrome_switches.h"
On 2013/01/02 17:59:33, grt wrote:
> remove?

Oops, leftover from before I moved it all to setup_main.cc :)!

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:384: *path =
installer::GetChromeInstallPath(false, dist);
On 2013/01/02 17:59:33, grt wrote:
> This has a different subtle flaw than the old one (nice catch there, btw!).
The
> old technique, when called by Chrome, would return the correct path even for
the
> case of chrome.exe executing outside of the normal per-user install directory
> (in a build tree or on a bot, for example).

Ah! Good catch :)! Fixed both now I think!

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/util/util...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:159: const char kSelfDestruct[] =
"self-destruct";
On 2013/01/02 17:59:33, grt wrote:
> I think this switch should be a little more explicit since it's for the case
of
> a self-destruct due to the presence of a system-level install. I can imagine
> someday there being a need for Chrome to uninstall itself (i.e., to "self
> destruct") for other reasons.
> 
> Thinking about this at a high level, we have a choice between using switches
> that describe the caller's reason for invoking setup (i.e., "Yo setup! Do the
> stuff you need to do to handle a user-level self-destruct because there's a
new
> Chrome in town!") versus using switches that tell setup specifically what the
> caller wants it to do (i.e., "Yo setup! Uninstall yourself without prompting
the
> user, and don't bother removing anything that might be shared with a
> system-level Chrome"). The latter seems to be historically what was used in
this
> case. What are the pros/cons to sticking with the latter? In this case, the
new
> switch could be something along the lines of
"launch-system-chrome-on-success".

I can imagine more cases where we would want to do something special in setup on
self-destruct then cases where we would want to fine grain call
--launch-system-chrome-on-success et al.

I think adding a new flag for every nit picky thing we want setup to do because
of the same reason is overkill; I prefer saying "Yo Setup! Please uninstall
Chrome, oh and btw this is because user-level Chrome is self-destructing so do
whatever you need to do in this case as well.".

On the other hand I agree that having a flag that does everything can be
dangerous/hard to maintain, but I don't think this is the case, nor will it be
in the near future...

I don't feel like the actual text value of the flag (i.e. --self-destruct versus
--omg-please-do-tha-thang-you-do-on-self-destruct-cause-hey-man-this-is-it-i-just-self-destructed)
matters at all since it's passed as a code constant on one side and read the
same way on the other (this is not the kind of flag we would mention in any sort
of public API...)

grt (UTC plus 2)

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_browser_main_win.cc#newcode372 chrome/browser/chrome_browser_main_win.cc:372: sei.lpParameters = params.c_str(); On 2013/01/02 21:15:48, gab wrote: > ...

7 years, 11 months ago (2013-01-03 15:41:27 UTC) #5

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/11685006/diff/1009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:372: sei.lpParameters =
params.c_str();
On 2013/01/02 21:15:48, gab wrote:
> On 2013/01/02 17:59:33, grt wrote:
> > MSDN says that quotes in lpParameters must be quoted by putting them in
quotes
> > (e.g., " -> """). This isn't the quoting strategy used by
> > CommandLine::GetArgumentsString. I suspect we do this everywhere we call
> > ShellExecute, so maybe there's nothing to be done except to be aware of it
and
> > hope that we never have a quoted string in the arguments.
> 
> Interesting, if this is ever a problem (and if it is, it was already one) it
> should be fixed in command_line.cc, not here imo.

Yup. Wasn't suggesting that this was the place, just raising awareness.

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1026: BrowserDistribution* dist =
products[0]->distribution();
even though there's a DCHECK to enforce that products[0] is the one you want,
it's safer to cache the result of FindProduct on line 1017 and use its
distribution() method to get the BD.

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/util/ins...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/util/ins...
chrome/installer/util/install_util.cc:390: FilePath user_level_install_path(
instead of requiring that the default install path be used, i think it's more
robust to look for chrome.exe in the current directory (true when called from
chrome or from setup.exe in a developer dir) and then two directories up (true
when called from setup.exe in any install dir).  so, something along the lines
of:
if (file_util::PathExists(exe_path.Append(installer::kChromeExe))) {
  *path = exe_path;
  return true;
}
FilePath product_dir(exe_path.DirName().DirName());
if (file_util::PathExists(product_dir.Append(installer::kChromeExe))) {
  *path = product_dir;
  return true;
}

gab

Done, PTAL. Thanks! Gab https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/setup_main.cc File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/setup_main.cc#newcode1026 chrome/installer/setup/setup_main.cc:1026: BrowserDistribution* dist = products[0]->distribution(); On ...

7 years, 11 months ago (2013-01-03 17:11:14 UTC) #6

Done, PTAL.

Thanks!
Gab

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1026: BrowserDistribution* dist =
products[0]->distribution();
On 2013/01/03 15:41:27, grt wrote:
> even though there's a DCHECK to enforce that products[0] is the one you want,
> it's safer to cache the result of FindProduct on line 1017 and use its
> distribution() method to get the BD.

Done.

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/util/ins...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11685006/diff/11001/chrome/installer/util/ins...
chrome/installer/util/install_util.cc:390: FilePath user_level_install_path(
On 2013/01/03 15:41:27, grt wrote:
> instead of requiring that the default install path be used, i think it's more
> robust to look for chrome.exe in the current directory (true when called from
> chrome or from setup.exe in a developer dir) and then two directories up (true
> when called from setup.exe in any install dir).  so, something along the lines
> of:
> if (file_util::PathExists(exe_path.Append(installer::kChromeExe))) {
>   *path = exe_path;
>   return true;
> }
> FilePath product_dir(exe_path.DirName().DirName());
> if (file_util::PathExists(product_dir.Append(installer::kChromeExe))) {
>   *path = product_dir;
>   return true;
> }

Fine, I was trying to avoid the assumption about being in the Installer
directory two levels above the product dir, but you convinced me otherwise :).

This can't return here though as there is further work to be done on |path| at
the bottom of this method. Because of this fact I think the most elegant way is
to use this if,elseif,else block; although this has the ugly aspect of always
initializing product_dir...

grt (UTC plus 2)

lgtm w/ a nit and comment clarification https://codereview.chromium.org/11685006/diff/17001/chrome/installer/util/install_util.cc File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11685006/diff/17001/chrome/installer/util/install_util.cc#newcode393 chrome/installer/util/install_util.cc:393: maybe_product_dir.Append(installer::kChromeExe))) { ...

7 years, 11 months ago (2013-01-03 17:48:58 UTC) #7

gab

Thanks, @brettw for OWNER approval on chrome/browser/chrome_browser_main_win.cc gab https://codereview.chromium.org/11685006/diff/17001/chrome/installer/util/install_util.cc File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/11685006/diff/17001/chrome/installer/util/install_util.cc#newcode393 chrome/installer/util/install_util.cc:393: maybe_product_dir.Append(installer::kChromeExe))) ...

7 years, 11 months ago (2013-01-03 18:39:49 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11685006/16008

7 years, 11 months ago (2013-01-04 22:05:05 UTC) #10

Message was sent while issue was closed.

Change committed as 175249

Expand Messages | Collapse Messages