Add installer::SetupSingleton.

Add installer::SetupSingleton.

Any modification to a Chrome installation should be done within the
scope of a SetupSingleton. There can be only one active SetupSingleton
per Chrome installation at a time.

SetupSingleton will be used to ensure that an update process doesn't
run at the same time as a process deleting old files.

BUG=310211
Committed: https://crrev.com/d9a53005086b3f45b5ed3cde83e7453b73c03b79
Cr-Commit-Position: refs/heads/master@{#417051}

[fdoray, 2016-08-30 20:07:52]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-08-30 20:07:53]

PTAL

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:71:
::WaitForSingleObject(setup_mutex_.Get(), INFINITE);
This could block forever if a process hangs while holding SetupSingleton.

Should we add a timeout after which we try to kill the process holding the
SetupSingleton?

Finding out which process holds the SetupSingleton isn't trivial (enumerating
processes running from the install directory doesn't work because setup.exe
sometimes run from a Temp directory). Maybe SetupSingleton could write the
current process id in a file after acquiring |setup_mutex_|?

[grt (UTC plus 2), 2016-08-31 12:03:48]

cool!

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:1754: InstallationState original_state;
many operations in the installer inspect |original_state| to see what they need
to do. if an operation blocks on the singleton while some other instance does
some work, |original_state| needs to be refreshed once the singleton is acquired
to reflect the new state of the machine. since |installer_state| builds up its
mental model partly based on |original_state|, it needs to be refreshed as well.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:30:
SetupSingleton::SetupSingleton(const InstallerState& installer_state)
perhaps this should take InstallerState and InstallationState as inputs and
refresh them as described in my other comment once the singleton is acquired.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:37: const base::string16 name_prefix =
system_install ? L"Global\\" : L"Local\\";
while i'm not sure if/how a user could be logged on to multiple sessions, i
don't see a reason not to use "Global\\" unconditionally just on the off chance
that two per-user installers could operate for the same user in different
sessions.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:43:
setup_mutex_.Set(::CreateMutex(nullptr, FALSE, setup_mutex_name.c_str()));
a mutex's name is limited to MAX_PATH chars. to avoid running into it (crazy
things happen out there in the wild world), i suggest hashing
install_dir_without_backslashes in some form.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:46: const base::string16
exit_event_name =
is the exit event only used for testing? if so, could you move it somehow into a
subclass within the unittest (e.g., by providing empty virtual methods in the
product implementation that are overridden in the test to do the event stuff)?

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:71:
::WaitForSingleObject(setup_mutex_.Get(), INFINITE);
On 2016/08/30 20:07:53, fdoray wrote:
> This could block forever if a process hangs while holding SetupSingleton.

That's not so good.

> Should we add a timeout after which we try to kill the process holding the
> SetupSingleton?

Let's take a more conservative approach to begin with. How about simply aborting
the wait and letting the caller know that they failed to get the mutex and
should exit pronto. We can use UMA and Google Update results to see the extent
to which this is happening.

> Finding out which process holds the SetupSingleton isn't trivial (enumerating
> processes running from the install directory doesn't work because setup.exe
> sometimes run from a Temp directory). Maybe SetupSingleton could write the
> current process id in a file after acquiring |setup_mutex_|?

