Skip to content

Commit

Permalink
Crashpad!: Use the Crashpad client instead of Breakpad on Mac OS X.
Browse files Browse the repository at this point in the history
Crashpad is always compiled in to Chrome and its handler is always
enabled. It is only possible to enable uploads in official builds.

Crashpad talks to the existing Breakpad server. There should be no
noticeable changes to crash reporting on the server side, except the
client IDs will all change to a new ID and will no longer be
synchronized with UMA client IDs. This is a one-time change. After this,
the client ID will remain stable within a single --user-data-dir.

BUG=386208,390217,415547,427611,crashpad:12
R=rsesek@chromium.org
TBR=cpu@chromium.org,jochen@chromium.org

Review URL: https://codereview.chromium.org/1001103002

Cr-Commit-Position: refs/heads/master@{#320466}
  • Loading branch information
markmentovai authored and Commit bot committed Mar 13, 2015
1 parent ec163e2 commit d413b2d
Show file tree
Hide file tree
Showing 33 changed files with 551 additions and 148 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ v8.log
/third_party/chromite
/third_party/cld_2/src
/third_party/colorama/src
/third_party/crashpad/crashpad
/third_party/cros
/third_party/cros_system_api
/third_party/cygwin
Expand Down
3 changes: 3 additions & 0 deletions DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ deps = {
'src/third_party/colorama/src':
Var('chromium_git') + '/external/colorama.git' + '@' + '799604a1041e9b3bc5d2789ecbd7e8db2e18e6b8',

'src/third_party/crashpad/crashpad':
Var('chromium_git') + '/crashpad/crashpad.git' + '@' + '58c7519598b39f58ec2ff1a821ace113f523b0fa',

'src/third_party/trace-viewer':
Var('chromium_git') + '/external/trace-viewer.git' + '@' + 'b3571f53f1bd7870ffd293a4bfca6473a21193d2',

Expand Down
6 changes: 3 additions & 3 deletions base/mac/scoped_mach_port.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ struct BASE_EXPORT SendRightTraits {
return MACH_PORT_NULL;
}

static void Free(mach_port_t port);
BASE_EXPORT static void Free(mach_port_t port);
};

struct BASE_EXPORT ReceiveRightTraits {
static mach_port_t InvalidValue() {
return MACH_PORT_NULL;
}

static void Free(mach_port_t port);
BASE_EXPORT static void Free(mach_port_t port);
};

struct PortSetTraits {
static mach_port_t InvalidValue() {
return MACH_PORT_NULL;
}

static void Free(mach_port_t port);
BASE_EXPORT static void Free(mach_port_t port);
};

} // namespace internal
Expand Down
1 change: 1 addition & 0 deletions build/all.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@
['OS=="mac"', {
'dependencies': [
'../sandbox/sandbox.gyp:*',
'../third_party/crashpad/crashpad/crashpad.gyp:*',
'../third_party/ocmock/ocmock.gyp:*',
],
}],
Expand Down
4 changes: 0 additions & 4 deletions chrome/app/chrome_crash_reporter_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ class ChromeCrashReporterClient : public crash_reporter::CrashReporterClient {
int GetAndroidMinidumpDescriptor() override;
#endif

#if defined(OS_MACOSX)
void InstallAdditionalFilters(BreakpadRef breakpad) override;
#endif

bool EnableBreakpadForProcess(const std::string& process_type) override;

private:
Expand Down
23 changes: 0 additions & 23 deletions chrome/app/chrome_crash_reporter_client_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,13 @@

#if !defined(DISABLE_NACL)
#include "base/command_line.h"
#import "breakpad/src/client/mac/Framework/Breakpad.h"
#include "chrome/common/chrome_switches.h"
#include "components/nacl/common/nacl_switches.h"
#include "native_client/src/trusted/service_runtime/osx/crash_filter.h"
#endif

namespace chrome {

namespace {

#if !defined(DISABLE_NACL)
bool NaClBreakpadCrashFilter(int exception_type,
int exception_code,
mach_port_t crashing_thread,
void* context) {
return !NaClMachThreadIsInUntrusted(crashing_thread);
}
#endif

} // namespace

void ChromeCrashReporterClient::InstallAdditionalFilters(BreakpadRef breakpad) {
#if !defined(DISABLE_NACL)
if (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
switches::kProcessType) == switches::kNaClLoaderProcess) {
BreakpadSetFilterCallback(breakpad, NaClBreakpadCrashFilter, NULL);
}
#endif
}

bool ChromeCrashReporterClient::ReportingIsEnforcedByPolicy(
bool* breakpad_enabled) {
base::ScopedCFTypeRef<CFStringRef> key(
Expand Down
44 changes: 6 additions & 38 deletions chrome/app/chrome_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,11 @@

#if defined(OS_MACOSX)
#include "base/mac/foundation_util.h"
#include "base/mac/os_crash_dumps.h"
#include "chrome/app/chrome_main_mac.h"
#include "chrome/browser/mac/relauncher.h"
#include "chrome/common/mac/cfbundle_blocker.h"
#include "chrome/common/mac/objc_zombie.h"
#include "components/crash/app/breakpad_mac.h"
#include "components/crash/app/crashpad_mac.h"
#include "ui/base/l10n/l10n_util_mac.h"
#endif

Expand Down Expand Up @@ -564,39 +563,11 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
void ChromeMainDelegate::InitMacCrashReporter(
const base::CommandLine& command_line,
const std::string& process_type) {
// TODO(mark): Right now, InitCrashReporter() needs to be called after
// CommandLine::Init() and chrome::RegisterPathProvider(). Ideally,
// Breakpad initialization could occur sooner, preferably even before the
// framework dylib is even loaded, to catch potential early crashes.
breakpad::InitCrashReporter(process_type);

#if defined(NDEBUG)
bool is_debug_build = false;
#else
bool is_debug_build = true;
#endif

// Details on when we enable Apple's Crash reporter.
//
// Motivation:
// In debug mode it takes Apple's crash reporter eons to generate a crash
// dump.
//
// What we do:
// * We only pass crashes for foreground processes to Apple's Crash
// reporter. At the time of this writing, that means just the Browser
// process.
// * If Breakpad is enabled, it will pass browser crashes to Crash Reporter
// itself.
// * If Breakpad is disabled, we only turn on Crash Reporter for the
// Browser process in release mode.
if (!command_line.HasSwitch(switches::kDisableBreakpad)) {
bool disable_apple_crash_reporter = is_debug_build ||
base::mac::IsBackgroundOnlyProcess();
if (!breakpad::IsCrashReporterEnabled() && disable_apple_crash_reporter) {
base::mac::DisableOSCrashDumps();
}
}
// TODO(mark): Right now, InitializeCrashpad() needs to be called after
// CommandLine::Init() and chrome::RegisterPathProvider(). Ideally, Crashpad
// initialization could occur sooner, preferably even before the framework
// dylib is even loaded, to catch potential early crashes.
crash_reporter::InitializeCrashpad(process_type);

// Mac Chrome is packaged with a main app bundle and a helper app bundle.
// The main app bundle should only be used for the browser process, so it
Expand Down Expand Up @@ -651,9 +622,6 @@ void ChromeMainDelegate::InitMacCrashReporter(
process_type.empty())
<< "Main application forbids --type, saw " << process_type;
}

if (breakpad::IsCrashReporterEnabled())
breakpad::InitCrashProcessInfo(process_type);
}
#endif // defined(OS_MACOSX)

Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/chrome_browser_main_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include "chrome/browser/ui/app_list/app_list_service.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h"
#include "components/crash/app/breakpad_mac.h"
#include "components/crash/app/crashpad_mac.h"
#include "components/metrics/metrics_service.h"
#include "content/public/common/main_function_params.h"
#include "content/public/common/result_codes.h"
Expand Down Expand Up @@ -292,8 +292,9 @@ void RecordCatSixtyFour() {
MacStartupProfiler::GetInstance()->Profile(
MacStartupProfiler::POST_PROFILE_INIT);
ChromeBrowserMainPartsPosix::PostProfileInit();

g_browser_process->metrics_service()->RecordBreakpadRegistration(
breakpad::IsCrashReporterEnabled());
crash_reporter::GetUploadsEnabled());
}

void ChromeBrowserMainPartsMac::DidEndMainMessageLoop() {
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/chrome_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@
#elif defined(OS_MACOSX)
#include "chrome/browser/chrome_browser_main_mac.h"
#include "chrome/browser/spellchecker/spellcheck_message_filter_mac.h"
#include "components/crash/app/breakpad_mac.h"
#elif defined(OS_CHROMEOS)
#include "chrome/browser/chromeos/chrome_browser_main_chromeos.h"
#include "chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.h"
Expand Down Expand Up @@ -1301,15 +1300,15 @@ bool IsAutoReloadVisibleOnlyEnabled() {
void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
base::CommandLine* command_line,
int child_process_id) {
#if defined(OS_POSIX)
#if defined(OS_POSIX) && !defined(OS_MACOSX)
if (breakpad::IsCrashReporterEnabled()) {
scoped_ptr<metrics::ClientInfo> client_info =
GoogleUpdateSettings::LoadMetricsClientInfo();
command_line->AppendSwitchASCII(switches::kEnableCrashReporter,
client_info ? client_info->client_id
: std::string());
}
#endif // defined(OS_POSIX)
#endif // defined(OS_POSIX) && !defined(OS_MACOSX)

if (logging::DialogsAreSuppressed())
command_line->AppendSwitch(switches::kNoErrorDialogs);
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/crash_upload_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "chrome/common/chrome_paths.h"
#if defined(OS_WIN)
#include "chrome/browser/crash_upload_list_win.h"
#elif defined(OS_MACOSX)
#include "chrome/browser/crash_upload_list_mac.h"
#endif

// static
Expand All @@ -22,6 +24,8 @@ CrashUploadList* CrashUploadList::Create(Delegate* delegate) {
crash_dir_path.AppendASCII(kReporterLogFilename);
#if defined(OS_WIN)
return new CrashUploadListWin(delegate, upload_log_path);
#elif defined(OS_MACOSX)
return new CrashUploadListMac(delegate, upload_log_path);
#else
return new CrashUploadList(delegate, upload_log_path);
#endif
Expand Down
30 changes: 30 additions & 0 deletions chrome/browser/crash_upload_list_mac.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2015 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/browser/crash_upload_list_mac.h"

#include "base/time/time.h"
#include "components/crash/app/crashpad_mac.h"

CrashUploadListMac::CrashUploadListMac(Delegate* delegate,
const base::FilePath& upload_log_path)
: CrashUploadList(delegate, upload_log_path) {
}

CrashUploadListMac::~CrashUploadListMac() {
}

void CrashUploadListMac::LoadUploadList() {
std::vector<crash_reporter::UploadedReport> uploaded_reports;
crash_reporter::GetUploadedReports(&uploaded_reports);

ClearUploads();
for (const crash_reporter::UploadedReport& uploaded_report :
uploaded_reports) {
AppendUploadInfo(
UploadInfo(uploaded_report.remote_id,
base::Time::FromTimeT(uploaded_report.creation_time),
uploaded_report.local_id));
}
}
31 changes: 31 additions & 0 deletions chrome/browser/crash_upload_list_mac.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright 2015 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.

#ifndef CHROME_BROWSER_CRASH_UPLOAD_LIST_MAC_H_
#define CHROME_BROWSER_CRASH_UPLOAD_LIST_MAC_H_

#include "chrome/browser/crash_upload_list.h"

#include "base/basictypes.h"
#include "base/files/file_path.h"

// A CrashUploadList that retrieves the list of uploaded reports from the
// Crashpad database.
class CrashUploadListMac : public CrashUploadList {
public:
// The |upload_log_path| argument is unused. It is only accepted because the
// base class constructor requires it, although it is entirely unused with
// LoadUploadList() being overridden.
CrashUploadListMac(Delegate* delegate, const base::FilePath& upload_log_path);

protected:
~CrashUploadListMac() override;

// Called on a blocking pool thread.
void LoadUploadList() override;

DISALLOW_COPY_AND_ASSIGN(CrashUploadListMac);
};

#endif // CHROME_BROWSER_CRASH_UPLOAD_LIST_MAC_H_
8 changes: 8 additions & 0 deletions chrome/browser/google/google_update_settings_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
#include "base/synchronization/lock.h"
#include "chrome/common/chrome_paths.h"

#if defined(OS_MACOSX)
#include "components/crash/app/crashpad_mac.h"
#endif

namespace {

base::LazyInstance<std::string>::Leaky g_posix_client_id =
Expand Down Expand Up @@ -58,6 +62,10 @@ bool GoogleUpdateSettings::GetCollectStatsConsent() {

// static
bool GoogleUpdateSettings::SetCollectStatsConsent(bool consented) {
#if defined(OS_MACOSX)
crash_reporter::SetUploadsEnabled(consented);
#endif

base::FilePath consent_dir;
PathService::Get(chrome::DIR_USER_DATA, &consent_dir);
if (!base::DirectoryExists(consent_dir))
Expand Down
9 changes: 0 additions & 9 deletions chrome/browser/ui/cocoa/first_run_dialog.mm
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include "chrome/browser/browser_process.h"
#include "chrome/common/pref_names.h"
#include "chrome/installer/util/google_update_settings.h"
#import "components/crash/app/breakpad_mac.h"
#endif

@interface FirstRunDialogController (PrivateMethods)
Expand Down Expand Up @@ -100,14 +99,6 @@ bool ShowFirstRun(Profile* profile) {
bool stats_enabled = [dialog.get() statsEnabled];
GoogleUpdateSettings::SetCollectStatsConsent(stats_enabled);

// Breakpad is normally enabled very early in the startup process. However,
// on the first run it may not have been enabled due to the missing opt-in
// from the user. If the user agreed now, enable breakpad if necessary.
if (!breakpad::IsCrashReporterEnabled() && stats_enabled) {
breakpad::InitCrashReporter(std::string());
breakpad::InitCrashProcessInfo(std::string());
}

// If selected set as default browser.
BOOL make_default_browser = [dialog.get() makeDefaultBrowser];
if (make_default_browser) {
Expand Down
2 changes: 2 additions & 0 deletions chrome/chrome_browser.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@
'browser/component_updater/swiftshader_component_installer.h',
'browser/crash_upload_list.cc',
'browser/crash_upload_list.h',
'browser/crash_upload_list_mac.cc',
'browser/crash_upload_list_mac.h',
'browser/crash_upload_list_win.cc',
'browser/crash_upload_list_win.h',
'browser/custom_handlers/protocol_handler_registry.cc',
Expand Down
28 changes: 9 additions & 19 deletions chrome/chrome_dll.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,15 @@
'xcode_settings': { 'OTHER_LDFLAGS': [ '-Wl,-ObjC' ], },
}],
['OS=="mac"', {
'dependencies': [
'../components/components.gyp:crash_component',
'../components/components.gyp:policy',
],
'sources': [
'app/chrome_crash_reporter_client.cc',
'app/chrome_crash_reporter_client.h',
'app/chrome_crash_reporter_client_mac.mm',
],
'xcode_settings': {
# Define the order of symbols within the framework. This
# sets -order_file.
Expand All @@ -302,25 +311,6 @@
],
},
],
'conditions': [
['mac_breakpad_compiled_in==1', {
'dependencies': [
'../breakpad/breakpad.gyp:breakpad',
'../components/components.gyp:crash_component',
'../components/components.gyp:policy',
],
'sources': [
'app/chrome_crash_reporter_client.cc',
'app/chrome_crash_reporter_client.h',
'app/chrome_crash_reporter_client_mac.mm',
],
}, { # else: mac_breakpad_compiled_in!=1
# No Breakpad, put in the stubs.
'dependencies': [
'../components/components.gyp:breakpad_stubs',
],
}], # mac_breakpad_compiled_in
], # conditions
}], # OS=="mac"
], # conditions
}, # target chrome_main_dll
Expand Down
Loading

0 comments on commit d413b2d

Please sign in to comment.