From 3d6430df9d723aab2d85ba18a44640ace4938f1a Mon Sep 17 00:00:00 2001 From: Nitish Sakhawalkar Date: Wed, 12 Jun 2019 23:42:21 -0700 Subject: [PATCH] fix: use crashpad on Windows (#18483) --- BUILD.gn | 35 +- DEPS | 2 +- appveyor.yml | 1 - atom/app/atom_main.cc | 32 +- atom/app/atom_main_delegate.cc | 7 + atom/app/node_main.cc | 7 + atom/browser/atom_browser_client.cc | 16 + atom/browser/atom_browser_client.h | 3 + atom/browser/ui/inspectable_web_contents.h | 2 + .../ui/inspectable_web_contents_impl.cc | 15 +- .../ui/inspectable_web_contents_impl.h | 4 + atom/common/api/api.mojom | 2 + atom/common/atom_constants.cc | 8 + atom/common/atom_constants.h | 10 + atom/common/crash_reporter/crash_reporter.cc | 44 ++- atom/common/crash_reporter/crash_reporter.h | 20 +- .../crash_reporter/crash_reporter_crashpad.cc | 118 +++++++ .../crash_reporter/crash_reporter_crashpad.h | 50 +++ .../crash_reporter/crash_reporter_linux.cc | 18 +- .../crash_reporter/crash_reporter_linux.h | 13 +- .../crash_reporter/crash_reporter_mac.h | 39 +- .../crash_reporter/crash_reporter_mac.mm | 124 +------ .../crash_reporter/crash_reporter_win.cc | 332 +++++------------- .../crash_reporter/crash_reporter_win.h | 56 +-- .../crash_reporter/win/crash_service_main.cc | 67 ++-- .../crash_reporter/win/crash_service_main.h | 4 +- atom/renderer/electron_api_service_impl.cc | 10 + atom/renderer/electron_api_service_impl.h | 2 +- docs/api/crash-reporter.md | 31 +- filenames.gni | 4 +- lib/browser/crash-reporter-init.js | 21 -- lib/common/crash-reporter.js | 1 - patches/common/chromium/.patches | 2 +- .../common/chromium/crashpad_pid_check.patch | 38 ++ ...b3be3fc0024d00358d5b7816d3215a8ff20c.patch | 141 -------- spec/api-crash-reporter-spec.js | 78 +--- spec/fixtures/api/crash-restart.html | 6 +- spec/fixtures/api/crash.html | 6 +- spec/static/main.js | 6 - 39 files changed, 562 insertions(+), 813 deletions(-) create mode 100644 atom/common/crash_reporter/crash_reporter_crashpad.cc create mode 100644 atom/common/crash_reporter/crash_reporter_crashpad.h create mode 100644 patches/common/chromium/crashpad_pid_check.patch delete mode 100644 patches/common/chromium/partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch diff --git a/BUILD.gn b/BUILD.gn index 7a217256bdf7f..cefba40e4be42 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -546,21 +546,13 @@ static_library("electron_lib") { if (is_mac) { deps += [ "//components/remote_cocoa/app_shim", - "//third_party/crashpad/crashpad/client", "//ui/accelerated_widget_mac", ] sources += [ "atom/browser/ui/views/autofill_popup_view.cc", "atom/browser/ui/views/autofill_popup_view.h", ] - include_dirs += [ - # NOTE(nornagon): other chromium files use the full path to include - # crashpad; this is just here for compatibility between GN and GYP, so that - # the #includes can be agnostic about where crashpad is vendored. - "//third_party/crashpad", - ] if (is_mas_build) { - deps -= [ "//third_party/crashpad/crashpad/client" ] sources += [ "atom/browser/api/atom_api_app_mas.mm" ] sources -= [ "atom/browser/auto_updater_mac.mm", @@ -590,10 +582,14 @@ static_library("electron_lib") { "//chrome/browser/ui/libgtkui", "//dbus", "//device/bluetooth", + "//third_party/breakpad:client", "//ui/events/devices/x11", "//ui/events/platform/x11", + "//ui/views/controls/webview", + "//ui/wm", ] configs += [ ":gio_unix" ] + include_dirs += [ "//third_party/breakpad" ] defines += [ # Disable warnings for g_settings_list_schemas. "GLIB_DISABLE_DEPRECATION_WARNINGS", @@ -604,20 +600,23 @@ static_library("electron_lib") { if (is_win) { libs += [ "dwmapi.lib" ] deps += [ - "//third_party/breakpad:breakpad_handler", - "//third_party/breakpad:breakpad_sender", "//ui/native_theme:native_theme_browser", + "//ui/views/controls/webview", + "//ui/wm", "//ui/wm/public", ] - public_deps += [ "//sandbox/win:sandbox" ] + public_deps += [ + "//sandbox/win:sandbox", + "//third_party/crashpad/crashpad/handler", + ] } - if (is_linux || is_win) { - deps += [ - "//third_party/breakpad:client", - "//ui/views/controls/webview", - "//ui/wm", + + if ((is_mac && !is_mas_build) || is_win) { + sources += [ + "atom/common/crash_reporter/crash_reporter_crashpad.cc", + "atom/common/crash_reporter/crash_reporter_crashpad.h", ] - include_dirs += [ "//third_party/breakpad" ] + deps += [ "//third_party/crashpad/crashpad/client" ] } if (enable_pdf) { @@ -844,6 +843,7 @@ if (is_mac) { } defines = [ "HELPER_EXECUTABLE" ] sources = filenames.app_sources + sources += [ "atom/common/atom_constants.cc" ] include_dirs = [ "." ] info_plist = "atom/renderer/resources/mac/Info.plist" extra_substitutions = [ "ATOM_BUNDLE_ID=$electron_mac_bundle_id.helper" ] @@ -951,6 +951,7 @@ if (is_mac) { mac_app_bundle("electron_app") { output_name = electron_product_name sources = filenames.app_sources + sources += [ "atom/common/atom_constants.cc" ] include_dirs = [ "." ] deps = [ ":electron_app_framework_bundle_data", diff --git a/DEPS b/DEPS index 8770642acf9cc..6c96394fad28b 100644 --- a/DEPS +++ b/DEPS @@ -12,7 +12,7 @@ vars = { 'chromium_version': '76.0.3809.23', 'node_version': - 'dee0db9864a001ffc16440f725f4952a1a417069', + '229bd3245b2f54c12ea9ad0abcadbc209f8023dc', 'boto_version': 'f7574aa6cc2c819430c1f05e9a1a1a666ef8169b', 'pyyaml_version': '3.12', diff --git a/appveyor.yml b/appveyor.yml index 0e45c5e54f15b..0f9db76c43d1b 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -28,7 +28,6 @@ build_cloud: libcc-20 image: libcc-20-vs2017-15.9 environment: GIT_CACHE_PATH: C:\Users\electron\libcc_cache - DISABLE_CRASH_REPORTER_TESTS: true ELECTRON_OUT_DIR: Default ELECTRON_ENABLE_STACK_DUMPING: 1 build_script: diff --git a/atom/app/atom_main.cc b/atom/app/atom_main.cc index 49a77c842b0d1..beef50e64f03b 100644 --- a/atom/app/atom_main.cc +++ b/atom/app/atom_main.cc @@ -4,6 +4,7 @@ #include "atom/app/atom_main.h" +#include #include #include #include @@ -39,6 +40,7 @@ #include "atom/app/node_main.h" #include "atom/common/atom_command_line.h" +#include "atom/common/atom_constants.h" #include "base/at_exit.h" #include "base/i18n/icu_util.h" #include "electron/buildflags/buildflags.h" @@ -49,10 +51,6 @@ namespace { -#if BUILDFLAG(ENABLE_RUN_AS_NODE) -const char kRunAsNode[] = "ELECTRON_RUN_AS_NODE"; -#endif - ALLOW_UNUSED_TYPE bool IsEnvSet(const char* name) { #if defined(OS_WIN) size_t required_size; @@ -86,6 +84,11 @@ void FixStdioStreams() { } // namespace #if defined(OS_WIN) + +namespace crash_reporter { +extern const char kCrashpadProcess[]; +} + int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) { struct Arguments { int argc = 0; @@ -122,7 +125,7 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) { #endif #if BUILDFLAG(ENABLE_RUN_AS_NODE) - bool run_as_node = IsEnvSet(kRunAsNode); + bool run_as_node = IsEnvSet(atom::kRunAsNode); #else bool run_as_node = false; #endif @@ -148,13 +151,11 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) { atexit([]() { OnThreadExit(nullptr, DLL_THREAD_DETACH, nullptr); }); #endif + std::vector argv(arguments.argc); + std::transform(arguments.argv, arguments.argv + arguments.argc, argv.begin(), + [](auto& a) { return _strdup(base::WideToUTF8(a).c_str()); }); #if BUILDFLAG(ENABLE_RUN_AS_NODE) if (run_as_node) { - std::vector argv(arguments.argc); - std::transform( - arguments.argv, arguments.argv + arguments.argc, argv.begin(), - [](auto& a) { return _strdup(base::WideToUTF8(a).c_str()); }); - base::AtExitManager atexit_manager; base::i18n::InitializeICU(); auto ret = atom::NodeMain(argv.size(), argv.data()); @@ -163,8 +164,11 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) { } #endif - if (IsEnvSet("ELECTRON_INTERNAL_CRASH_SERVICE")) { - return crash_service::Main(cmd); + base::CommandLine::Init(argv.size(), argv.data()); + const base::CommandLine& cmd_line = *base::CommandLine::ForCurrentProcess(); + if (cmd_line.GetSwitchValueASCII("type") == + crash_reporter::kCrashpadProcess) { + return crash_service::Main(&argv); } if (!atom::CheckCommandLineArguments(arguments.argc, arguments.argv)) @@ -187,7 +191,7 @@ int main(int argc, char* argv[]) { FixStdioStreams(); #if BUILDFLAG(ENABLE_RUN_AS_NODE) - if (IsEnvSet(kRunAsNode)) { + if (IsEnvSet(atom::kRunAsNode)) { base::i18n::InitializeICU(); base::AtExitManager atexit_manager; return atom::NodeMain(argc, argv); @@ -208,7 +212,7 @@ int main(int argc, char* argv[]) { FixStdioStreams(); #if BUILDFLAG(ENABLE_RUN_AS_NODE) - if (IsEnvSet(kRunAsNode)) { + if (IsEnvSet(atom::kRunAsNode)) { return AtomInitializeICUandStartNode(argc, argv); } #endif diff --git a/atom/app/atom_main_delegate.cc b/atom/app/atom_main_delegate.cc index 1f1cf0f6d3376..edf881303c9c7 100644 --- a/atom/app/atom_main_delegate.cc +++ b/atom/app/atom_main_delegate.cc @@ -49,6 +49,9 @@ #if defined(OS_WIN) #include "base/win/win_util.h" +#if defined(_WIN64) +#include "atom/common/crash_reporter/crash_reporter_win.h" +#endif #endif namespace atom { @@ -133,6 +136,10 @@ bool AtomMainDelegate::BasicStartupComplete(int* exit_code) { logging::LoggingSettings settings; #if defined(OS_WIN) +#if defined(_WIN64) + crash_reporter::CrashReporterWin::SetUnhandledExceptionFilter(); +#endif + // On Windows the terminal returns immediately, so we add a new line to // prevent output in the same line as the prompt. if (IsBrowserProcess(command_line)) diff --git a/atom/app/node_main.cc b/atom/app/node_main.cc index 5636c51023a62..3b0d7b23c492f 100644 --- a/atom/app/node_main.cc +++ b/atom/app/node_main.cc @@ -25,6 +25,10 @@ #include "gin/v8_initializer.h" #include "native_mate/dictionary.h" +#if defined(_WIN64) +#include "atom/common/crash_reporter/crash_reporter_win.h" +#endif + namespace atom { int NodeMain(int argc, char* argv[]) { @@ -52,6 +56,9 @@ int NodeMain(int argc, char* argv[]) { // Initialize gin::IsolateHolder. JavascriptEnvironment gin_env(loop); +#if defined(_WIN64) + crash_reporter::CrashReporterWin::SetUnhandledExceptionFilter(); +#endif // Explicitly register electron's builtin modules. NodeBindings::RegisterBuiltinModules(); diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc index 2dd9e1404b779..8be8f89f4fd77 100644 --- a/atom/browser/atom_browser_client.cc +++ b/atom/browser/atom_browser_client.cc @@ -80,6 +80,10 @@ #include "ui/base/resource/resource_bundle.h" #include "v8/include/v8.h" +#if defined(OS_WIN) +#include "sandbox/win/src/sandbox_policy.h" +#endif + #if defined(USE_NSS_CERTS) #include "net/ssl/client_cert_store_nss.h" #elif defined(OS_WIN) @@ -907,6 +911,18 @@ void AtomBrowserClient::SetUserAgent(const std::string& user_agent) { user_agent_override_ = user_agent; } +#if defined(OS_WIN) +bool AtomBrowserClient::PreSpawnRenderer(sandbox::TargetPolicy* policy) { + // Allow crashpad to communicate via named pipe. + sandbox::ResultCode result = policy->AddRule( + sandbox::TargetPolicy::SUBSYS_FILES, + sandbox::TargetPolicy::FILES_ALLOW_ANY, L"\\??\\pipe\\crashpad_*"); + if (result != sandbox::SBOX_ALL_OK) + return false; + return true; +} +#endif // defined(OS_WIN) + std::string AtomBrowserClient::GetApplicationLocale() { if (BrowserThread::CurrentlyOn(BrowserThread::IO)) return g_io_thread_application_locale.Get(); diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h index cd7b256a4f412..33d88858c7a29 100644 --- a/atom/browser/atom_browser_client.h +++ b/atom/browser/atom_browser_client.h @@ -162,6 +162,9 @@ class AtomBrowserClient : public content::ContentBrowserClient, network::mojom::NetworkService* network_service) override; bool ShouldBypassCORB(int render_process_id) const override; std::string GetProduct() const override; +#if defined(OS_WIN) + bool PreSpawnRenderer(sandbox::TargetPolicy* policy) override; +#endif // content::RenderProcessHostObserver: void RenderProcessHostDestroyed(content::RenderProcessHost* host) override; diff --git a/atom/browser/ui/inspectable_web_contents.h b/atom/browser/ui/inspectable_web_contents.h index fe4c2c3bd0d7b..7d6d6ea72a67b 100644 --- a/atom/browser/ui/inspectable_web_contents.h +++ b/atom/browser/ui/inspectable_web_contents.h @@ -25,6 +25,8 @@ namespace atom { class InspectableWebContentsDelegate; class InspectableWebContentsView; +// TODO(zcbenz): Remove this abstract wrapper and rename +// InspectableWebContentsImpl to InspectableWebContents instead. class InspectableWebContents { public: // The returned InspectableWebContents takes ownership of the passed-in diff --git a/atom/browser/ui/inspectable_web_contents_impl.cc b/atom/browser/ui/inspectable_web_contents_impl.cc index 5c9670a0b3f65..e133d9209ec7e 100644 --- a/atom/browser/ui/inspectable_web_contents_impl.cc +++ b/atom/browser/ui/inspectable_web_contents_impl.cc @@ -3,11 +3,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE-CHROMIUM file. +#include "atom/browser/ui/inspectable_web_contents_impl.h" + #include #include -#include "atom/browser/ui/inspectable_web_contents_impl.h" - #include "atom/browser/ui/inspectable_web_contents_delegate.h" #include "atom/browser/ui/inspectable_web_contents_view.h" #include "atom/browser/ui/inspectable_web_contents_view_delegate.h" @@ -75,6 +75,9 @@ const char kTitleFormat[] = "Developer Tools - %s"; const size_t kMaxMessageChunkSize = IPC::Channel::kMaximumMessageSize / 4; +// Stores all instances of InspectableWebContentsImpl. +InspectableWebContentsImpl::List g_web_contents_instances_; + base::Value RectToDictionary(const gfx::Rect& bounds) { base::Value dict(base::Value::Type::DICTIONARY); dict.SetKey("x", base::Value(bounds.x())); @@ -227,6 +230,12 @@ class InspectableWebContentsImpl::NetworkResourceLoader InspectableWebContentsView* CreateInspectableContentsView( InspectableWebContentsImpl* inspectable_web_contents_impl); +// static +const InspectableWebContentsImpl::List& InspectableWebContentsImpl::GetAll() { + return g_web_contents_instances_; +} + +// static void InspectableWebContentsImpl::RegisterPrefs(PrefRegistrySimple* registry) { registry->RegisterDictionaryPref(kDevToolsBoundsPref, RectToDictionary(gfx::Rect(0, 0, 800, 600))); @@ -270,9 +279,11 @@ InspectableWebContentsImpl::InspectableWebContentsImpl( display.y() + (display.height() - devtools_bounds_.height()) / 2); } } + g_web_contents_instances_.push_back(this); } InspectableWebContentsImpl::~InspectableWebContentsImpl() { + g_web_contents_instances_.remove(this); // Unsubscribe from devtools and Clean up resources. if (GetDevToolsWebContents()) { if (managed_devtools_web_contents_) diff --git a/atom/browser/ui/inspectable_web_contents_impl.h b/atom/browser/ui/inspectable_web_contents_impl.h index 569597cf05ff3..c75a05b6ec5bd 100644 --- a/atom/browser/ui/inspectable_web_contents_impl.h +++ b/atom/browser/ui/inspectable_web_contents_impl.h @@ -6,6 +6,7 @@ #ifndef ATOM_BROWSER_UI_INSPECTABLE_WEB_CONTENTS_IMPL_H_ #define ATOM_BROWSER_UI_INSPECTABLE_WEB_CONTENTS_IMPL_H_ +#include #include #include #include @@ -38,6 +39,9 @@ class InspectableWebContentsImpl public content::WebContentsDelegate, public DevToolsEmbedderMessageDispatcher::Delegate { public: + using List = std::list; + + static const List& GetAll(); static void RegisterPrefs(PrefRegistrySimple* pref_registry); InspectableWebContentsImpl(content::WebContents* web_contents, diff --git a/atom/common/api/api.mojom b/atom/common/api/api.mojom index abd87fe6a8154..5eb77a8e3cceb 100644 --- a/atom/common/api/api.mojom +++ b/atom/common/api/api.mojom @@ -10,6 +10,8 @@ interface ElectronRenderer { mojo_base.mojom.ListValue arguments, int32 sender_id); + UpdateCrashpadPipeName(string pipe_name); + TakeHeapSnapshot(handle file) => (bool success); }; diff --git a/atom/common/atom_constants.cc b/atom/common/atom_constants.cc index 19b2bde65a01c..4d4c596ced5c2 100644 --- a/atom/common/atom_constants.cc +++ b/atom/common/atom_constants.cc @@ -27,6 +27,14 @@ const char kSecureProtocolDescription[] = "The connection to this site is using a strong protocol version " "and cipher suite."; +#if defined(OS_WIN) +const char kCrashpadPipeName[] = "ELECTRON_CRASHPAD_PIPE_NAME"; +#endif + +#if BUILDFLAG(ENABLE_RUN_AS_NODE) +const char kRunAsNode[] = "ELECTRON_RUN_AS_NODE"; +#endif + #if BUILDFLAG(ENABLE_PDF_VIEWER) const char kPdfPluginMimeType[] = "application/x-google-chrome-pdf"; const char kPdfPluginPath[] = "chrome://pdf-viewer/"; diff --git a/atom/common/atom_constants.h b/atom/common/atom_constants.h index 3b86a2cf90792..463d654d86ef3 100644 --- a/atom/common/atom_constants.h +++ b/atom/common/atom_constants.h @@ -5,6 +5,7 @@ #ifndef ATOM_COMMON_ATOM_CONSTANTS_H_ #define ATOM_COMMON_ATOM_CONSTANTS_H_ +#include "build/build_config.h" #include "electron/buildflags/buildflags.h" namespace atom { @@ -26,6 +27,15 @@ extern const char kValidCertificateDescription[]; extern const char kSecureProtocol[]; extern const char kSecureProtocolDescription[]; +#if defined(OS_WIN) +// Crashpad pipe name. +extern const char kCrashpadPipeName[]; +#endif + +#if BUILDFLAG(ENABLE_RUN_AS_NODE) +extern const char kRunAsNode[]; +#endif + #if BUILDFLAG(ENABLE_PDF_VIEWER) // The MIME type used for the PDF plugin. extern const char kPdfPluginMimeType[]; diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index f804f983e59aa..c0b177955c086 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -4,10 +4,14 @@ #include "atom/common/crash_reporter/crash_reporter.h" +#include + #include "atom/browser/browser.h" +#include "atom/common/atom_constants.h" #include "atom/common/atom_version.h" #include "atom/common/native_mate_converters/file_path_converter.h" #include "base/command_line.h" +#include "base/environment.h" #include "base/files/file_util.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" @@ -16,13 +20,26 @@ namespace crash_reporter { +const char kCrashpadProcess[] = "crash-handler"; +const char kCrashesDirectoryKey[] = "crashes-directory"; + CrashReporter::CrashReporter() { - auto* cmd = base::CommandLine::ForCurrentProcess(); - is_browser_ = cmd->GetSwitchValueASCII(switches::kProcessType).empty(); + std::unique_ptr env = base::Environment::Create(); + if (env->HasVar(atom::kRunAsNode)) { + process_type_ = "node"; + } else { + auto* cmd = base::CommandLine::ForCurrentProcess(); + process_type_ = cmd->GetSwitchValueASCII(switches::kProcessType); + } + // process_type_ will be empty for browser process } CrashReporter::~CrashReporter() {} +bool CrashReporter::IsInitialized() { + return is_initialized_; +} + void CrashReporter::Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, @@ -30,15 +47,19 @@ void CrashReporter::Start(const std::string& product_name, bool upload_to_server, bool skip_system_crash_handler, const StringMap& extra_parameters) { + is_initialized_ = true; SetUploadParameters(extra_parameters); - InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url, - crashes_dir, upload_to_server, skip_system_crash_handler); + Init(product_name, company_name, submit_url, crashes_dir, upload_to_server, + skip_system_crash_handler); } void CrashReporter::SetUploadParameters(const StringMap& parameters) { upload_parameters_ = parameters; - upload_parameters_["process_type"] = is_browser_ ? "browser" : "renderer"; + upload_parameters_["process_type"] = + process_type_.empty() ? "browser" : process_type_; + upload_parameters_["prod"] = ATOM_PRODUCT_NAME; + upload_parameters_["ver"] = ATOM_VERSION_STRING; // Setting platform dependent parameters. SetUploadParameters(); @@ -74,13 +95,12 @@ CrashReporter::GetUploadedReports(const base::FilePath& crashes_dir) { return result; } -void CrashReporter::InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool auto_submit, - bool skip_system_crash_handler) {} +void CrashReporter::Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool auto_submit, + bool skip_system_crash_handler) {} void CrashReporter::SetUploadParameters() {} diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 99c4c9818d8c4..357d91e8f9910 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -16,6 +16,9 @@ namespace crash_reporter { +extern const char kCrashpadProcess[]; +extern const char kCrashesDirectoryKey[]; + class CrashReporter { public: typedef std::map StringMap; @@ -24,6 +27,7 @@ class CrashReporter { static CrashReporter* GetInstance(); static void StartInstance(const mate::Dictionary& options); + bool IsInitialized(); void Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, @@ -46,19 +50,19 @@ class CrashReporter { CrashReporter(); virtual ~CrashReporter(); - virtual void InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler); + virtual void Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler); virtual void SetUploadParameters(); StringMap upload_parameters_; - bool is_browser_; + std::string process_type_; private: + bool is_initialized_ = false; void SetUploadParameters(const StringMap& parameters); DISALLOW_COPY_AND_ASSIGN(CrashReporter); diff --git a/atom/common/crash_reporter/crash_reporter_crashpad.cc b/atom/common/crash_reporter/crash_reporter_crashpad.cc new file mode 100644 index 0000000000000..87265ed0b0b44 --- /dev/null +++ b/atom/common/crash_reporter/crash_reporter_crashpad.cc @@ -0,0 +1,118 @@ +// Copyright (c) 2019 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "atom/common/crash_reporter/crash_reporter_crashpad.h" + +#include +#include + +#include "base/files/file_util.h" +#include "base/strings/string_piece.h" +#include "base/strings/stringprintf.h" +#include "base/strings/sys_string_conversions.h" +#include "base/threading/thread_restrictions.h" +#include "third_party/crashpad/crashpad/client/settings.h" + +namespace crash_reporter { + +CrashReporterCrashpad::CrashReporterCrashpad() {} + +CrashReporterCrashpad::~CrashReporterCrashpad() {} + +bool CrashReporterCrashpad::GetUploadToServer() { + bool enabled = true; + if (database_) { + database_->GetSettings()->GetUploadsEnabled(&enabled); + } + return enabled; +} + +void CrashReporterCrashpad::SetUploadToServer(const bool upload_to_server) { + if (database_) { + database_->GetSettings()->SetUploadsEnabled(upload_to_server); + } +} + +void CrashReporterCrashpad::SetCrashKeyValue(const base::StringPiece& key, + const base::StringPiece& value) { + simple_string_dictionary_->SetKeyValue(key.data(), value.data()); +} + +void CrashReporterCrashpad::SetInitialCrashKeyValues() { + for (const auto& upload_parameter : upload_parameters_) + SetCrashKeyValue(upload_parameter.first, upload_parameter.second); +} + +void CrashReporterCrashpad::AddExtraParameter(const std::string& key, + const std::string& value) { + if (simple_string_dictionary_) { + SetCrashKeyValue(key, value); + } else { + upload_parameters_[key] = value; + } +} + +void CrashReporterCrashpad::RemoveExtraParameter(const std::string& key) { + if (simple_string_dictionary_) + simple_string_dictionary_->RemoveKey(key.data()); + else + upload_parameters_.erase(key); +} + +std::map CrashReporterCrashpad::GetParameters() + const { + if (simple_string_dictionary_) { + std::map ret; + crashpad::SimpleStringDictionary::Iterator iter(*simple_string_dictionary_); + for (;;) { + auto* const entry = iter.Next(); + if (!entry) + break; + ret[entry->key] = entry->value; + } + return ret; + } + return upload_parameters_; +} + +std::vector +CrashReporterCrashpad::GetUploadedReports(const base::FilePath& crashes_dir) { + std::vector uploaded_reports; + + { + base::ThreadRestrictions::ScopedAllowIO allow_io; + if (!base::PathExists(crashes_dir)) { + return uploaded_reports; + } + } + // Load crashpad database. + std::unique_ptr database = + crashpad::CrashReportDatabase::Initialize(crashes_dir); + DCHECK(database); + + std::vector completed_reports; + crashpad::CrashReportDatabase::OperationStatus status = + database->GetCompletedReports(&completed_reports); + if (status != crashpad::CrashReportDatabase::kNoError) { + return uploaded_reports; + } + + for (const crashpad::CrashReportDatabase::Report& completed_report : + completed_reports) { + if (completed_report.uploaded) { + uploaded_reports.push_back( + UploadReportResult(static_cast(completed_report.creation_time), + completed_report.id)); + } + } + + auto sort_by_time = [](const UploadReportResult& a, + const UploadReportResult& b) { + return a.first >= b.first; + }; + std::sort(uploaded_reports.begin(), uploaded_reports.end(), sort_by_time); + return uploaded_reports; +} + +} // namespace crash_reporter diff --git a/atom/common/crash_reporter/crash_reporter_crashpad.h b/atom/common/crash_reporter/crash_reporter_crashpad.h new file mode 100644 index 0000000000000..bd721d533717f --- /dev/null +++ b/atom/common/crash_reporter/crash_reporter_crashpad.h @@ -0,0 +1,50 @@ +// Copyright (c) 2013 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_CRASHPAD_H_ +#define ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_CRASHPAD_H_ + +#include +#include +#include +#include + +#include "atom/common/crash_reporter/crash_reporter.h" +#include "base/compiler_specific.h" +#include "base/strings/string_piece.h" +#include "third_party/crashpad/crashpad/client/crash_report_database.h" +#include "third_party/crashpad/crashpad/client/simple_string_dictionary.h" + +namespace crash_reporter { + +class CrashReporterCrashpad : public CrashReporter { + public: + void SetUploadToServer(bool upload_to_server) override; + bool GetUploadToServer() override; + void AddExtraParameter(const std::string& key, + const std::string& value) override; + void RemoveExtraParameter(const std::string& key) override; + std::map GetParameters() const override; + + protected: + CrashReporterCrashpad(); + ~CrashReporterCrashpad() override; + + void SetUploadsEnabled(bool enable_uploads); + void SetCrashKeyValue(const base::StringPiece& key, + const base::StringPiece& value); + void SetInitialCrashKeyValues(); + + std::vector GetUploadedReports( + const base::FilePath& crashes_dir) override; + + std::unique_ptr simple_string_dictionary_; + std::unique_ptr database_; + + DISALLOW_COPY_AND_ASSIGN(CrashReporterCrashpad); +}; + +} // namespace crash_reporter + +#endif // ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_CRASHPAD_H_ diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index 7928c64fb8dac..c293cdcbdff4e 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -51,22 +51,18 @@ CrashReporterLinux::CrashReporterLinux() : pid_(getpid()) { CrashReporterLinux::~CrashReporterLinux() {} -void CrashReporterLinux::InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) { +void CrashReporterLinux::Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) { EnableCrashDumping(crashes_dir); - crash_keys_.reset(new CrashKeyStorage()); - - crash_keys_->SetKeyValue("prod", ATOM_PRODUCT_NAME); - crash_keys_->SetKeyValue("ver", version.c_str()); upload_url_ = submit_url; upload_to_server_ = upload_to_server; + crash_keys_.reset(new CrashKeyStorage()); for (StringMap::const_iterator iter = upload_parameters_.begin(); iter != upload_parameters_.end(); ++iter) crash_keys_->SetKeyValue(iter->first.c_str(), iter->second.c_str()); diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index ce9d85fa4b8c7..0f22d553c1c6e 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -28,13 +28,12 @@ class CrashReporterLinux : public CrashReporter { public: static CrashReporterLinux* GetInstance(); - void InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) override; + void Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) override; void SetUploadToServer(bool upload_to_server) override; void SetUploadParameters() override; bool GetUploadToServer() override; diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index 7a68a4389b1ea..d1b4bcbc337d2 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -5,16 +5,10 @@ #ifndef ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_MAC_H_ #define ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_MAC_H_ -#include #include #include -#include -#include "atom/common/crash_reporter/crash_reporter.h" -#include "base/compiler_specific.h" -#include "base/strings/string_piece.h" -#include "crashpad/client/crash_report_database.h" -#include "crashpad/client/simple_string_dictionary.h" +#include "atom/common/crash_reporter/crash_reporter_crashpad.h" namespace base { template @@ -23,24 +17,17 @@ struct DefaultSingletonTraits; namespace crash_reporter { -class CrashReporterMac : public CrashReporter { +class CrashReporterMac : public CrashReporterCrashpad { public: static CrashReporterMac* GetInstance(); - void InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) override; + void Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) override; void SetUploadParameters() override; - void SetUploadToServer(bool upload_to_server) override; - bool GetUploadToServer() override; - void AddExtraParameter(const std::string& key, - const std::string& value) override; - void RemoveExtraParameter(const std::string& key) override; - std::map GetParameters() const override; private: friend struct base::DefaultSingletonTraits; @@ -48,16 +35,6 @@ class CrashReporterMac : public CrashReporter { CrashReporterMac(); ~CrashReporterMac() override; - void SetUploadsEnabled(bool enable_uploads); - void SetCrashKeyValue(const base::StringPiece& key, - const base::StringPiece& value); - - std::vector GetUploadedReports( - const base::FilePath& crashes_dir) override; - - std::unique_ptr simple_string_dictionary_; - std::unique_ptr database_; - DISALLOW_COPY_AND_ASSIGN(CrashReporterMac); }; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 636ce4b065fdb..22a061c866b46 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -6,17 +6,11 @@ #include -#include "base/files/file_util.h" #include "base/mac/bundle_locations.h" #include "base/mac/mac_util.h" #include "base/memory/singleton.h" -#include "base/strings/string_piece.h" -#include "base/strings/stringprintf.h" -#include "base/strings/sys_string_conversions.h" -#include "base/threading/thread_restrictions.h" -#include "crashpad/client/crashpad_client.h" -#include "crashpad/client/crashpad_info.h" -#include "crashpad/client/settings.h" +#include "third_party/crashpad/crashpad/client/crashpad_client.h" +#include "third_party/crashpad/crashpad/client/crashpad_info.h" namespace crash_reporter { @@ -24,19 +18,18 @@ CrashReporterMac::~CrashReporterMac() {} -void CrashReporterMac::InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) { +void CrashReporterMac::Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) { // check whether crashpad has been initialized. // Only need to initialize once. if (simple_string_dictionary_) return; - if (is_browser_) { + if (process_type_.empty()) { // browser process @autoreleasepool { base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath(); base::FilePath handler_path = @@ -63,112 +56,17 @@ simple_string_dictionary_.reset(new crashpad::SimpleStringDictionary()); crashpad_info->set_simple_annotations(simple_string_dictionary_.get()); - SetCrashKeyValue("prod", ATOM_PRODUCT_NAME); - SetCrashKeyValue("process_type", is_browser_ ? "browser" : "renderer"); - SetCrashKeyValue("ver", version); - - for (const auto& upload_parameter : upload_parameters_) { - SetCrashKeyValue(upload_parameter.first, upload_parameter.second); - } - if (is_browser_) { + SetInitialCrashKeyValues(); + if (process_type_.empty()) { // browser process database_ = crashpad::CrashReportDatabase::Initialize(crashes_dir); SetUploadToServer(upload_to_server); } } -bool CrashReporterMac::GetUploadToServer() { - bool enabled = true; - if (database_) { - database_->GetSettings()->GetUploadsEnabled(&enabled); - } - return enabled; -} - -void CrashReporterMac::SetUploadToServer(const bool upload_to_server) { - if (database_) { - database_->GetSettings()->SetUploadsEnabled(upload_to_server); - } -} - void CrashReporterMac::SetUploadParameters() { upload_parameters_["platform"] = "darwin"; } -void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, - const base::StringPiece& value) { - simple_string_dictionary_->SetKeyValue(key.data(), value.data()); -} - -void CrashReporterMac::AddExtraParameter(const std::string& key, - const std::string& value) { - if (simple_string_dictionary_) { - SetCrashKeyValue(key, value); - } else { - upload_parameters_[key] = value; - } -} - -void CrashReporterMac::RemoveExtraParameter(const std::string& key) { - if (simple_string_dictionary_) - simple_string_dictionary_->RemoveKey(key.data()); - else - upload_parameters_.erase(key); -} - -std::map CrashReporterMac::GetParameters() const { - if (simple_string_dictionary_) { - std::map ret; - crashpad::SimpleStringDictionary::Iterator iter(*simple_string_dictionary_); - for (;;) { - auto* const entry = iter.Next(); - if (!entry) - break; - ret[entry->key] = entry->value; - } - return ret; - } - return upload_parameters_; -} - -std::vector -CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) { - std::vector uploaded_reports; - - { - base::ThreadRestrictions::ScopedAllowIO allow_io; - if (!base::PathExists(crashes_dir)) { - return uploaded_reports; - } - } - // Load crashpad database. - std::unique_ptr database = - crashpad::CrashReportDatabase::Initialize(crashes_dir); - DCHECK(database); - - std::vector completed_reports; - crashpad::CrashReportDatabase::OperationStatus status = - database->GetCompletedReports(&completed_reports); - if (status != crashpad::CrashReportDatabase::kNoError) { - return uploaded_reports; - } - - for (const crashpad::CrashReportDatabase::Report& completed_report : - completed_reports) { - if (completed_report.uploaded) { - uploaded_reports.push_back( - UploadReportResult(static_cast(completed_report.creation_time), - completed_report.id)); - } - } - - auto sort_by_time = [](const UploadReportResult& a, - const UploadReportResult& b) { - return a.first >= b.first; - }; - std::sort(uploaded_reports.begin(), uploaded_reports.end(), sort_by_time); - return uploaded_reports; -} - // static CrashReporterMac* CrashReporterMac::GetInstance() { return base::Singleton::get(); diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index bbb586dec621f..6d48b9533c2ef 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -4,276 +4,128 @@ #include "atom/common/crash_reporter/crash_reporter_win.h" -#include +#include +#include -#include "base/files/file_util.h" -#include "base/logging.h" +#include "atom/browser/ui/inspectable_web_contents_impl.h" +#include "atom/common/atom_constants.h" +#include "base/environment.h" #include "base/memory/singleton.h" -#include "base/strings/string_util.h" +#include "base/path_service.h" +#include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" -#include "content/public/common/result_codes.h" -#include "gin/public/debug.h" -#include "sandbox/win/src/nt_internals.h" - -#pragma intrinsic(_AddressOfReturnAddress) -#pragma intrinsic(_ReturnAddress) +#include "electron/atom/common/api/api.mojom.h" +#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" +#include "third_party/crashpad/crashpad/client/crashpad_client.h" +#include "third_party/crashpad/crashpad/client/crashpad_info.h" -#ifdef _WIN64 -// See http://msdn.microsoft.com/en-us/library/ddssxxy8.aspx -typedef struct _UNWIND_INFO { - unsigned char Version : 3; - unsigned char Flags : 5; - unsigned char SizeOfProlog; - unsigned char CountOfCodes; - unsigned char FrameRegister : 4; - unsigned char FrameOffset : 4; - ULONG ExceptionHandler; -} UNWIND_INFO, *PUNWIND_INFO; +#if defined(_WIN64) +#include "gin/public/debug.h" #endif -namespace crash_reporter { - namespace { -// Minidump with stacks, PEB, TEB, and unloaded module list. -const MINIDUMP_TYPE kSmallDumpType = static_cast( - MiniDumpWithProcessThreadData | // Get PEB and TEB. - MiniDumpWithUnloadedModules); // Get unloaded modules when available. - -const wchar_t kWaitEventFormat[] = L"$1CrashServiceWaitEvent"; -const wchar_t kPipeNameFormat[] = L"\\\\.\\pipe\\$1 Crash Service"; - -// Matches breakpad/src/client/windows/common/ipc_protocol.h. -const int kNameMaxLength = 64; -const int kValueMaxLength = 64; - -typedef NTSTATUS(WINAPI* NtTerminateProcessPtr)(HANDLE ProcessHandle, - NTSTATUS ExitStatus); -char* g_real_terminate_process_stub = NULL; - -void TerminateProcessWithoutDump() { - // Patched stub exists based on conditions (See InitCrashReporter). - // As a side note this function also gets called from - // WindowProcExceptionFilter. - if (g_real_terminate_process_stub == NULL) { - ::TerminateProcess(::GetCurrentProcess(), content::RESULT_CODE_KILLED); - } else { - NtTerminateProcessPtr real_terminate_proc = - reinterpret_cast( - static_cast(g_real_terminate_process_stub)); - real_terminate_proc(::GetCurrentProcess(), content::RESULT_CODE_KILLED); - } -} - -#ifdef _WIN64 -int CrashForExceptionInNonABICompliantCodeRange( - PEXCEPTION_RECORD ExceptionRecord, - ULONG64 EstablisherFrame, - PCONTEXT ContextRecord, - PDISPATCHER_CONTEXT DispatcherContext) { - EXCEPTION_POINTERS info = {ExceptionRecord, ContextRecord}; - if (!CrashReporter::GetInstance()) - return EXCEPTION_CONTINUE_SEARCH; - return static_cast(CrashReporter::GetInstance()) - ->CrashForException(&info); +#if defined(_WIN64) +int CrashForException(EXCEPTION_POINTERS* info) { + auto* reporter = crash_reporter::CrashReporterWin::GetInstance(); + if (reporter->IsInitialized()) + reporter->GetCrashpadClient().DumpAndCrash(info); + return EXCEPTION_CONTINUE_SEARCH; } - -struct ExceptionHandlerRecord { - RUNTIME_FUNCTION runtime_function; - UNWIND_INFO unwind_info; - unsigned char thunk[12]; -}; - -bool RegisterNonABICompliantCodeRange(void* start, size_t size_in_bytes) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - // We assume that the first page of the code range is executable and - // committed and reserved for breakpad. What could possibly go wrong? - - // All addresses are 32bit relative offsets to start. - record->runtime_function.BeginAddress = 0; -#if defined(_M_ARM64) - record->runtime_function.FunctionLength = - base::checked_cast(size_in_bytes); -#else - record->runtime_function.EndAddress = - base::checked_cast(size_in_bytes); #endif - record->runtime_function.UnwindData = - offsetof(ExceptionHandlerRecord, unwind_info); - - // Create unwind info that only specifies an exception handler. - record->unwind_info.Version = 1; - record->unwind_info.Flags = UNW_FLAG_EHANDLER; - record->unwind_info.SizeOfProlog = 0; - record->unwind_info.CountOfCodes = 0; - record->unwind_info.FrameRegister = 0; - record->unwind_info.FrameOffset = 0; - record->unwind_info.ExceptionHandler = - offsetof(ExceptionHandlerRecord, thunk); - - // Hardcoded thunk. - // mov imm64, rax - record->thunk[0] = 0x48; - record->thunk[1] = 0xb8; - void* handler = - reinterpret_cast(&CrashForExceptionInNonABICompliantCodeRange); - memcpy(&record->thunk[2], &handler, 8); - - // jmp rax - record->thunk[10] = 0xff; - record->thunk[11] = 0xe0; - - // Protect reserved page against modifications. - DWORD old_protect; - return VirtualProtect(start, sizeof(ExceptionHandlerRecord), - PAGE_EXECUTE_READ, &old_protect) && - RtlAddFunctionTable(&record->runtime_function, 1, - reinterpret_cast(start)); -} - -void UnregisterNonABICompliantCodeRange(void* start) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - RtlDeleteFunctionTable(&record->runtime_function); -} - -#endif // _WIN64 } // namespace +namespace crash_reporter { + CrashReporterWin::CrashReporterWin() {} CrashReporterWin::~CrashReporterWin() {} -void CrashReporterWin::InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) { - skip_system_crash_handler_ = skip_system_crash_handler; - - base::string16 pipe_name = base::ReplaceStringPlaceholders( - kPipeNameFormat, base::UTF8ToUTF16(product_name), NULL); - base::string16 wait_name = base::ReplaceStringPlaceholders( - kWaitEventFormat, base::UTF8ToUTF16(product_name), NULL); +#if defined(_WIN64) +void CrashReporterWin::SetUnhandledExceptionFilter() { + gin::Debug::SetUnhandledExceptionCallback(&CrashForException); +} +#endif - // Wait until the crash service is started. - HANDLE wait_event = ::CreateEventW(NULL, TRUE, FALSE, wait_name.c_str()); - if (wait_event != NULL) { - WaitForSingleObject(wait_event, 1000); - CloseHandle(wait_event); +void CrashReporterWin::Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) { + // check whether crashpad has been initialized. + // Only need to initialize once. + if (simple_string_dictionary_) + return; + if (process_type_.empty()) { // browser process + base::FilePath handler_path; + base::PathService::Get(base::FILE_EXE, &handler_path); + + std::vector args = { + "--no-rate-limit", + "--no-upload-gzip", // not all servers accept gzip + }; + args.push_back(base::StringPrintf("--type=%s", kCrashpadProcess)); + args.push_back( + base::StringPrintf("--%s=%s", kCrashesDirectoryKey, + base::UTF16ToUTF8(crashes_dir.value()).c_str())); + crashpad_client_.StartHandler(handler_path, crashes_dir, crashes_dir, + submit_url, StringMap(), args, true, false); + UpdatePipeName(); + } else { + std::unique_ptr env(base::Environment::Create()); + std::string pipe_name_utf8; + if (env->GetVar(atom::kCrashpadPipeName, &pipe_name_utf8)) { + base::string16 pipe_name = base::UTF8ToUTF16(pipe_name_utf8); + if (!crashpad_client_.SetHandlerIPCPipe(pipe_name)) + LOG(ERROR) << "Failed to set handler IPC pipe name: " << pipe_name; + } else { + LOG(ERROR) << "Unable to get pipe name for crashpad"; + } } + crashpad::CrashpadInfo* crashpad_info = + crashpad::CrashpadInfo::GetCrashpadInfo(); + if (skip_system_crash_handler) { + crashpad_info->set_system_crash_reporter_forwarding( + crashpad::TriState::kDisabled); + } + simple_string_dictionary_.reset(new crashpad::SimpleStringDictionary()); + crashpad_info->set_simple_annotations(simple_string_dictionary_.get()); - // ExceptionHandler() attaches our handler and ~ExceptionHandler() detaches - // it, so we must explicitly reset *before* we instantiate our new handler - // to allow any previous handler to detach in the correct order. - breakpad_.reset(); - - breakpad_.reset(new google_breakpad::ExceptionHandler( - crashes_dir.DirName().value(), FilterCallback, MinidumpCallback, this, - google_breakpad::ExceptionHandler::HANDLER_ALL, kSmallDumpType, - pipe_name.c_str(), - GetCustomInfo(product_name, version, company_name, upload_to_server))); - - if (!breakpad_->IsOutOfProcess()) - LOG(ERROR) << "Cannot initialize out-of-process crash handler"; - -#ifdef _WIN64 - // Hook up V8 to breakpad. - if (!code_range_registered_) { - code_range_registered_ = true; - // gin::Debug::SetCodeRangeCreatedCallback only runs the callback when - // Isolate is just created, so we have to manually run following code here. - void* code_range = nullptr; - size_t size = 0; - v8::Isolate::GetCurrent()->GetCodeRange(&code_range, &size); - if (code_range && size && - RegisterNonABICompliantCodeRange(code_range, size)) { - // FIXME(nornagon): This broke with https://crrev.com/c/1474703 - gin::Debug::SetCodeRangeDeletedCallback( - UnregisterNonABICompliantCodeRange); - } + SetInitialCrashKeyValues(); + if (process_type_.empty()) { // browser process + database_ = crashpad::CrashReportDatabase::Initialize(crashes_dir); + SetUploadToServer(upload_to_server); } -#endif } void CrashReporterWin::SetUploadParameters() { upload_parameters_["platform"] = "win32"; } -int CrashReporterWin::CrashForException(EXCEPTION_POINTERS* info) { - if (breakpad_) { - breakpad_->WriteMinidumpForException(info); - if (skip_system_crash_handler_) - TerminateProcessWithoutDump(); - else - RaiseFailFastException(info->ExceptionRecord, info->ContextRecord, 0); - } - return EXCEPTION_CONTINUE_SEARCH; +crashpad::CrashpadClient& CrashReporterWin::GetCrashpadClient() { + return crashpad_client_; } -// static -bool CrashReporterWin::FilterCallback(void* context, - EXCEPTION_POINTERS* exinfo, - MDRawAssertionInfo* assertion) { - return true; -} - -// static -bool CrashReporterWin::MinidumpCallback(const wchar_t* dump_path, - const wchar_t* minidump_id, - void* context, - EXCEPTION_POINTERS* exinfo, - MDRawAssertionInfo* assertion, - bool succeeded) { - CrashReporterWin* self = static_cast(context); - if (succeeded && self->skip_system_crash_handler_) - return true; - else - return false; -} - -google_breakpad::CustomClientInfo* CrashReporterWin::GetCustomInfo( - const std::string& product_name, - const std::string& version, - const std::string& company_name, - bool upload_to_server) { - custom_info_entries_.clear(); - custom_info_entries_.reserve(3 + upload_parameters_.size()); - - custom_info_entries_.push_back( - google_breakpad::CustomInfoEntry(L"prod", L"Electron")); - custom_info_entries_.push_back(google_breakpad::CustomInfoEntry( - L"ver", base::UTF8ToWide(version).c_str())); - if (!upload_to_server) { - custom_info_entries_.push_back( - google_breakpad::CustomInfoEntry(L"skip_upload", L"1")); - } - - for (StringMap::const_iterator iter = upload_parameters_.begin(); - iter != upload_parameters_.end(); ++iter) { - // breakpad has hardcoded the length of name/value, and doesn't truncate - // the values itself, so we have to truncate them here otherwise weird - // things may happen. - std::wstring name = base::UTF8ToWide(iter->first); - std::wstring value = base::UTF8ToWide(iter->second); - if (name.length() > kNameMaxLength - 1) - name.resize(kNameMaxLength - 1); - if (value.length() > kValueMaxLength - 1) - value.resize(kValueMaxLength - 1); - - custom_info_entries_.push_back( - google_breakpad::CustomInfoEntry(name.c_str(), value.c_str())); +void CrashReporterWin::UpdatePipeName() { + std::string pipe_name = + base::UTF16ToUTF8(crashpad_client_.GetHandlerIPCPipe()); + std::unique_ptr env(base::Environment::Create()); + env->SetVar(atom::kCrashpadPipeName, pipe_name); + + // Notify all WebContents of the pipe name. + const auto& pages = atom::InspectableWebContentsImpl::GetAll(); + for (auto* page : pages) { + auto* frame_host = page->GetWebContents()->GetMainFrame(); + if (!frame_host) + continue; + + atom::mojom::ElectronRendererAssociatedPtr electron_ptr; + frame_host->GetRemoteAssociatedInterfaces()->GetInterface( + mojo::MakeRequest(&electron_ptr)); + electron_ptr->UpdateCrashpadPipeName(pipe_name); } - - custom_info_.entries = &custom_info_entries_.front(); - custom_info_.count = custom_info_entries_.size(); - return &custom_info_; } // static diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index 89a2a5966d6ca..f41385de37201 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -7,11 +7,9 @@ #include #include -#include -#include "atom/common/crash_reporter/crash_reporter.h" -#include "base/compiler_specific.h" -#include "breakpad/src/client/windows/handler/exception_handler.h" +#include "atom/common/crash_reporter/crash_reporter_crashpad.h" +#include "third_party/crashpad/crashpad/client/crashpad_client.h" namespace base { template @@ -20,55 +18,31 @@ struct DefaultSingletonTraits; namespace crash_reporter { -class CrashReporterWin : public CrashReporter { +class CrashReporterWin : public CrashReporterCrashpad { public: static CrashReporterWin* GetInstance(); +#if defined(_WIN64) + static void SetUnhandledExceptionFilter(); +#endif - void InitBreakpad(const std::string& product_name, - const std::string& version, - const std::string& company_name, - const std::string& submit_url, - const base::FilePath& crashes_dir, - bool upload_to_server, - bool skip_system_crash_handler) override; + void Init(const std::string& product_name, + const std::string& company_name, + const std::string& submit_url, + const base::FilePath& crashes_dir, + bool upload_to_server, + bool skip_system_crash_handler) override; void SetUploadParameters() override; - // Crashes the process after generating a dump for the provided exception. - int CrashForException(EXCEPTION_POINTERS* info); + crashpad::CrashpadClient& GetCrashpadClient(); private: friend struct base::DefaultSingletonTraits; - CrashReporterWin(); ~CrashReporterWin() override; - static bool FilterCallback(void* context, - EXCEPTION_POINTERS* exinfo, - MDRawAssertionInfo* assertion); - - static bool MinidumpCallback(const wchar_t* dump_path, - const wchar_t* minidump_id, - void* context, - EXCEPTION_POINTERS* exinfo, - MDRawAssertionInfo* assertion, - bool succeeded); + void UpdatePipeName(); - // Returns the custom info structure based on parameters. - google_breakpad::CustomClientInfo* GetCustomInfo( - const std::string& product_name, - const std::string& version, - const std::string& company_name, - bool upload_to_server); - - // Custom information to be passed to crash handler. - std::vector custom_info_entries_; - google_breakpad::CustomClientInfo custom_info_; - - bool skip_system_crash_handler_ = false; -#ifdef _WIN64 - bool code_range_registered_ = false; -#endif - std::unique_ptr breakpad_; + crashpad::CrashpadClient crashpad_client_; DISALLOW_COPY_AND_ASSIGN(CrashReporterWin); }; diff --git a/atom/common/crash_reporter/win/crash_service_main.cc b/atom/common/crash_reporter/win/crash_service_main.cc index c25ee858de0b5..99caaaa391fd8 100644 --- a/atom/common/crash_reporter/win/crash_service_main.cc +++ b/atom/common/crash_reporter/win/crash_service_main.cc @@ -4,21 +4,22 @@ #include "atom/common/crash_reporter/win/crash_service_main.h" +#include + +#include "atom/common/crash_reporter/crash_reporter.h" #include "atom/common/crash_reporter/win/crash_service.h" #include "base/at_exit.h" #include "base/command_line.h" #include "base/files/file_util.h" #include "base/logging.h" #include "base/strings/string_util.h" +#include "base/strings/utf_string_conversions.h" +#include "third_party/crashpad/crashpad/handler/handler_main.h" namespace crash_service { namespace { -const char kApplicationName[] = "application-name"; -const char kCrashesDirectory[] = "crashes-directory"; - -const wchar_t kPipeNameFormat[] = L"\\\\.\\pipe\\$1 Crash Service"; const wchar_t kStandardLogFile[] = L"operation_log.txt"; void InvalidParameterHandler(const wchar_t*, @@ -37,35 +38,29 @@ bool CreateCrashServiceDirectory(const base::FilePath& temp_dir) { return true; } +void RemoveArgs(std::vector* args) { + args->erase( + std::remove_if(args->begin(), args->end(), [](const std::string& str) { + return base::StartsWith(str, "--type", base::CompareCase::SENSITIVE) || + base::StartsWith( + str, + std::string("--") + crash_reporter::kCrashesDirectoryKey, + base::CompareCase::INSENSITIVE_ASCII); + })); +} + } // namespace. -int Main(const wchar_t* cmd) { +int Main(std::vector* args) { // Ignore invalid parameter errors. _set_invalid_parameter_handler(InvalidParameterHandler); // Initialize all Chromium things. base::AtExitManager exit_manager; - base::CommandLine::Init(0, NULL); - base::CommandLine& cmd_line = *base::CommandLine::ForCurrentProcess(); - - // Use the application's name as pipe name and output directory. - if (!cmd_line.HasSwitch(kApplicationName)) { - LOG(ERROR) << "Application's name must be specified with --" - << kApplicationName; - return 1; - } - std::wstring application_name = - cmd_line.GetSwitchValueNative(kApplicationName); - - if (!cmd_line.HasSwitch(kCrashesDirectory)) { - LOG(ERROR) << "Crashes directory path must be specified with --" - << kCrashesDirectory; - return 1; - } - + base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess(); // We use/create a directory under the user's temp folder, for logging. base::FilePath operating_dir( - cmd_line.GetSwitchValueNative(kCrashesDirectory)); + cmd_line->GetSwitchValueNative(crash_reporter::kCrashesDirectoryKey)); CreateCrashServiceDirectory(operating_dir); base::FilePath log_file = operating_dir.Append(kStandardLogFile); @@ -78,27 +73,9 @@ int Main(const wchar_t* cmd) { // Logging with pid, tid and timestamp. logging::SetLogItems(true, true, true, false); - VLOG(1) << "Session start. cmdline is [" << cmd << "]"; - - // Setting the crash reporter. - base::string16 pipe_name = - base::ReplaceStringPlaceholders(kPipeNameFormat, application_name, NULL); - cmd_line.AppendSwitch("no-window"); - cmd_line.AppendSwitchASCII("max-reports", "128"); - cmd_line.AppendSwitchASCII("reporter", ATOM_PROJECT_NAME "-crash-service"); - cmd_line.AppendSwitchNative("pipe-name", pipe_name); - - breakpad::CrashService crash_service; - if (!crash_service.Initialize(application_name, operating_dir, operating_dir)) - return 2; - - VLOG(1) << "Ready to process crash requests"; - - // Enter the message loop. - int retv = crash_service.ProcessingLoop(); - // Time to exit. - VLOG(1) << "Session end. return code is " << retv; - return retv; + // Crashpad cannot handle unknown arguments, so we need to remove it + RemoveArgs(args); + return crashpad::HandlerMain(args->size(), args->data(), nullptr); } } // namespace crash_service diff --git a/atom/common/crash_reporter/win/crash_service_main.h b/atom/common/crash_reporter/win/crash_service_main.h index b536313dbc321..4481fb50ae666 100644 --- a/atom/common/crash_reporter/win/crash_service_main.h +++ b/atom/common/crash_reporter/win/crash_service_main.h @@ -5,10 +5,12 @@ #ifndef ATOM_COMMON_CRASH_REPORTER_WIN_CRASH_SERVICE_MAIN_H_ #define ATOM_COMMON_CRASH_REPORTER_WIN_CRASH_SERVICE_MAIN_H_ +#include + namespace crash_service { // Program entry, should be called by main(); -int Main(const wchar_t* cmd_line); +int Main(std::vector* args); } // namespace crash_service diff --git a/atom/renderer/electron_api_service_impl.cc b/atom/renderer/electron_api_service_impl.cc index 380cc6755a460..99bbcdbbe29e4 100644 --- a/atom/renderer/electron_api_service_impl.cc +++ b/atom/renderer/electron_api_service_impl.cc @@ -8,8 +8,10 @@ #include #include +#include "atom/common/atom_constants.h" #include "atom/common/heap_snapshot.h" #include "atom/common/native_mate_converters/value_converter.h" +#include "base/environment.h" #include "base/macros.h" #include "base/threading/thread_restrictions.h" #include "electron/atom/common/api/event_emitter_caller.h" @@ -147,6 +149,14 @@ void ElectronApiServiceImpl::Message(bool internal, } } +void ElectronApiServiceImpl::UpdateCrashpadPipeName( + const std::string& pipe_name) { +#if defined(OS_WIN) + std::unique_ptr env(base::Environment::Create()); + env->SetVar(kCrashpadPipeName, pipe_name); +#endif +} + void ElectronApiServiceImpl::TakeHeapSnapshot( mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) { diff --git a/atom/renderer/electron_api_service_impl.h b/atom/renderer/electron_api_service_impl.h index 19c51b81091d3..4c198a9ccccef 100644 --- a/atom/renderer/electron_api_service_impl.h +++ b/atom/renderer/electron_api_service_impl.h @@ -29,7 +29,7 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer, const std::string& channel, base::Value arguments, int32_t sender_id) override; - + void UpdateCrashpadPipeName(const std::string& pipe_name) override; void TakeHeapSnapshot(mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) override; diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 4591771363b71..9c15974c37145 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -66,26 +66,7 @@ reports temporarily. You can test this out by calling `process.crash()` to crash first call `start` you can call `addExtraParameter` on macOS or call `start` again with the new/updated `extra` parameters on Linux and Windows. -**Note:** To collect crash reports from child process in Windows, you need to add this extra code as well. -This will start the process that will monitor and send the crash reports. Replace `submitURL`, `productName` -and `crashesDirectory` with appropriate values. - -```js -const args = [ - `--reporter-url=${submitURL}`, - `--application-name=${productName}`, - `--crashes-directory=${crashesDirectory}` -] -const env = { - ELECTRON_INTERNAL_CRASH_SERVICE: 1 -} -spawn(process.execPath, args, { - env: env, - detached: true -}) -``` - -**Note:** On macOS, Electron uses a new `crashpad` client for crash collection and reporting. +**Note:** On macOS and windows, Electron uses a new `crashpad` client for crash collection and reporting. If you want to enable crash reporting, initializing `crashpad` from the main process using `crashReporter.start` is required regardless of which process you want to collect crashes from. Once initialized this way, the crashpad handler collects crashes from all processes. You still have to call `crashReporter.start` from the renderer or child process, otherwise crashes from @@ -104,14 +85,14 @@ Returns [`CrashReport[]`](structures/crash-report.md): Returns all uploaded crash reports. Each report contains the date and uploaded ID. -### `crashReporter.getUploadToServer()` _Linux_ _macOS_ +### `crashReporter.getUploadToServer()` Returns `Boolean` - Whether reports should be submitted to the server. Set through the `start` method or `setUploadToServer`. **Note:** This API can only be called from the main process. -### `crashReporter.setUploadToServer(uploadToServer)` _Linux_ _macOS_ +### `crashReporter.setUploadToServer(uploadToServer)` * `uploadToServer` Boolean _macOS_ - Whether reports should be submitted to the server. @@ -120,15 +101,15 @@ called before `start` is called. **Note:** This API can only be called from the main process. -### `crashReporter.addExtraParameter(key, value)` _macOS_ +### `crashReporter.addExtraParameter(key, value)` _macOS_ _Windows_ * `key` String - Parameter key, must be less than 64 characters long. * `value` String - Parameter value, must be less than 64 characters long. Set an extra parameter to be sent with the crash report. The values -specified here will be sent in addition to any values set via the `extra` option when `start` was called. This API is only available on macOS, if you need to add/update extra parameters on Linux and Windows after your first call to `start` you can call `start` again with the updated `extra` options. +specified here will be sent in addition to any values set via the `extra` option when `start` was called. This API is only available on macOS and windows, if you need to add/update extra parameters on Linux after your first call to `start` you can call `start` again with the updated `extra` options. -### `crashReporter.removeExtraParameter(key)` _macOS_ +### `crashReporter.removeExtraParameter(key)` _macOS_ _Windows_ * `key` String - Parameter key, must be less than 64 characters long. diff --git a/filenames.gni b/filenames.gni index c6730ccacd7cf..50d22fb381cae 100644 --- a/filenames.gni +++ b/filenames.gni @@ -584,12 +584,10 @@ filenames = { "atom/common/crash_reporter/crash_reporter_linux.h", "atom/common/crash_reporter/crash_reporter_mac.h", "atom/common/crash_reporter/crash_reporter_mac.mm", - "atom/common/crash_reporter/crash_reporter_win.cc", "atom/common/crash_reporter/crash_reporter_win.h", + "atom/common/crash_reporter/crash_reporter_win.cc", "atom/common/crash_reporter/linux/crash_dump_handler.cc", "atom/common/crash_reporter/linux/crash_dump_handler.h", - "atom/common/crash_reporter/win/crash_service.cc", - "atom/common/crash_reporter/win/crash_service.h", "atom/common/crash_reporter/win/crash_service_main.cc", "atom/common/crash_reporter/win/crash_service_main.h", "atom/common/draggable_region.cc", diff --git a/lib/browser/crash-reporter-init.js b/lib/browser/crash-reporter-init.js index ed03cc2145104..6dbc8e861c7c7 100644 --- a/lib/browser/crash-reporter-init.js +++ b/lib/browser/crash-reporter-init.js @@ -16,31 +16,10 @@ const getTempDirectory = function () { exports.crashReporterInit = function (options) { const productName = options.productName || app.getName() const crashesDirectory = path.join(getTempDirectory(), `${productName} Crashes`) - let crashServicePid - - if (process.platform === 'win32') { - const env = { - ELECTRON_INTERNAL_CRASH_SERVICE: 1 - } - const args = [ - '--reporter-url=' + options.submitURL, - '--application-name=' + productName, - '--crashes-directory=' + crashesDirectory, - '--v=1' - ] - - const crashServiceProcess = cp.spawn(process.helperExecPath, args, { - env, - detached: true - }) - - crashServicePid = crashServiceProcess.pid - } return { productName, crashesDirectory, - crashServicePid, appVersion: app.getVersion() } } diff --git a/lib/common/crash-reporter.js b/lib/common/crash-reporter.js index fe62bab5fa870..ae651f9523ba2 100644 --- a/lib/common/crash-reporter.js +++ b/lib/common/crash-reporter.js @@ -34,7 +34,6 @@ class CrashReporter { this.productName = ret.productName this.crashesDirectory = ret.crashesDirectory - this.crashServicePid = ret.crashServicePid if (extra._productName == null) extra._productName = ret.productName if (extra._companyName == null) extra._companyName = companyName diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index 2af23600067dc..0ff2e8f8f85b9 100644 --- a/patches/common/chromium/.patches +++ b/patches/common/chromium/.patches @@ -74,9 +74,9 @@ viz_osr.patch patch_the_ensure_gn_version_py_script_to_work_on_mac_ci.patch build_add_electron_tracing_category.patch add_contentgpuclient_precreatemessageloop_callback.patch -partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch disable_custom_libcxx_on_windows.patch fix_breakpad_symbol_generation_on_linux_arm.patch woa_compiler_workaround.patch cross_site_document_resource_handler.patch frame_host_manager.patch +crashpad_pid_check.patch diff --git a/patches/common/chromium/crashpad_pid_check.patch b/patches/common/chromium/crashpad_pid_check.patch new file mode 100644 index 0000000000000..f3e8ac39f6971 --- /dev/null +++ b/patches/common/chromium/crashpad_pid_check.patch @@ -0,0 +1,38 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Cheng Zhao +Date: Tue Jun 4 11:30:12 JST 2019 +Subject: crashpad_pid_check.patch + +When both browser process and renderer process are connecting to the pipe, +the API may return the PID of browser process as real_pid, which is different +from the PID of renderer process. + +This is caused by the crashReporter getting started after the sanbox, after +we redesign crashReporter's API to make it alwasy start before the +sanbox, we can remove this patch. + +See following links for more: +https://github.com/electron/electron/pull/18483#discussion_r292703588 +https://github.com/electron/electron/pull/18483#issuecomment-501090683 + +diff --git a/third_party/crashpad/crashpad/util/win/exception_handler_server.cc b/third_party/crashpad/crashpad/util/win/exception_handler_server.cc +index 2593ff2de032..e89b8ff675be 100644 +--- a/third_party/crashpad/crashpad/util/win/exception_handler_server.cc ++++ b/third_party/crashpad/crashpad/util/win/exception_handler_server.cc +@@ -448,9 +448,16 @@ bool ExceptionHandlerServer::ServiceClientConnection( + DWORD real_pid = 0; + if (get_named_pipe_client_process_id(service_context.pipe(), &real_pid) && + message.registration.client_process_id != real_pid) { ++ // Electron: When both browser process and renderer process are connecting ++ // to the pipe, the API may return the PID of browser process as real_pid, ++ // which is different from the PID of renderer process. ++ // ++ // I don't understand why Chromium does not have this issue. ++#if 0 + LOG(ERROR) << "forged client pid, real pid: " << real_pid + << ", got: " << message.registration.client_process_id; + return false; ++#endif + } + } + diff --git a/patches/common/chromium/partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch b/patches/common/chromium/partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch deleted file mode 100644 index 900d4f8c2e387..0000000000000 --- a/patches/common/chromium/partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch +++ /dev/null @@ -1,141 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Nitish Sakhawalkar -Date: Fri, 19 Apr 2019 14:01:12 -0700 -Subject: Partially revert fb4ab3be3fc0024d00358d5b7816d3215a8ff20c - -This change partially reverts the changes made in gin in fb4ab3be3fc0024d00358d5b7816d3215a8ff20c. Electron uses breakpad for windows, while chromium has moved to crashpad. Once electron uses crashpad for windows, we can get rid of this change. - -diff --git a/gin/debug_impl.cc b/gin/debug_impl.cc -index ca0577ea4caabce42bb4ec1aad8062b59eaaa8e4..5c3b7ffc932f063c8ad458d92643564edba393dc 100644 ---- a/gin/debug_impl.cc -+++ b/gin/debug_impl.cc -@@ -8,6 +8,10 @@ namespace gin { - - namespace { - v8::JitCodeEventHandler g_jit_code_event_handler = NULL; -+#if defined(OS_WIN) -+Debug::CodeRangeCreatedCallback g_code_range_created_callback = NULL; -+Debug::CodeRangeDeletedCallback g_code_range_deleted_callback = NULL; -+#endif - } // namespace - - // static -@@ -17,9 +21,13 @@ void Debug::SetJitCodeEventHandler(v8::JitCodeEventHandler event_handler) { - - #if defined(OS_WIN) - // static --void Debug::SetUnhandledExceptionCallback( -- v8::UnhandledExceptionCallback callback) { -- v8::V8::SetUnhandledExceptionCallback(callback); -+void Debug::SetCodeRangeCreatedCallback(CodeRangeCreatedCallback callback) { -+ g_code_range_created_callback = callback; -+} -+ -+// static -+void Debug::SetCodeRangeDeletedCallback(CodeRangeDeletedCallback callback) { -+ g_code_range_deleted_callback = callback; - } - #endif - -@@ -28,4 +36,16 @@ v8::JitCodeEventHandler DebugImpl::GetJitCodeEventHandler() { - return g_jit_code_event_handler; - } - -+#if defined(OS_WIN) -+// static -+Debug::CodeRangeCreatedCallback DebugImpl::GetCodeRangeCreatedCallback() { -+ return g_code_range_created_callback; -+} -+ -+// static -+Debug::CodeRangeDeletedCallback DebugImpl::GetCodeRangeDeletedCallback() { -+ return g_code_range_deleted_callback; -+} -+#endif -+ - } // namespace gin -diff --git a/gin/debug_impl.h b/gin/debug_impl.h -index b0b7931f3e10592577858f048eab37d4eff4c18f..b88c0b6c0896f60c6421023fe5783b614d596769 100644 ---- a/gin/debug_impl.h -+++ b/gin/debug_impl.h -@@ -13,6 +13,10 @@ namespace gin { - class DebugImpl { - public: - static v8::JitCodeEventHandler GetJitCodeEventHandler(); -+#if defined(OS_WIN) -+ static Debug::CodeRangeCreatedCallback GetCodeRangeCreatedCallback(); -+ static Debug::CodeRangeDeletedCallback GetCodeRangeDeletedCallback(); -+#endif - }; - - } // namespace gin -diff --git a/gin/isolate_holder.cc b/gin/isolate_holder.cc -index 4dd48f6a871cccb374c58adacb8ad9a80da89a5a..ec61b873d4e2dcdca833c8503beabb88d0798f2d 100644 ---- a/gin/isolate_holder.cc -+++ b/gin/isolate_holder.cc -@@ -91,9 +91,31 @@ IsolateHolder::IsolateHolder( - - isolate_memory_dump_provider_.reset( - new V8IsolateMemoryDumpProvider(this, task_runner)); -+#if defined(OS_WIN) -+ { -+ void* code_range; -+ size_t size; -+ isolate_->GetCodeRange(&code_range, &size); -+ Debug::CodeRangeCreatedCallback callback = -+ DebugImpl::GetCodeRangeCreatedCallback(); -+ if (code_range && size && callback) -+ callback(code_range, size); -+ } -+#endif - } - - IsolateHolder::~IsolateHolder() { -+#if defined(OS_WIN) -+ { -+ void* code_range; -+ size_t size; -+ isolate_->GetCodeRange(&code_range, &size); -+ Debug::CodeRangeDeletedCallback callback = -+ DebugImpl::GetCodeRangeDeletedCallback(); -+ if (code_range && callback) -+ callback(code_range); -+ } -+#endif - isolate_memory_dump_provider_.reset(); - isolate_data_.reset(); - isolate_->Dispose(); -diff --git a/gin/public/debug.h b/gin/public/debug.h -index 4e567876f7ac0f010783eded5e57f8b2293542d8..8c2eee341c3bc64331926e258e1fd20080373a80 100644 ---- a/gin/public/debug.h -+++ b/gin/public/debug.h -@@ -24,11 +24,25 @@ class GIN_EXPORT Debug { - static void SetJitCodeEventHandler(v8::JitCodeEventHandler event_handler); - - #if defined(OS_WIN) -- /* Sets a callback that is invoked for exceptions that arise in V8-generated -- * code (jitted code or embedded builtins). -+ typedef void (__cdecl *CodeRangeCreatedCallback)(void*, size_t); -+ -+ /* Sets a callback that is invoked for every new code range being created. -+ * -+ * On Win64, exception handling in jitted code is broken due to the fact -+ * that JS stack frames are not ABI compliant. It is possible to install -+ * custom handlers for the code range which holds the jitted code to work -+ * around this issue. -+ * -+ * https://code.google.com/p/v8/issues/detail?id=3598 -+ */ -+ static void SetCodeRangeCreatedCallback(CodeRangeCreatedCallback callback); -+ -+ typedef void (__cdecl *CodeRangeDeletedCallback)(void*); -+ -+ /* Sets a callback that is invoked for every previously registered code range -+ * when it is deleted. - */ -- static void SetUnhandledExceptionCallback( -- v8::UnhandledExceptionCallback callback); -+ static void SetCodeRangeDeletedCallback(CodeRangeDeletedCallback callback); - #endif - }; - diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index bd9928a3308f3..bbff0a25cd79e 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -44,10 +44,6 @@ describe('crashReporter module', () => { afterEach(() => closeWindow(w).then(() => { w = null })) - afterEach(() => { - stopCrashService() - }) - afterEach((done) => { if (stopServer != null) { stopServer(done) @@ -57,9 +53,6 @@ describe('crashReporter module', () => { }) it('should send minidump when renderer crashes', function (done) { - // TODO(alexeykuzmin): Skip the test instead of marking it as passed. - if (process.env.APPVEYOR === 'True') return done() - this.timeout(180000) stopServer = startServer({ @@ -72,34 +65,17 @@ describe('crashReporter module', () => { }) it('should send minidump when node processes crash', function (done) { - // TODO(alexeykuzmin): Skip the test instead of marking it as passed. - if (process.env.APPVEYOR === 'True') return done() - this.timeout(180000) stopServer = startServer({ callback (port) { - const crashesDir = path.join(app.getPath('temp'), `${process.platform === 'win32' ? 'Zombies' : app.getName()} Crashes`) + const crashesDir = path.join(app.getPath('temp'), `${app.getName()} Crashes`) const version = app.getVersion() const crashPath = path.join(fixtures, 'module', 'crash.js') - if (process.platform === 'win32') { - const crashServiceProcess = childProcess.spawn(process.execPath, [ - `--reporter-url=http://127.0.0.1:${port}`, - '--application-name=Zombies', - `--crashes-directory=${crashesDir}` - ], { - env: { - ELECTRON_INTERNAL_CRASH_SERVICE: 1 - }, - detached: true - }) - remote.process.crashServicePid = crashServiceProcess.pid - } - childProcess.fork(crashPath, [port, version, crashesDir], { silent: true }) }, - processType: 'browser', + processType: 'node', done: done }) }) @@ -110,14 +86,18 @@ describe('crashReporter module', () => { let dumpFile let crashesDir = crashReporter.getCrashesDirectory() const existingDumpFiles = new Set() - if (process.platform === 'darwin') { + if (process.platform !== 'linux') { // crashpad puts the dump files in the "completed" subdirectory - crashesDir = path.join(crashesDir, 'completed') + if (process.platform === 'darwin') { + crashesDir = path.join(crashesDir, 'completed') + } else { + crashesDir = path.join(crashesDir, 'reports') + } crashReporter.setUploadToServer(false) } const testDone = (uploaded) => { if (uploaded) return done(new Error('Uploaded crash report')) - if (process.platform === 'darwin') crashReporter.setUploadToServer(true) + if (process.platform !== 'linux') crashReporter.setUploadToServer(true) assert(fs.existsSync(dumpFile)) done() } @@ -167,9 +147,6 @@ describe('crashReporter module', () => { }) it('should send minidump with updated extra parameters', function (done) { - // TODO(alexeykuzmin): Skip the test instead of marking it as passed. - if (process.env.APPVEYOR === 'True') return done() - this.timeout(180000) stopServer = startServer({ @@ -209,7 +186,7 @@ describe('crashReporter module', () => { describe('getProductName', () => { it('returns the product name if one is specified', () => { const name = crashReporter.getProductName() - const expectedName = (process.platform === 'darwin') ? 'Electron Test' : 'Zombies' + const expectedName = 'Electron Test' assert.strictEqual(name, expectedName) }) }) @@ -241,12 +218,7 @@ describe('crashReporter module', () => { describe('getCrashesDirectory', () => { it('correctly returns the directory', () => { const crashesDir = crashReporter.getCrashesDirectory() - let dir - if (process.platform === 'win32') { - dir = `${app.getPath('temp')}/Zombies Crashes` - } else { - dir = `${app.getPath('temp')}/Electron Test Crashes` - } + const dir = path.join(app.getPath('temp'), 'Electron Test Crashes') assert.strictEqual(crashesDir, dir) }) }) @@ -291,7 +263,7 @@ describe('crashReporter module', () => { assert.throws(() => require('electron').crashReporter.getUploadToServer()) }) it('returns true when uploadToServer is set to true', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -305,7 +277,7 @@ describe('crashReporter module', () => { assert.strictEqual(crashReporter.getUploadToServer(), true) }) it('returns false when uploadToServer is set to false', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -326,7 +298,7 @@ describe('crashReporter module', () => { assert.throws(() => require('electron').crashReporter.setUploadToServer('arg')) }) it('sets uploadToServer false when called with false', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -341,7 +313,7 @@ describe('crashReporter module', () => { assert.strictEqual(crashReporter.getUploadToServer(), false) }) it('sets uploadToServer true when called with true', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -368,7 +340,7 @@ describe('crashReporter module', () => { assert(typeof parameters === 'object') }) it('adds a parameter to current parameters', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -383,7 +355,7 @@ describe('crashReporter module', () => { assert('hello' in crashReporter.getParameters()) }) it('removes a parameter from current parameters', function () { - if (process.platform !== 'darwin') { + if (process.platform === 'linux') { // FIXME(alexeykuzmin): Skip the test. // this.skip() return @@ -464,7 +436,7 @@ const startServer = ({ callback, processType, done }) => { server.listen(port, '127.0.0.1', () => { port = server.address().port remote.process.port = port - if (process.platform === 'darwin') { + if (process.platform !== 'linux') { crashReporter.start({ companyName: 'Umbrella Corporation', submitURL: 'http://127.0.0.1:' + port @@ -482,17 +454,3 @@ const startServer = ({ callback, processType, done }) => { }) } } - -const stopCrashService = () => { - const { crashServicePid } = remote.process - if (crashServicePid) { - remote.process.crashServicePid = 0 - try { - process.kill(crashServicePid) - } catch (error) { - if (error.code !== 'ESRCH') { - throw error - } - } - } -} diff --git a/spec/fixtures/api/crash-restart.html b/spec/fixtures/api/crash-restart.html index 7dec07ad88190..0ee4ad53503b5 100644 --- a/spec/fixtures/api/crash-restart.html +++ b/spec/fixtures/api/crash-restart.html @@ -18,12 +18,8 @@ } }) -if (process.platform === 'win32') { - ipcRenderer.sendSync('crash-service-pid', crashReporter.crashServicePid) -} - setImmediate(() => { - if (process.platform === 'darwin') { + if (process.platform !== 'linux') { crashReporter.addExtraParameter('extra2', 'extra2') crashReporter.removeExtraParameter('extra3') } else { diff --git a/spec/fixtures/api/crash.html b/spec/fixtures/api/crash.html index 588737c792992..bcce825426c93 100644 --- a/spec/fixtures/api/crash.html +++ b/spec/fixtures/api/crash.html @@ -19,10 +19,6 @@ } }) - if (process.platform === 'win32') { - ipcRenderer.sendSync('crash-service-pid', crashReporter.crashServicePid) - } - if (!uploadToServer) { ipcRenderer.sendSync('list-existing-dumps') } @@ -31,4 +27,4 @@ - \ No newline at end of file + diff --git a/spec/static/main.js b/spec/static/main.js index fee3e06d682ed..6663337a19d15 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -19,7 +19,6 @@ let window = null // will be used by crash-reporter spec. process.port = 0 -process.crashServicePid = 0 v8.setFlagsFromString('--expose_gc') app.commandLine.appendSwitch('js-flags', '--expose_gc') @@ -408,11 +407,6 @@ ipcMain.on('handle-unhandled-rejection', (event, message) => { }) }) -ipcMain.on('crash-service-pid', (event, pid) => { - process.crashServicePid = pid - event.returnValue = null -}) - ipcMain.on('test-webcontents-navigation-observer', (event, options) => { let contents = null let destroy = () => {}