Skip to content

Commit

Permalink
Make SetupSingleton resilient to CreateEvent/CreateMutex errors.
Browse files Browse the repository at this point in the history
With this CL, SetupSingleton::Acquire will record an histogram and
return nullptr when CreateEvent or CreateMutex return an invalid
handle. Previously, it crashed.

BUG=650981

Review-Url: https://codereview.chromium.org/2380983003
Cr-Commit-Position: refs/heads/master@{#422096}
  • Loading branch information
fdoray authored and Commit bot committed Sep 30, 2016
1 parent 0168d09 commit 8a7bdb9
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 17 deletions.
54 changes: 40 additions & 14 deletions chrome/installer/setup/setup_singleton.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@

#include "chrome/installer/setup/setup_singleton.h"

#include <functional>
#include <utility>

#include "base/files/file_path.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "base/metrics/histogram_macros.h"
#include "base/strings/string16.h"
#include "base/strings/string_number_conversions.h"
#include "base/time/time.h"
#include "chrome/installer/util/installation_state.h"
Expand All @@ -23,6 +28,12 @@ enum SetupSingletonAcquisitionResult {
SETUP_SINGLETON_ACQUISITION_EXIT_EVENT_MUTEX_TIMEOUT = 1,
// Acquisition of the setup mutex timed out.
SETUP_SINGLETON_ACQUISITION_SETUP_MUTEX_TIMEOUT = 2,
// Creation of the setup mutex failed.
SETUP_SINGLETON_ACQUISITION_SETUP_MUTEX_CREATION_FAILED = 3,
// Creation of the exit event failed.
SETUP_SINGLETON_ACQUISITION_EXIT_EVENT_CREATION_FAILED = 4,
// Creation of the exit event mutex failed.
SETUP_SINGLETON_ACQUISITION_EXIT_EVENT_MUTEX_CREATION_FAILED = 5,
SETUP_SINGLETON_ACQUISITION_RESULT_COUNT,
};

Expand All @@ -46,8 +57,26 @@ std::unique_ptr<SetupSingleton> SetupSingleton::Acquire(
base::SizeTToString16(std::hash<base::FilePath::StringType>()(
installer_state->target_path().value())));

std::unique_ptr<SetupSingleton> setup_singleton(
new SetupSingleton(sync_primitive_name_suffix));
base::win::ScopedHandle setup_mutex(::CreateMutex(
nullptr, FALSE,
(L"Global\\ChromeSetupMutex_" + sync_primitive_name_suffix).c_str()));
if (!setup_mutex.IsValid()) {
RecordSetupSingletonAcquisitionResultHistogram(
SETUP_SINGLETON_ACQUISITION_SETUP_MUTEX_CREATION_FAILED);
return nullptr;
}

base::win::ScopedHandle exit_event(::CreateEvent(
nullptr, TRUE, FALSE,
(L"Global\\ChromeSetupExitEvent_" + sync_primitive_name_suffix).c_str()));
if (!exit_event.IsValid()) {
RecordSetupSingletonAcquisitionResultHistogram(
SETUP_SINGLETON_ACQUISITION_EXIT_EVENT_CREATION_FAILED);
return nullptr;
}

auto setup_singleton = base::WrapUnique(
new SetupSingleton(std::move(setup_mutex), std::move(exit_event)));

{
// Acquire a mutex to ensure that a single call to SetupSingleton::Acquire()
Expand All @@ -57,7 +86,12 @@ std::unique_ptr<SetupSingleton> SetupSingleton::Acquire(
nullptr, FALSE,
(L"Global\\ChromeSetupExitEventMutex_" + sync_primitive_name_suffix)
.c_str()));
DCHECK(exit_event_mutex.IsValid());
if (!exit_event_mutex.IsValid()) {
RecordSetupSingletonAcquisitionResultHistogram(
SETUP_SINGLETON_ACQUISITION_EXIT_EVENT_MUTEX_CREATION_FAILED);
return nullptr;
}

ScopedHoldMutex scoped_hold_exit_event_mutex;
if (!scoped_hold_exit_event_mutex.Acquire(exit_event_mutex.Get())) {
RecordSetupSingletonAcquisitionResultHistogram(
Expand Down Expand Up @@ -121,17 +155,9 @@ bool SetupSingleton::ScopedHoldMutex::Acquire(HANDLE mutex) {
return false;
}

SetupSingleton::SetupSingleton(const base::string16& sync_primitive_name_suffix)
: setup_mutex_(::CreateMutex(
nullptr,
FALSE,
(L"Global\\ChromeSetupMutex_" + sync_primitive_name_suffix).c_str())),
exit_event_(base::win::ScopedHandle(::CreateEvent(
nullptr,
TRUE,
FALSE,
(L"Global\\ChromeSetupExitEvent_" + sync_primitive_name_suffix)
.c_str()))) {
SetupSingleton::SetupSingleton(base::win::ScopedHandle setup_mutex,
base::win::ScopedHandle exit_event)
: setup_mutex_(std::move(setup_mutex)), exit_event_(std::move(exit_event)) {
DCHECK(setup_mutex_.IsValid());
}

Expand Down
5 changes: 2 additions & 3 deletions chrome/installer/setup/setup_singleton.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,8 @@ class SetupSingleton {
DISALLOW_COPY_AND_ASSIGN(ScopedHoldMutex);
};

// |sync_primitive_name_suffix| is a suffix for the name of |setup_mutex_| and
// |exit_event_|.
explicit SetupSingleton(const base::string16& sync_primitive_name_suffix);
SetupSingleton(base::win::ScopedHandle setup_mutex,
base::win::ScopedHandle exit_event);

// A mutex that must be held to modify the Chrome installation directory.
base::win::ScopedHandle setup_mutex_;
Expand Down
3 changes: 3 additions & 0 deletions tools/metrics/histograms/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -96566,6 +96566,9 @@ value.
<int value="0" label="The setup singleton was acquired successfully."/>
<int value="1" label="Acquisition of the exit event mutex timed out."/>
<int value="2" label="Acquisition of the setup mutex timed out."/>
<int value="3" label="Creation of the setup mutex failed."/>
<int value="4" label="Creation of the exit event failed."/>
<int value="5" label="Creation of the exit event mutex failed."/>
</enum>

<enum name="SHA1Status" type="int">
Expand Down

0 comments on commit 8a7bdb9

Please sign in to comment.