A named shared memory segment is even betters; see
https://chromium.googlesource.com/chromium/src/+/a9f74a6b78ecfb8f868d19d99e43....
That is a little more complicated than you need here since it needed to allow
low-integrity procs to read from the segment. Your case doesn't. I believe
you'll need to write both the PID and the process creation time to uniquely
identify the holder.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:81:
::ReleaseMutex(setup_mutex_.Get());
how about using base::ScopedGeneric for auto-releasing a mutex?

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton_unittest.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:7: #include <Windows.h>
nit: 'w'

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:35: constexpr int
kWaitDurationMs = 50;
can you get rid of this and use something in base/test/test_timeouts.h?

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:46:
std::replace(path_without_backslashes.begin(), path_without_backslashes.end(),
#include <algorithm>

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:55: base::File
file(sentinel_file_path,
nit: no need to keep this in a local var. you can replace lines 55-59 with:
  if (!base::File(...).IsValid()) {
    return SENTINEL_FILE_CREATE_ERROR;
  }

alternatively, can you open the file with DELETE_ON_CLOSE and just let the File
go out of scope after the wait?

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:81: // fail it the sentinel
file already exists (which shouldn't be the case since
it -> if

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:147:
::testing::Values(InstallerState::SYSTEM_LEVEL,
i find that the logs (especially failures) are easier to understand if there are
two instantiations, each with one level. for example:

INSTANTIATE_TEST_CASE_P(SystemLevel, SetupSingletonTest,
::testing::Values(InstallerState::SYSTEM_LEVEL));
INSTANTIATE_TEST_CASE_P(UserLevel, SetupSingletonTest,
::testing::Values(InstallerState::USER_LEVEL));

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:153: constexpr size_t
kNumProcesses = 10;
nit: use int for this

[fdoray, 2016-08-31 19:14:22]

PTAnL

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:1754: InstallationState original_state;
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> many operations in the installer inspect |original_state| to see what they
need
> to do. if an operation blocks on the singleton while some other instance does
> some work, |original_state| needs to be refreshed once the singleton is
acquired
> to reflect the new state of the machine. since |installer_state| builds up its
> mental model partly based on |original_state|, it needs to be refreshed as
well.

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:30:
SetupSingleton::SetupSingleton(const InstallerState& installer_state)
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> perhaps this should take InstallerState and InstallationState as inputs and
> refresh them as described in my other comment once the singleton is acquired.

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:37: const base::string16 name_prefix =
system_install ? L"Global\\" : L"Local\\";
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> while i'm not sure if/how a user could be logged on to multiple sessions, i
> don't see a reason not to use "Global\\" unconditionally just on the off
chance
> that two per-user installers could operate for the same user in different
> sessions.

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:43:
setup_mutex_.Set(::CreateMutex(nullptr, FALSE, setup_mutex_name.c_str()));
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> a mutex's name is limited to MAX_PATH chars. to avoid running into it (crazy
> things happen out there in the wild world), i suggest hashing
> install_dir_without_backslashes in some form.

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:46: const base::string16
exit_event_name =
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> is the exit event only used for testing? if so, could you move it somehow into
a
> subclass within the unittest (e.g., by providing empty virtual methods in the
> product implementation that are overridden in the test to do the event stuff)?

No. The cleanup process will call Wait() between attempts to delete old files.
When another process tries to acquire the SetupSingleton, the exit event will be
signaled. The Wait() will return and the cleanup process will exit.

while (!DeleteOldVersions()) {
  const bool should_exit = setup_singleton.Wait(TimeDelta::FromMinutes(5));
  if (should_exit) {
    // Another process tries to acquire a SetupSingleton.
    // It's time to exit the cleanup process!
    return;
  }
}

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:71:
::WaitForSingleObject(setup_mutex_.Get(), INFINITE);
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> On 2016/08/30 20:07:53, fdoray wrote:
> > This could block forever if a process hangs while holding SetupSingleton.
> 
> That's not so good.
> 
> > Should we add a timeout after which we try to kill the process holding the
> > SetupSingleton?
> 
> Let's take a more conservative approach to begin with. How about simply
aborting
> the wait and letting the caller know that they failed to get the mutex and
> should exit pronto. We can use UMA and Google Update results to see the extent
> to which this is happening.
> 
> > Finding out which process holds the SetupSingleton isn't trivial
(enumerating
> > processes running from the install directory doesn't work because setup.exe
> > sometimes run from a Temp directory). Maybe SetupSingleton could write the
> > current process id in a file after acquiring |setup_mutex_|?
> 
> A named shared memory segment is even betters; see
>
https://chromium.googlesource.com/chromium/src/+/a9f74a6b78ecfb8f868d19d99e43....
> That is a little more complicated than you need here since it needed to allow
> low-integrity procs to read from the segment. Your case doesn't. I believe
> you'll need to write both the PID and the process creation time to uniquely
> identify the holder.

setup.exe now returns SETUP_SINGLETON_ACQUISITION_FAILED when it waits for more
than 30 seconds on a mutex. I want a timeout that is long enough to allow a
cleanup process to exit when another process tries to acquire the
SetupSingleton. At the same time, I don't want to wait too long before returning
SETUP_SINGLETON_ACQUISITION_FAILED if the process holding the SetupSingleton is
unlikely to release it soon. Note that since an update takes much more than 30
seconds, it is expected that a setup.exe process launched when an update is in
progress will return SETUP_SINGLETON_ACQUISITION_FAILED.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:81:
::ReleaseMutex(setup_mutex_.Get());
On 2016/08/31 12:03:47, grt (UTC plus 2) wrote:
> how about using base::ScopedGeneric for auto-releasing a mutex?

Having a class that takes care of acquiring + auto-releasing the mutex seemed
better than ScopedGeneric which can only take care of auto-releasing.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton_unittest.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:7: #include <Windows.h>
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> nit: 'w'

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:35: constexpr int
kWaitDurationMs = 50;
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> can you get rid of this and use something in base/test/test_timeouts.h?

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:46:
std::replace(path_without_backslashes.begin(), path_without_backslashes.end(),
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> #include <algorithm>

N/A

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:55: base::File
file(sentinel_file_path,
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> nit: no need to keep this in a local var. you can replace lines 55-59 with:
>   if (!base::File(...).IsValid()) {
>     return SENTINEL_FILE_CREATE_ERROR;
>   }
> 
> alternatively, can you open the file with DELETE_ON_CLOSE and just let the
File
> go out of scope after the wait?

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:81: // fail it the sentinel
file already exists (which shouldn't be the case since
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> it -> if

Done.

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:147:
::testing::Values(InstallerState::SYSTEM_LEVEL,
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> i find that the logs (especially failures) are easier to understand if there
are
> two instantiations, each with one level. for example:
> 
> INSTANTIATE_TEST_CASE_P(SystemLevel, SetupSingletonTest,
> ::testing::Values(InstallerState::SYSTEM_LEVEL));
> INSTANTIATE_TEST_CASE_P(UserLevel, SetupSingletonTest,
> ::testing::Values(InstallerState::USER_LEVEL));

N/A

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:153: constexpr size_t
kNumProcesses = 10;
On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> nit: use int for this

Done.

[grt (UTC plus 2), 2016-09-01 12:05:40]

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton_unittest.cc (right):

https://codereview.chromium.org/2292293002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton_unittest.cc:7: #include <Windows.h>
On 2016/08/31 19:14:21, fdoray wrote:
> On 2016/08/31 12:03:48, grt (UTC plus 2) wrote:
> > nit: 'w'
> 
> Done.

try harder. :-)

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1858: return
installer::SETUP_SINGLETON_ACQUISITION_FAILED;
please use WriteInstallerResult as well so that this error is reported through
Google Update's installer result API properly. i think there should also be a UI
string so that users see something like "Another operation on Google Chrome is
in progress; please try again later."

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:41: // Signal |exit_event_|. This
causes any call to Wait() on a SetupSingleton
this comment applies to line 48, no? please move it down there.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:44: std::unique_ptr<ScopedHoldMutex>
scoped_hold_exit_event_mutex(
rather than using a unique_ptr that you release() below, how about putting this
in its own block?
    {
      ScopedHoldMutex scoped_hold_exit_event_mutex;
      if (!scoped_hold_exit_event_mutex.Acquire(...))
        return nullptr;
       ...
    }

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:58: // Update |installer_state| and
|original_state|.
nit: swap these in the comment so that they match the order in the code.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:67:
::ReleaseMutex(setup_mutex_.Get());
remove this? doesn't scoped_hold_setup_mutex_ auto-release?

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:87: mutex,
base::saturated_cast<DWORD>(
since you control the input, i think a static_cast is fine here. saturated_cast
is better for cases where the input is from an untrusted source.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:95:
DCHECK_EQ(static_cast<DWORD>(WAIT_TIMEOUT), wait_return_value);
if you change this this to:
  DPCHECK(wait_return_value != WAIT_FAILED);
then the logging magic will include a friendly string for the last-error code.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.h (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:33: // Returns a SetupSingleton which,
throughout its lifetime, gives to the
language nit "to the" -> "the"

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:37: // updated using |command_line| and
 just before this returns in case the
"and |master_preferences|"?

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:37: // updated using |command_line| and
 just before this returns in case the
maybe change to something like "...updated using ... to reflect the new state of
the installation after acquisition."

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:50: // Waits until |max_time| has
passed or someone tries to acquire a
"someone" -> "another process"

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:51: // SetupSingleton for the same
Chrome installation as this SetupSingleton. In
nit: omit " as this SetupSingleton". i think it's implied.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:55: bool Wait(const base::TimeDelta&
max_time);
how about WaitForInterrupt?

[fdoray, 2016-09-01 15:45:23]

PTAnL

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1858: return
installer::SETUP_SINGLETON_ACQUISITION_FAILED;
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> please use WriteInstallerResult as well so that this error is reported through
> Google Update's installer result API properly. i think there should also be a
UI
> string so that users see something like "Another operation on Google Chrome is
> in progress; please try again later."

Done.

When is the UI string shown? Users should see it when they try to manually
update Chrome but not when a --rename-chrome-exe is launched by Chrome.

Why don't we always write the installer result? e.g. installer::RENAME_FAILED is
not written
https://cs.chromium.org/chromium/src/chrome/installer/setup/setup_main.cc?l=415
Is it ok to write installer::SETUP_SINGLETON_ACQUISITION_FAILED from a
--rename-chrome-exe process like I did above?

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:41: // Signal |exit_event_|. This
causes any call to Wait() on a SetupSingleton
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> this comment applies to line 48, no? please move it down there.

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:44: std::unique_ptr<ScopedHoldMutex>
scoped_hold_exit_event_mutex(
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> rather than using a unique_ptr that you release() below, how about putting
this
> in its own block?
>     {
>       ScopedHoldMutex scoped_hold_exit_event_mutex;
>       if (!scoped_hold_exit_event_mutex.Acquire(...))
>         return nullptr;
>        ...
>     }

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:58: // Update |installer_state| and
|original_state|.
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> nit: swap these in the comment so that they match the order in the code.

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:67:
::ReleaseMutex(setup_mutex_.Get());
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> remove this? doesn't scoped_hold_setup_mutex_ auto-release?

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:87: mutex,
base::saturated_cast<DWORD>(
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> since you control the input, i think a static_cast is fine here.
saturated_cast
> is better for cases where the input is from an untrusted source.

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:95:
DCHECK_EQ(static_cast<DWORD>(WAIT_TIMEOUT), wait_return_value);
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> if you change this this to:
>   DPCHECK(wait_return_value != WAIT_FAILED);
> then the logging magic will include a friendly string for the last-error code.

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.h (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:33: // Returns a SetupSingleton which,
throughout its lifetime, gives to the
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> language nit "to the" -> "the"

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:37: // updated using |command_line| and
 just before this returns in case the
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> "and |master_preferences|"?

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:37: // updated using |command_line| and
 just before this returns in case the
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> maybe change to something like "...updated using ... to reflect the new state
of
> the installation after acquisition."

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:50: // Waits until |max_time| has
passed or someone tries to acquire a
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> "someone" -> "another process"

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:51: // SetupSingleton for the same
Chrome installation as this SetupSingleton. In
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> nit: omit " as this SetupSingleton". i think it's implied.

Done.

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.h:55: bool Wait(const base::TimeDelta&
max_time);
On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> how about WaitForInterrupt?

Done.

[grt (UTC plus 2), 2016-09-01 21:22:42]

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1858: return
installer::SETUP_SINGLETON_ACQUISITION_FAILED;
On 2016/09/01 15:45:22, fdoray wrote:
> On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> > please use WriteInstallerResult as well so that this error is reported
through
> > Google Update's installer result API properly. i think there should also be
a
> UI
> > string so that users see something like "Another operation on Google Chrome
is
> > in progress; please try again later."
> 
> Done.
> 
> When is the UI string shown?

Google Update will show the string if the user is using an installer from the
dlapge (including a .msi) for either a same-version repair or an overinstall.
Chrome will show it in chrome://help if the user is doing an on-demand update.

> Users should see it when they try to manually
> update Chrome but not when a --rename-chrome-exe is launched by Chrome.

Exactly. There is no UX for the rename dance.

> Why don't we always write the installer result? e.g. installer::RENAME_FAILED
is
> not written
>
https://cs.chromium.org/chromium/src/chrome/installer/setup/setup_main.cc?l=415
> Is it ok to write installer::SETUP_SINGLETON_ACQUISITION_FAILED from a
> --rename-chrome-exe process like I did above?

No, don't do that. :-)  It only makes sense to write into the Google Update
"installer result api" values when the installer is being run by Google Update.
The rename dance is purely within Chrome. UMA is wonderful, though, so how about
a histogram for the timeout case there?

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:85:
static_cast<DWORD>(base::TimeDelta::FromSeconds(1).InMilliseconds()));
did you mean to change this to 1 second?

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:82: // binaries will be uninstalled if
they are not
gah, this is ugly, please ignore "git cl format" and make this pretty again!

[fdoray, 2016-09-06 17:29:39]

PTAnL

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2292293002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1858: return
installer::SETUP_SINGLETON_ACQUISITION_FAILED;
On 2016/09/01 21:22:41, grt (UTC plus 2) wrote:
> On 2016/09/01 15:45:22, fdoray wrote:
> > On 2016/09/01 12:05:39, grt (UTC plus 2) wrote:
> > > please use WriteInstallerResult as well so that this error is reported
> through
> > > Google Update's installer result API properly. i think there should also
be
> a
> > UI
> > > string so that users see something like "Another operation on Google
Chrome
> is
> > > in progress; please try again later."
> > 
> > Done.
> > 
> > When is the UI string shown?
> 
> Google Update will show the string if the user is using an installer from the
> dlapge (including a .msi) for either a same-version repair or an overinstall.
> Chrome will show it in chrome://help if the user is doing an on-demand update.
> 
> > Users should see it when they try to manually
> > update Chrome but not when a --rename-chrome-exe is launched by Chrome.
> 
> Exactly. There is no UX for the rename dance.
> 
> > Why don't we always write the installer result? e.g.
installer::RENAME_FAILED
> is
> > not written
> >
>
https://cs.chromium.org/chromium/src/chrome/installer/setup/setup_main.cc?l=415
> > Is it ok to write installer::SETUP_SINGLETON_ACQUISITION_FAILED from a
> > --rename-chrome-exe process like I did above?
> 
> No, don't do that. :-)  It only makes sense to write into the Google Update
> "installer result api" values when the installer is being run by Google
Update.
> The rename dance is purely within Chrome. UMA is wonderful, though, so how
about
> a histogram for the timeout case there?

Removed WriteInstallerResult for the --rename-chrome-exe case. Added UMA
histogram in setup_singleton.cc

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:85:
static_cast<DWORD>(base::TimeDelta::FromSeconds(1).InMilliseconds()));
On 2016/09/01 21:22:41, grt (UTC plus 2) wrote:
> did you mean to change this to 1 second?

Yes. I think we should wait long enough to allow a cleanup process to release
the ChromeSetupMutex (after having been woken up by the ChromeSetupExitEvent),
but not more than that. 30 seconds is probably too much time for that but 1
second might not be enough for cases where the machine is under a heavy load.
I'll try 5 seconds and adjust if necessary when we get the failure rate from
UMA.

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:82: // binaries will be uninstalled if
they are not
On 2016/09/01 21:22:41, grt (UTC plus 2) wrote:
> gah, this is ugly, please ignore "git cl format" and make this pretty again!

Done.

[grt (UTC plus 2), 2016-09-06 20:22:11]

nice! lgtm with nits.

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_singleton.cc:85:
static_cast<DWORD>(base::TimeDelta::FromSeconds(1).InMilliseconds()));
On 2016/09/06 17:29:39, fdoray wrote:
> On 2016/09/01 21:22:41, grt (UTC plus 2) wrote:
> > did you mean to change this to 1 second?
> 
> Yes. I think we should wait long enough to allow a cleanup process to release
> the ChromeSetupMutex (after having been woken up by the ChromeSetupExitEvent),
> but not more than that. 30 seconds is probably too much time for that but 1
> second might not be enough for cases where the machine is under a heavy load.
> I'll try 5 seconds and adjust if necessary when we get the failure rate from
> UMA. 

sgtm

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton.cc:31:
UMA_HISTOGRAM_ENUMERATION("Installer.SetupSingletonAcquisitionResult", result,
nit: other installer histograms are called Setup.Install.FooBarBlech, please
follow suit.

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
File chrome/installer/setup/setup_singleton_unittest.cc (right):

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton_unittest.cc:33: constexpr base::char16
kSentinelFileName[] = L"sentinel.txt";
constexpr base::FilePath::CharType kSentinelFileName[] =
FILE_PATH_LITERAL("sentinel.txt");

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton_unittest.cc:50:
std::hash<base::FilePath::StringType>()(path.value()));
#include <functional>

[fdoray, 2016-09-06 20:49:53]

fdoray@chromium.org changed reviewers:
+ asvitkine@chromium.org

[fdoray, 2016-09-06 20:49:54]

asvitkine@: Please review changes in histograms.xml

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton.cc:31:
UMA_HISTOGRAM_ENUMERATION("Installer.SetupSingletonAcquisitionResult", result,
On 2016/09/06 20:22:11, grt (UTC plus 2) wrote:
> nit: other installer histograms are called Setup.Install.FooBarBlech, please
> follow suit.

Done.

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
File chrome/installer/setup/setup_singleton_unittest.cc (right):

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton_unittest.cc:33: constexpr base::char16
kSentinelFileName[] = L"sentinel.txt";
On 2016/09/06 20:22:11, grt (UTC plus 2) wrote:
> constexpr base::FilePath::CharType kSentinelFileName[] =
> FILE_PATH_LITERAL("sentinel.txt");

Done.

https://codereview.chromium.org/2292293002/diff/140001/chrome/installer/setup...
chrome/installer/setup/setup_singleton_unittest.cc:50:
std::hash<base::FilePath::StringType>()(path.value()));
On 2016/09/06 20:22:11, grt (UTC plus 2) wrote:
> #include <functional>

Done.

[Alexei Svitkine (slow), 2016-09-07 18:45:10]

lgtm

[fdoray, 2016-09-07 18:50:51]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-09-07 18:50:51]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2292293002/#ps160001
(title: "CR grt #9")

[commit-bot: I haz the power, 2016-09-07 18:51:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 21:42:34]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-07 21:44:54]

Description was changed from

==========
Add installer::SetupSingleton.

Any modification to a Chrome installation should be done within the
scope of a SetupSingleton. There can be only one active SetupSingleton
per Chrome installation at a time.

SetupSingleton will be used to ensure that an update process doesn't
run at the same time as a process deleting old files.

BUG=310211
==========

to

==========
Add installer::SetupSingleton.

Any modification to a Chrome installation should be done within the
scope of a SetupSingleton. There can be only one active SetupSingleton
per Chrome installation at a time.

SetupSingleton will be used to ensure that an update process doesn't
run at the same time as a process deleting old files.

BUG=310211

Committed: https://crrev.com/d9a53005086b3f45b5ed3cde83e7453b73c03b79
Cr-Commit-Position: refs/heads/master@{#417051}
==========

[commit-bot: I haz the power, 2016-09-07 21:44:55]

Patchset 9 (id:??) landed as
https://crrev.com/d9a53005086b3f45b5ed3cde83e7453b73c03b79
Cr-Commit-Position: refs/heads/master@{#417051}