Add installer::SetupSingleton.

chrome/installer/setup/setup_singleton.cc

Index: chrome/installer/setup/setup_singleton.cc
diff --git a/chrome/installer/setup/setup_singleton.cc b/chrome/installer/setup/setup_singleton.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a8ab80286ae40e28ec81df384d6f62fa03f68d1a
--- /dev/null
+++ b/chrome/installer/setup/setup_singleton.cc
@@ -0,0 +1,89 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/installer/setup/setup_singleton.h"
+
+#include <Windows.h>
+
+#include <algorithm>
+
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/strings/string16.h"
+#include "base/time/time.h"
+#include "chrome/installer/util/installer_state.h"
+
+namespace installer {
+
+namespace {
+
+base::string16 ReplaceBackslashes(const base::string16& path) {
+  base::string16 path_without_backslashes(path);
+  std::replace(path_without_backslashes.begin(), path_without_backslashes.end(),
+               L'\\', L'/');
+  return path_without_backslashes;
+}
+
+}  // namespace
+
+SetupSingleton::SetupSingleton(const InstallerState& installer_state)

[grt (UTC plus 2), 2016/08/31 12:03:47]

perhaps this should take InstallerState and InstallationState as inputs and
refresh them as described in my other comment once the singleton is acquired.

[fdoray, 2016/08/31 19:14:21]

Done.

+    : SetupSingleton(installer_state.target_path(),
+                     installer_state.system_install()) {}
+
+SetupSingleton::SetupSingleton(const base::FilePath& install_dir,
+                               bool system_install) {
+  // Create mutexes and event.
+  const base::string16 name_prefix = system_install ? L"Global\\" : L"Local\\";

[grt (UTC plus 2), 2016/08/31 12:03:47]

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.

[fdoray, 2016/08/31 19:14:21]

Done.

+  const base::string16 install_dir_without_backslashes(
+      ReplaceBackslashes(install_dir.value()));
+
+  const base::string16 setup_mutex_name =
+      name_prefix + L"ChromeSetupMutex_" + install_dir_without_backslashes;
+  setup_mutex_.Set(::CreateMutex(nullptr, FALSE, setup_mutex_name.c_str()));

[grt (UTC plus 2), 2016/08/31 12:03:47]

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.

[fdoray, 2016/08/31 19:14:21]

Done.

+  DCHECK(setup_mutex_.IsValid());
+
+  const base::string16 exit_event_name =

[grt (UTC plus 2), 2016/08/31 12:03:47]

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)?

[fdoray, 2016/08/31 19:14:21]

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;
  }
}

+      name_prefix + L"ChromeSetupExitEvent_" + install_dir_without_backslashes;
+  exit_event_.reset(new base::WaitableEvent(base::win::ScopedHandle(
+      ::CreateEvent(nullptr, TRUE, FALSE, exit_event_name.c_str()))));
+
+  // The exit mutex must be acquired before signaling |exit_event_| and released
+  // after acquiring |setup_mutex_|. It ensures that a single SetupSingleton
+  // signals the exit event and waits for |setup_mutex_| to be released at a
+  // time.
+  const base::string16 exit_event_mutex_name = name_prefix +
+                                               L"ChromeSetupExitEventMutex_" +
+                                               install_dir_without_backslashes;
+  base::win::ScopedHandle exit_event_mutex(
+      ::CreateMutex(nullptr, FALSE, exit_event_mutex_name.c_str()));
+  DCHECK(exit_event_mutex.IsValid());
+
+  // Ask existing SetupSingletons to release |setup_mutex_| as soon as possible.
+  const DWORD wait_exit_event_mutex_return_value =
+      ::WaitForSingleObject(exit_event_mutex.Get(), INFINITE);
+  DCHECK(wait_exit_event_mutex_return_value == WAIT_ABANDONED ||
+         wait_exit_event_mutex_return_value == WAIT_OBJECT_0);
+  exit_event_->Signal();
+
+  // Acquire |setup_mutex_|.
+  const DWORD wait_setup_mutex_return_value =
+      ::WaitForSingleObject(setup_mutex_.Get(), INFINITE);

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

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:47]

That's not so good.
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.
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.

[fdoray, 2016/08/31 19:14:21]

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.

+  DCHECK(wait_setup_mutex_return_value == WAIT_ABANDONED ||
+         wait_setup_mutex_return_value == WAIT_OBJECT_0);
+  exit_event_->Reset();
+  const BOOL release_mutex_return_value =
+      ::ReleaseMutex(exit_event_mutex.Get());
+  DCHECK(release_mutex_return_value);
+}
+
+SetupSingleton::~SetupSingleton() {
+  ::ReleaseMutex(setup_mutex_.Get());

[grt (UTC plus 2), 2016/08/31 12:03:47]

how about using base::ScopedGeneric for auto-releasing a mutex?

[fdoray, 2016/08/31 19:14:21]

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

+}
+
+bool SetupSingleton::Wait(const base::TimeDelta& max_time) {
+  const bool exit_event_signaled = exit_event_->TimedWait(max_time);
+  return exit_event_signaled;
+}
+
+}  // namespace installer