Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1271893003:
Add options to write version number to registry. (Closed)
DescriptionAdd options to write version number to registry.
The MSI installer creates a DisplayVersion registry entry
with an encoded version of Chrome's actual version. Since
this is done _after_ the setup call completes, we can't
simply set the value in the normal setup process. Instead,
we launch a background process that waits a while (for
msiexec to complete) and then writes the updated version.
This is accomplished by having setup, upon successful completion,
run itself in the background and then exit normally.
--set-display-version-product="ABCD-EF-GH-IJKL"
--set-display-version-value="A.B.C.D"
--delay=30
BUG=67348
Committed: https://crrev.com/0d3e644a25802bdb3afa7fc09eff5aab456c5733
Cr-Commit-Position: refs/heads/master@{#348556}
Committed: https://crrev.com/30198c1e69bc8c91e4e0853fa399702b8bddb302
Cr-Commit-Position: refs/heads/master@{#348732}
Patch Set 1 #Patch Set 2 : extract registry overwrite to its own method #
Total comments: 16
Patch Set 3 : address review comments by robertshield #
Total comments: 2
Patch Set 4 : restore ReadValue call (HasValue check fails) #Patch Set 5 : comment improvements #Patch Set 6 : start subprocess to set DisplayVersion #
Total comments: 16
Patch Set 7 : addressed review comments #
Total comments: 14
Patch Set 8 : address review comments by grt #Patch Set 9 : convert msi-product-guid to squid for registry lookup #Patch Set 10 : reduce delay from 30s to 10s #Patch Set 11 : change DisplayVersion in Uninstall, too #
Total comments: 18
Patch Set 12 : addressed review comments by grt #
Total comments: 8
Patch Set 13 : addressed more review comments #
Total comments: 2
Patch Set 14 : fix format to %ls #
Total comments: 2
Patch Set 15 : fix parameters for %ls #Patch Set 16 : fix parethesis #Messages
Total messages: 66 (18 generated)
bcwhite@chromium.org changed reviewers: + robertshield@chromium.org
Robert... This is only partially complete. Still missing is the code to actually start itself as a new process with the new parameters but I wanted to get your thoughts on this first in case I'm going down the wrong path. Thanks.
Lg, a few comments below. While reading this, two other possible approaches came to mind: 1) Use RegNotifyChangeKeyValue 2) Pass a handle to the original mini_installer's ancestor MSi process to the child setup.exe process and have the child wait on the handle instead of sleeping. Let me know if you think either of those would be workable. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:94: LONG OverwriteDisplayVersion(base::string16 path, base::string16 value) { pass by reference, no need to make a copy: const base::string16& https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:96: LONG result; = 0 https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:97: std::wstring existing; Should be able to use base::string16 here https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:104: return result; It looks like we don't use the value of |existing|. If we just want to check for existence, then key.HasValue() could serve instead. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1691: if (cmd_line.HasSwitch(installer::switches::kDelay)) { Could this block be placed at the top of HandleNonInstallCmdLineOptions? https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1692: std::string delay_seconds_string( const https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1694: int delay_seconds = std::stoi(delay_seconds_string); Most code I see uses base::StringToInt rather than stoi()
https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:103: if ((result = key.ReadValue(kDisplayVersion, &existing)) != ERROR_SUCCESS) We should log to the installer log when the value can't be found as that will suggest that the --delay parameter isn't long enough.
> 1) Use RegNotifyChangeKeyValue Can be sure it will change and will change only once? > 2) Pass a handle to the original mini_installer's > ancestor MSi process to the > child setup.exe process and have the child wait on the > handle instead of sleeping. That makes sense. Can you pass a handle as a string on the command-line? https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:94: LONG OverwriteDisplayVersion(base::string16 path, base::string16 value) { On 2015/08/08 04:32:38, robertshield wrote: > pass by reference, no need to make a copy: const base::string16& Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:96: LONG result; On 2015/08/08 04:32:39, robertshield wrote: > = 0 Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:97: std::wstring existing; On 2015/08/08 04:32:39, robertshield wrote: > Should be able to use base::string16 here Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:103: if ((result = key.ReadValue(kDisplayVersion, &existing)) != ERROR_SUCCESS) On 2015/08/08 04:33:30, robertshield wrote: > We should log to the installer log when the value can't be found as that will > suggest that the --delay parameter isn't long enough. Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:104: return result; On 2015/08/08 04:32:39, robertshield wrote: > It looks like we don't use the value of |existing|. If we just want to check for > existence, then key.HasValue() could serve instead. Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1691: if (cmd_line.HasSwitch(installer::switches::kDelay)) { On 2015/08/08 04:32:39, robertshield wrote: > Could this block be placed at the top of HandleNonInstallCmdLineOptions? Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1692: std::string delay_seconds_string( On 2015/08/08 04:32:38, robertshield wrote: > const Done. https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1694: int delay_seconds = std::stoi(delay_seconds_string); On 2015/08/08 04:32:38, robertshield wrote: > Most code I see uses base::StringToInt rather than stoi() Done.
On Thu, Aug 13, 2015 at 12:25 PM, <bcwhite@chromium.org> wrote: > 1) Use RegNotifyChangeKeyValue >> > > Can be sure it will change and will change only once? > > 2) Pass a handle to the original mini_installer's >> ancestor MSi process to the >> child setup.exe process and have the child wait on the >> handle instead of sleeping. >> > > That makes sense. Can you pass a handle as a string on the command-line? Yes, though by default a handle is only valid in a single process. Two ways to work around this: - when you're creating a child process you can set it to inherit all of the parent's handles using the bInheritHandle parameter to CreateProcess - you can use DuplicateHandle() to dupe a specific handle into a given process With either of these done, you can pass the handle (which, like all things, is really just a number) down on the command line. > > > > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > File chrome/installer/setup/setup_main.cc (right): > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:94: LONG > OverwriteDisplayVersion(base::string16 path, base::string16 value) { > On 2015/08/08 04:32:38, robertshield wrote: > >> pass by reference, no need to make a copy: const base::string16& >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:96: LONG result; > On 2015/08/08 04:32:39, robertshield wrote: > >> = 0 >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:97: std::wstring existing; > On 2015/08/08 04:32:39, robertshield wrote: > >> Should be able to use base::string16 here >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:103: if ((result = > key.ReadValue(kDisplayVersion, &existing)) != ERROR_SUCCESS) > On 2015/08/08 04:33:30, robertshield wrote: > >> We should log to the installer log when the value can't be found as >> > that will > >> suggest that the --delay parameter isn't long enough. >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:104: return result; > On 2015/08/08 04:32:39, robertshield wrote: > >> It looks like we don't use the value of |existing|. If we just want to >> > check for > >> existence, then key.HasValue() could serve instead. >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1691: if > (cmd_line.HasSwitch(installer::switches::kDelay)) { > On 2015/08/08 04:32:39, robertshield wrote: > >> Could this block be placed at the top of >> > HandleNonInstallCmdLineOptions? > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1692: std::string > delay_seconds_string( > On 2015/08/08 04:32:38, robertshield wrote: > >> const >> > > Done. > > > https://codereview.chromium.org/1271893003/diff/20001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1694: int delay_seconds = > std::stoi(delay_seconds_string); > On 2015/08/08 04:32:38, robertshield wrote: > >> Most code I see uses base::StringToInt rather than stoi() >> > > Done. > > https://codereview.chromium.org/1271893003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/1271893003/diff/40001/chrome/installer/util/u... File chrome/installer/util/util_constants.cc (right): https://codereview.chromium.org/1271893003/diff/40001/chrome/installer/util/u... chrome/installer/util/util_constants.cc:36: // only after some time has passed. *This is used by the MSI installer https://codereview.chromium.org/1271893003/diff/40001/chrome/installer/util/u... chrome/installer/util/util_constants.cc:142: // Set the DisplayVersion in the registry to match Chrome's real version number. * Set the MSI-managed DisplayVersion value in the registry...
Added code to fork sub-process and set registry entry, 30 second delay. PTAL I've tested this by hard-coding the destination id. Now there needs to be code added to the MSI builder to stuff the id into the installer-data structure.
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:135: base::string16 command_line; base::CommandLine provides convenience methods to do this much more simply. AppendSwitchNative() will help. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), use base::LaunchProcess here (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:156: (LPWSTR)command_line.c_str(), // discards const can you const_cast instead? https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/util/... File chrome/installer/util/master_preferences_constants.h (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/util/... chrome/installer/util/master_preferences_constants.h:87: // String. The installer ID under which the MSI stores its information. This s/installer ID/MSI Product ID/
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), On 2015/08/18 17:27:26, robertshield wrote: > use base::LaunchProcess here > (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) i wonder if this should also use force_breakaway_from_job_? i don't know whether or not msiexec runs procs in a job, but i think it should be safe to try to breakaway in any case. what do you think, robert? the reason to do so is that it would be bad if msiexec was using a job object and did something like wait for all procs in the job to complete. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1248: const base::string16 registry_value( i think it's better to extract the version from the current binary rather than pass it on the command line (see chrome/app/client_util.cc's GetCurrentModuleVersion for code to copy/paste). https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1642: DelayedOverwriteDisplayVersion(setup_exe, install_id, *installer_version); if i'm reading the code correctly, |setup_exe| is the path to the currently running setup.exe, which will be deleted once this execution of setup.exe exits. this code should instead use the path to the setup.exe that was just installed. the call to GetInstallerDirectory on line 1648 gives you the directory containing the installed setup.exe.
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), On 2015/08/19 17:43:30, grt (no reviews 7.31-8.19) wrote: > On 2015/08/18 17:27:26, robertshield wrote: > > use base::LaunchProcess here > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) > > i wonder if this should also use force_breakaway_from_job_? i don't know whether > or not msiexec runs procs in a job, but i think it should be safe to try to > breakaway in any case. what do you think, robert? the reason to do so is that it > would be bad if msiexec was using a job object and did something like wait for > all procs in the job to complete. I don't think it would hurt. MSI could also conceivably be using the job object to perform install cancellation, which we'd then escape from. Afaict we no-op above in OverwriteDisplayVersion if the expected values aren't present so this should be okay.
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:135: base::string16 command_line; On 2015/08/18 17:27:26, robertshield wrote: > base::CommandLine provides convenience methods to do this much more simply. > AppendSwitchNative() will help. Done. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), On 2015/08/18 17:27:26, robertshield wrote: > use base::LaunchProcess here > (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) Why here but not elsewhere in this file? https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:156: (LPWSTR)command_line.c_str(), // discards const On 2015/08/18 17:27:26, robertshield wrote: > can you const_cast instead? Already gone with switch to base::CommandLine. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1248: const base::string16 registry_value( On 2015/08/19 17:43:30, grt wrote: > i think it's better to extract the version from the current binary rather than > pass it on the command line (see chrome/app/client_util.cc's > GetCurrentModuleVersion for code to copy/paste). If the original process has already gone through the trouble to get it, is it worth doing this? Also, this leaves the flexibility of the caller being able to choose something different if so desired. Lastly, it is (IMO) more symmetric to pass both key & value rather than just one. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1642: DelayedOverwriteDisplayVersion(setup_exe, install_id, *installer_version); On 2015/08/19 17:43:30, grt wrote: > if i'm reading the code correctly, |setup_exe| is the path to the currently > running setup.exe, which will be deleted once this execution of setup.exe exits. > this code should instead use the path to the setup.exe that was just installed. > the call to GetInstallerDirectory on line 1648 gives you the directory > containing the installed setup.exe. Done. https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/util/... File chrome/installer/util/master_preferences_constants.h (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/util/... chrome/installer/util/master_preferences_constants.h:87: // String. The installer ID under which the MSI stores its information. This On 2015/08/18 17:27:26, robertshield wrote: > s/installer ID/MSI Product ID/ Done.
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), On 2015/09/02 15:21:45, bcwhite wrote: > On 2015/08/18 17:27:26, robertshield wrote: > > use base::LaunchProcess here > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) > > Why here but not elsewhere in this file? base::LaunchProcess should always be used unless something special it doesn't support is needed. If there are other direct uses of CreateProcess here, they should likely be switched. It's very easy to mis-use CreateProcess in interesting ways. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:140: base::UTF8ToUTF16(version.GetString())); nit: base::ASCIIToUTF16 here since we're certain the version string will be ASCII. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:997: ::Sleep(delay_seconds * 1000); if sleeping is really the only way to do this (as opposed to waiting for the registry value to be written; see base::win::RegKey::StartWatching), please use base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(delay_seconds)). that said, i'm concerned about the reliability of the sleeping strategy. why not either wait on the key or one of the parent procs? waiting on the key is probably easier and more reliable. if you're concerned about multiple writes to the value, you could sleep a little bit after the initial write. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1639: std::string install_id; nit: move install_id and new_setup into the body of the if below https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1641: installer_state.GetInstallerDirectory(*installer_version); four-space indent https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1642: new_setup.Append(kSetupExe); either "new_setup = new_setup.Append(kSetupExe);", or put the Append on the line above. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1649: if (installer_directory) nit: braces for multi-line body https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/util/... File chrome/installer/util/util_constants.h (right): https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/util/... chrome/installer/util/util_constants.h:142: extern const char kDelay[]; i think it makes sense for these constants to be in setup/setup_constants.{cc,h} since they are implementation details of setup.exe and aren't needed by chrome.exe.
https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/100001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:155: if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(), On 2015/09/02 20:43:55, grt wrote: > On 2015/09/02 15:21:45, bcwhite wrote: > > On 2015/08/18 17:27:26, robertshield wrote: > > > use base::LaunchProcess here > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...) > > > > Why here but not elsewhere in this file? > > base::LaunchProcess should always be used unless something special it doesn't > support is needed. If there are other direct uses of CreateProcess here, they > should likely be switched. It's very easy to mis-use CreateProcess in > interesting ways. Done. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:140: base::UTF8ToUTF16(version.GetString())); On 2015/09/02 20:43:55, grt wrote: > nit: base::ASCIIToUTF16 here since we're certain the version string will be > ASCII. Done. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:997: ::Sleep(delay_seconds * 1000); On 2015/09/02 20:43:55, grt wrote: > if sleeping is really the only way to do this (as opposed to waiting for the > registry value to be written; see base::win::RegKey::StartWatching), please use > base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(delay_seconds)). > > that said, i'm concerned about the reliability of the sleeping strategy. why not > either wait on the key or one of the parent procs? waiting on the key is > probably easier and more reliable. if you're concerned about multiple writes to > the value, you could sleep a little bit after the initial write. Done. I think the best way is to have the process wait for the msiexec process to end and then do the write. I'm looking into it but want to get everything else resolved first. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1639: std::string install_id; On 2015/09/02 20:43:55, grt wrote: > nit: move install_id and new_setup into the body of the if below install_id is used by the "if" statement but I've moved new_setup https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1641: installer_state.GetInstallerDirectory(*installer_version); On 2015/09/02 20:43:55, grt wrote: > four-space indent Done. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1642: new_setup.Append(kSetupExe); On 2015/09/02 20:43:55, grt wrote: > either "new_setup = new_setup.Append(kSetupExe);", or put the Append on the line > above. Done. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1649: if (installer_directory) On 2015/09/02 20:43:55, grt wrote: > nit: braces for multi-line body Done. https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/util/... File chrome/installer/util/util_constants.h (right): https://codereview.chromium.org/1271893003/diff/120001/chrome/installer/util/... chrome/installer/util/util_constants.h:142: extern const char kDelay[]; On 2015/09/02 20:43:55, grt wrote: > i think it makes sense for these constants to be in setup/setup_constants.{cc,h} > since they are implementation details of setup.exe and aren't needed by > chrome.exe. Done.
Robert: do you think this should use WMI to launch the sub-proc rather than just breakaway (see comment here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...
On 2015/09/10 15:56:30, grt wrote: > Robert: do you think this should use WMI to launch the sub-proc rather than just > breakaway (see comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u... The documented behaviour of CREATE_BREAKAWAY_FROM_JOB should accomplish the desired effect of getting out of the job object which is the thing MSI should be using to manage sub processes. The only reason to use WMI instead IMO would be if we thought MSI was tracking created child processes outside of the Job mechanism and waiting on them all, but that would be both more complicated and I'd be surprised if MSI was doing that. It should be relatively easy to test if the breakaway flag works (have setup block forever, see if MSI kills it or waits on it).
> The documented behaviour of CREATE_BREAKAWAY_FROM_JOB should accomplish the > desired effect of getting out of the job object which is the thing MSI should be > using to manage sub processes. > > The only reason to use WMI instead IMO would be if we thought MSI was tracking > created child processes outside of the Job mechanism and waiting on them all, > but that would be both more complicated and I'd be surprised if MSI was doing > that. > > It should be relatively easy to test if the breakaway flag works (have setup > block forever, see if MSI kills it or waits on it). The delay works fine both with and without the "breakaway" flag. It appears that Microsoft figures that an MSI will be well-behaved. I guess that if it wants to be malicious, killing jobs isn't an effective mitigation technique.
I've reduced the delay time from 30s to 10s. All my runs show that msiexec exits less than 2.5s after the main setup.exe completes. I think 4x this time is sufficient to cover variances and reduce the risk of the system closing before the DisplayVersion write can complete. I've tested this on both Win7 and Win10 VMs and the overwrite occurs properly. The problem is that this doesn't change the Version listed under "Programs and Features". *That* display comes from the "Uninstall" registry key. I'll add code to update that version as well.
I think that covers it. Improvements to when the registry change runs can be done in another CL if need be but this should be sufficient for now.
https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:83: const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\"; please move all of these constants that are only used in this file into the unnamed namespace below. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:136: reg_path += base::ASCIIToUTF16( can GuidToSquid operate on base::string16 so we don't need to convert to ascii and back like this? https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:143: reg_path = kMsiUninstallRegistryPrefix; i think it's more clear to use base::StringPrintf with a format string rather than hand-roll like this. may as well do the same for the path above, too. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:161: command_line.AppendSwitchNative(installer::switches::kSetDisplayVersionValue, nit: use AppendSwitchASCII and remove the ASCIIToUFT16 conversion here https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:170: LOG(ERROR) << "Failed to set DisplayVersion: " tip: PLOG(ERROR) and remove the GetLastError() tidbit. PLOG will magically format the last-error-code onto the end of the message. nice, eh? https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:172: << " (error=" << ::GetLastError() << ")\n" i think it's best not to put newlines in log messages. i like when each line begins with the log header. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1671: new_setup, base::ASCIIToUTF16(install_id), *installer_version); nit: pass install_id directly and use AppendSwitchASCII in DelayedOverwriteDisplayVersions. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:632: std::string GuidToSquid(const std::string& guid) { can you get rid of the extra copy by using an output iterator? #include <iterator> { std::string squid; squid.reserve(32); auto iter = squid.begin() auto output = std::back_inserter(squid); std::reverse_copy(iter + 0, iter + 8, output); // Skip over separator std::reverse_copy(iter + 9, iter + 13, output); ... return squid; }
https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:83: const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\"; On 2015/09/11 18:14:26, grt wrote: > please move all of these constants that are only used in this file into the > unnamed namespace below. Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:136: reg_path += base::ASCIIToUTF16( On 2015/09/11 18:14:26, grt wrote: > can GuidToSquid operate on base::string16 so we don't need to convert to ascii > and back like this? Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:143: reg_path = kMsiUninstallRegistryPrefix; On 2015/09/11 18:14:26, grt wrote: > i think it's more clear to use base::StringPrintf with a format string rather > than hand-roll like this. may as well do the same for the path above, too. Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:161: command_line.AppendSwitchNative(installer::switches::kSetDisplayVersionValue, On 2015/09/11 18:14:26, grt wrote: > nit: use AppendSwitchASCII and remove the ASCIIToUFT16 conversion here Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:170: LOG(ERROR) << "Failed to set DisplayVersion: " On 2015/09/11 18:14:26, grt wrote: > tip: PLOG(ERROR) and remove the GetLastError() tidbit. PLOG will magically > format the last-error-code onto the end of the message. nice, eh? Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:172: << " (error=" << ::GetLastError() << ")\n" On 2015/09/11 18:14:26, grt wrote: > i think it's best not to put newlines in log messages. i like when each line > begins with the log header. Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1671: new_setup, base::ASCIIToUTF16(install_id), *installer_version); On 2015/09/11 18:14:26, grt wrote: > nit: pass install_id directly and use AppendSwitchASCII in > DelayedOverwriteDisplayVersions. Done. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:632: std::string GuidToSquid(const std::string& guid) { On 2015/09/11 18:14:26, grt wrote: > can you get rid of the extra copy by using an output iterator? > > #include <iterator> > > { > std::string squid; > squid.reserve(32); > auto iter = squid.begin() > auto output = std::back_inserter(squid); > std::reverse_copy(iter + 0, iter + 8, output); > // Skip over separator > std::reverse_copy(iter + 9, iter + 13, output); > ... > return squid; > } Done.
https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1671: new_setup, base::ASCIIToUTF16(install_id), *installer_version); On 2015/09/11 19:24:44, bcwhite wrote: > On 2015/09/11 18:14:26, grt wrote: > > nit: pass install_id directly and use AppendSwitchASCII in > > DelayedOverwriteDisplayVersions. > > Done. Not done? https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:98: L"SOFTWARE\\Wow6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\"; apologies for missing this earlier: the KEY_WOW64_32KEY flag must be used rather than "Wow6432Node" to access a 32-bit proc's view of the registry from any bitness process. i suppose that Chrome is in the 32-bit view because the .msi itself is somehow built as a 32-bit app even if the Chrome within is 64. to future-proof this, can you make the code check both views of the registry, making the edit to the one where it finds a previous value? https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:136: L"%s%s%s", kMsiInstallRegistryPrefix, installer::GuidToSquid(product), what i had in mind was a format string of: L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Installer\\UserData\\" L"S-1-5-18\\Products\\%ls\\InstallProperties" https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:143: L"%s{%s}%s", kMsiUninstallRegistryPrefix, product, L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\{%ls}" https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:169: LOG(ERROR) << command_line.GetCommandLineString(); nit: merge this into the previous log message.
I verified the update of Uninstall through checking both WoW64 nodes. You're probably not around so I'm going to submit this so I can get a build out to the testers early next week. If you have more comments, I'll address them in a second CL. https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/200001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1671: new_setup, base::ASCIIToUTF16(install_id), *installer_version); On 2015/09/11 20:07:01, grt wrote: > On 2015/09/11 19:24:44, bcwhite wrote: > > On 2015/09/11 18:14:26, grt wrote: > > > nit: pass install_id directly and use AppendSwitchASCII in > > > DelayedOverwriteDisplayVersions. > > > > Done. > > Not done? Weird. I absolutely did change that; no idea how it got reverted. Done again. https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:98: L"SOFTWARE\\Wow6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\"; On 2015/09/11 20:07:01, grt wrote: > apologies for missing this earlier: the KEY_WOW64_32KEY flag must be used rather > than "Wow6432Node" to access a 32-bit proc's view of the registry from any > bitness process. i suppose that Chrome is in the 32-bit view because the .msi > itself is somehow built as a 32-bit app even if the Chrome within is 64. to > future-proof this, can you make the code check both views of the registry, > making the edit to the one where it finds a previous value? Done. https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:136: L"%s%s%s", kMsiInstallRegistryPrefix, installer::GuidToSquid(product), On 2015/09/11 20:07:01, grt wrote: > what i had in mind was a format string of: > L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Installer\\UserData\\" > L"S-1-5-18\\Products\\%ls\\InstallProperties" Done. https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:143: L"%s{%s}%s", kMsiUninstallRegistryPrefix, product, On 2015/09/11 20:07:01, grt wrote: > L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\{%ls}" Done. https://codereview.chromium.org/1271893003/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:169: LOG(ERROR) << command_line.GetCommandLineString(); On 2015/09/11 20:07:01, grt wrote: > nit: merge this into the previous log message. Done.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/1271893003/#ps240001 (title: "addressed more review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/240001
https://codereview.chromium.org/1271893003/diff/240001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:131: L"%s\\Products\\%s\\InstallProperties", kSystemPrincipalSid, This needs to be %ls (below, too); see string_util.h.
The CQ bit was unchecked by grt@chromium.org
LGTM w/ format string fix.
https://codereview.chromium.org/1271893003/diff/240001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/240001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:131: L"%s\\Products\\%s\\InstallProperties", kSystemPrincipalSid, On 2015/09/12 13:41:32, grt wrote: > This needs to be %ls (below, too); see string_util.h. Right. I should have remembered that.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1271893003/#ps260001 (title: "fix format to %ls")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/0d3e644a25802bdb3afa7fc09eff5aab456c5733 Cr-Commit-Position: refs/heads/master@{#348556}
Message was sent while issue was closed.
hans@chromium.org changed reviewers: + hans@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1271893003/diff/260001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1271893003/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:132: installer::GuidToSquid(product)); This passes a non-POD type (installer::GuidToSquid returns a string16) to a variadic function (StringPrintf), which is not allowed (see e.g. http://stackoverflow.com/a/10083921) Also, %ls expects a wchar_t*, nothing else. https://codereview.chromium.org/1271893003/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:139: product); Same thing here. You'll want to pass product.c_str().
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1336923004/ by hans@chromium.org. The reason for reverting is: This broke the Windows Clang build. The error is pointing out a real problem: ..\..\chrome\installer\setup\setup_main.cc(132,7) : error: cannot pass object of non-trivial type 'base::string16' (aka 'basic_string<wchar_t, char_traits<wchar_t>, allocator<wchar_t> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs] installer::GuidToSquid(product)); ^ (From http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/3152/step...).
Message was sent while issue was closed.
On 2015/09/14 16:33:43, hans wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/1336923004/ by mailto:hans@chromium.org. > > The reason for reverting is: This broke the Windows Clang build. The error is > pointing out a real problem: > > ..\..\chrome\installer\setup\setup_main.cc(132,7) : > error: cannot pass object of non-trivial type 'base::string16' > (aka 'basic_string<wchar_t, char_traits<wchar_t>, allocator<wchar_t> >') > through variadic function; call will abort at runtime [-Wnon-pod-varargs] > installer::GuidToSquid(product)); > ^ > > (From > http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/3152/step...). Don! I *hate* printf! Odd that MSVC works just fine. Please check my fix since I don't know how to force a clang compile (pointer?).
On 2015/09/14 19:29:23, bcwhite wrote: > Don! I *hate* printf! Odd that MSVC works just fine. It mostly works by luck though. MSVC chooses to support passing non-PODs through varargs (that's ok, C++11 says it's optional and implementation defined), so it will push the std::wstring on the stack. Luckily, the pointer to the data is the first member of std::wstring, so that gets pushed on the stack first. > Please check my fix Looks right, just some misplaced parentheses on the first one. > since I don't know how to force a clang compile (pointer?). We don't have any trybots for Windows Clang unfortunately. Most of our bots are here: http://build.chromium.org/p/chromium.fyi/console?category=win%20clang
On 2015/09/14 19:56:30, hans wrote: > On 2015/09/14 19:29:23, bcwhite wrote: > > Don! I *hate* printf! Odd that MSVC works just fine. > > It mostly works by luck though. MSVC chooses to support passing non-PODs through > varargs (that's ok, C++11 says it's optional and implementation defined), so it > will push the std::wstring on the stack. Luckily, the pointer to the data is the > first member of std::wstring, so that gets pushed on the stack first. > > > Please check my fix > > Looks right, just some misplaced parentheses on the first one. > > > since I don't know how to force a clang compile (pointer?). > > We don't have any trybots for Windows Clang unfortunately. Most of our bots are > here: http://build.chromium.org/p/chromium.fyi/console?category=win%20clang Fixed, thanks. Is there documentation on how to do a clang build on my own workstation?
lgtm! On 2015/09/14 20:06:39, bcwhite wrote: > Fixed, thanks. Is there documentation on how to do a clang build on my own > workstation? Yes, see https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/h20a68y8Q_Q...
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1271893003/#ps300001 (title: "fix parethesis")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271893003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271893003/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/30198c1e69bc8c91e4e0853fa399702b8bddb302 Cr-Commit-Position: refs/heads/master@{#348732}
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/0d3e644a25802bdb3afa7fc09eff5aab456c5733 Cr-Commit-Position: refs/heads/master@{#348556}
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/30198c1e69bc8c91e4e0853fa399702b8bddb302 Cr-Commit-Position: refs/heads/master@{#348732} |
