diff --git a/atom/common/api/lib/crash-reporter.coffee b/atom/common/api/lib/crash-reporter.coffee index ecbd2f165d..6fd839de3f 100644 --- a/atom/common/api/lib/crash-reporter.coffee +++ b/atom/common/api/lib/crash-reporter.coffee @@ -1,18 +1,21 @@ -{spawn} = require 'child_process' binding = process.atomBinding 'crash_reporter' +fs = require 'fs' +os = require 'os' +path = require 'path' +{spawn} = require 'child_process' class CrashReporter start: (options={}) -> - {productName, companyName, submitUrl, autoSubmit, ignoreSystemCrashHandler, extra} = options + {@productName, companyName, submitUrl, autoSubmit, ignoreSystemCrashHandler, extra} = options - productName ?= 'Atom-Shell' + @productName ?= 'Atom-Shell' companyName ?= 'GitHub, Inc' submitUrl ?= 'http://54.249.141.255:1127/post' autoSubmit ?= true ignoreSystemCrashHandler ?= false extra ?= {} - extra._productName ?= productName + extra._productName ?= @productName extra._companyName ?= companyName extra._version ?= if process.type is 'browser' @@ -20,12 +23,12 @@ class CrashReporter else require('remote').require('app').getVersion() - start = -> binding.start productName, companyName, submitUrl, autoSubmit, ignoreSystemCrashHandler, extra + start = => binding.start @productName, companyName, submitUrl, autoSubmit, ignoreSystemCrashHandler, extra if process.platform is 'win32' args = [ "--reporter-url=#{submitUrl}" - "--application-name=#{productName}" + "--application-name=#{@productName}" "--v=1" ] env = ATOM_SHELL_INTERNAL_CRASH_SERVICE: 1 @@ -35,4 +38,20 @@ class CrashReporter else start() -module.exports = new CrashReporter + getLastCrashReport: -> + tmpdir = + if process.platform is 'win32' + os.tmpdir() + else + '/tmp' + log = path.join tmpdir, "#{@productName} Crashes", 'uploads.log' + try + reports = String(fs.readFileSync(log)).split('\n') + return null unless reports.length > 1 + [time, id] = reports[reports.length - 2].split ',' + return {date: new Date(parseInt(time) * 1000), id} + catch e + return null + +crashRepoter = new CrashReporter +module.exports = crashRepoter diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index ab12869ba7..89e2f7e76a 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -12,11 +12,12 @@ #include "base/debug/crash_logging.h" #include "base/files/file_path.h" +#include "base/files/file_util.h" #include "base/linux_util.h" #include "base/logging.h" -#include "base/path_service.h" #include "base/process/memory.h" #include "base/memory/singleton.h" +#include "base/strings/stringprintf.h" #include "vendor/breakpad/src/client/linux/handler/exception_handler.h" #include "vendor/breakpad/src/common/linux/linux_libc_support.h" @@ -61,7 +62,7 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name, const std::string& submit_url, bool auto_submit, bool skip_system_crash_handler) { - EnableCrashDumping(); + EnableCrashDumping(product_name); crash_keys_.SetKeyValue("prod", "Atom-Shell"); crash_keys_.SetKeyValue("ver", version.c_str()); @@ -76,11 +77,15 @@ void CrashReporterLinux::SetUploadParameters() { upload_parameters_["platform"] = "linux"; } -void CrashReporterLinux::EnableCrashDumping() { - base::FilePath tmp_path("/tmp"); - PathService::Get(base::DIR_TEMP, &tmp_path); +void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) { + std::string dump_dir = "/tmp/" + product_name + " Crashes"; + base::FilePath dumps_path(dump_dir); + base::CreateDirectory(dumps_path); + + std::string log_file = base::StringPrintf( + "%s/%s", dump_dir.c_str(), "uploads.log"); + strncpy(g_crash_log_path, log_file.c_str(), sizeof(g_crash_log_path)); - base::FilePath dumps_path(tmp_path); MinidumpDescriptor minidump_descriptor(dumps_path.value()); minidump_descriptor.set_size_limit(kMaxMinidumpFileSize); diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index 660c471d2a..c1055aded1 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -39,7 +39,7 @@ class CrashReporterLinux : public CrashReporter { CrashReporterLinux(); virtual ~CrashReporterLinux(); - void EnableCrashDumping(); + void EnableCrashDumping(const std::string& product_name); static bool CrashDone(const google_breakpad::MinidumpDescriptor& minidump, void* context, diff --git a/atom/common/crash_reporter/linux/crash_dump_handler.cc b/atom/common/crash_reporter/linux/crash_dump_handler.cc index c5ae6ac94f..56a5e094d4 100644 --- a/atom/common/crash_reporter/linux/crash_dump_handler.cc +++ b/atom/common/crash_reporter/linux/crash_dump_handler.cc @@ -75,6 +75,10 @@ uint64_t kernel_timeval_to_ms(struct kernel_timeval *tv) { return ret; } +bool my_isxdigit(char c) { + return (c >= '0' && c <= '9') || ((c | 0x20) >= 'a' && (c | 0x20) <= 'f'); +} + size_t LengthWithoutTrailingSpaces(const char* str, size_t len) { while (len > 0 && str[len - 1] == ' ') { len--; @@ -345,13 +349,12 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info, kWgetBinary, header, post_file, - // TODO(zcbenz): Enabling custom upload url. info.upload_url, "--timeout=60", // Set a timeout so we don't hang forever. "--tries=1", // Don't retry if the upload fails. "--quiet", // Be silent. "-O", // output reply to /dev/null. - "/dev/null", + "/dev/fd/3", NULL, }; static const char msg[] = "Cannot upload crash dump: cannot exec " @@ -361,8 +364,98 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info, sys__exit(1); } +// Runs in the helper process to wait for the upload process running +// ExecUploadProcessOrTerminate() to finish. Returns the number of bytes written +// to |fd| and save the written contents to |buf|. +// |buf| needs to be big enough to hold |bytes_to_read| + 1 characters. +size_t WaitForCrashReportUploadProcess(int fd, size_t bytes_to_read, + char* buf) { + size_t bytes_read = 0; + + // Upload should finish in about 10 seconds. Add a few more 500 ms + // internals to account for process startup time. + for (size_t wait_count = 0; wait_count < 24; ++wait_count) { + struct kernel_pollfd poll_fd; + poll_fd.fd = fd; + poll_fd.events = POLLIN | POLLPRI | POLLERR; + int ret = sys_poll(&poll_fd, 1, 500); + if (ret < 0) { + // Error + break; + } else if (ret > 0) { + // There is data to read. + ssize_t len = HANDLE_EINTR( + sys_read(fd, buf + bytes_read, bytes_to_read - bytes_read)); + if (len < 0) + break; + bytes_read += len; + if (bytes_read == bytes_to_read) + break; + } + // |ret| == 0 -> timed out, continue waiting. + // or |bytes_read| < |bytes_to_read| still, keep reading. + } + buf[bytes_to_read] = 0; // Always NUL terminate the buffer. + return bytes_read; +} + +// |buf| should be |expected_len| + 1 characters in size and NULL terminated. +bool IsValidCrashReportId(const char* buf, size_t bytes_read, + size_t expected_len) { + if (bytes_read != expected_len) + return false; + for (size_t i = 0; i < bytes_read; ++i) { + if (!my_isxdigit(buf[i]) && buf[i] != '-') + return false; + } + return true; +} + +// |buf| should be |expected_len| + 1 characters in size and NULL terminated. +void HandleCrashReportId(const char* buf, size_t bytes_read, + size_t expected_len) { + if (!IsValidCrashReportId(buf, bytes_read, expected_len)) { + static const char msg[] = "Failed to get crash dump id."; + WriteLog(msg, sizeof(msg) - 1); + WriteNewline(); + + static const char id_msg[] = "Report Id: "; + WriteLog(id_msg, sizeof(id_msg) - 1); + WriteLog(buf, bytes_read); + WriteNewline(); + return; + } + + // Write crash dump id to stderr. + static const char msg[] = "Crash dump id: "; + WriteLog(msg, sizeof(msg) - 1); + WriteLog(buf, my_strlen(buf)); + WriteNewline(); + + // Write crash dump id to crash log as: seconds_since_epoch,crash_id + struct kernel_timeval tv; + if (!sys_gettimeofday(&tv, NULL)) { + uint64_t time = kernel_timeval_to_ms(&tv) / 1000; + char time_str[kUint64StringSize]; + const unsigned time_len = my_uint64_len(time); + my_uint64tos(time_str, time, time_len); + + const int kLogOpenFlags = O_CREAT | O_WRONLY | O_APPEND | O_CLOEXEC; + int log_fd = sys_open(g_crash_log_path, kLogOpenFlags, 0600); + if (log_fd > 0) { + sys_write(log_fd, time_str, time_len); + sys_write(log_fd, ",", 1); + sys_write(log_fd, buf, my_strlen(buf)); + sys_write(log_fd, "\n", 1); + IGNORE_RET(sys_close(log_fd)); + } + } +} + } // namespace +char g_crash_log_path[256]; + void HandleCrashDump(const BreakpadInfo& info) { int dumpfd; bool keep_fd = false; @@ -616,33 +709,13 @@ void HandleCrashDump(const BreakpadInfo& info) { // Helper process. if (upload_child > 0) { IGNORE_RET(sys_close(fds[1])); - char id_buf[17]; // Crash report IDs are expected to be 16 chars. - ssize_t len = -1; - // Upload should finish in about 10 seconds. Add a few more 500 ms - // internals to account for process startup time. - for (size_t wait_count = 0; wait_count < 24; ++wait_count) { - struct kernel_pollfd poll_fd; - poll_fd.fd = fds[0]; - poll_fd.events = POLLIN | POLLPRI | POLLERR; - int ret = sys_poll(&poll_fd, 1, 500); - if (ret < 0) { - // Error - break; - } else if (ret > 0) { - // There is data to read. - len = HANDLE_EINTR(sys_read(fds[0], id_buf, sizeof(id_buf) - 1)); - break; - } - // ret == 0 -> timed out, continue waiting. - } - if (len > 0) { - // Write crash dump id to stderr. - id_buf[len] = 0; - static const char msg[] = "\nCrash dump id: "; - WriteLog(msg, sizeof(msg) - 1); - WriteLog(id_buf, my_strlen(id_buf)); - WriteLog("\n", 1); - } + + const size_t kCrashIdLength = 36; + char id_buf[kCrashIdLength + 1]; + size_t bytes_read = + WaitForCrashReportUploadProcess(fds[0], kCrashIdLength, id_buf); + HandleCrashReportId(id_buf, bytes_read, kCrashIdLength); + if (sys_waitpid(upload_child, NULL, WNOHANG) == 0) { // Upload process is still around, kill it. sys_kill(upload_child, SIGKILL); @@ -666,4 +739,8 @@ size_t WriteLog(const char* buf, size_t nbytes) { return sys_write(2, buf, nbytes); } +size_t WriteNewline() { + return WriteLog("\n", 1); +} + } // namespace crash_reporter diff --git a/atom/common/crash_reporter/linux/crash_dump_handler.h b/atom/common/crash_reporter/linux/crash_dump_handler.h index 9331a8fbd9..f600c9e0d1 100644 --- a/atom/common/crash_reporter/linux/crash_dump_handler.h +++ b/atom/common/crash_reporter/linux/crash_dump_handler.h @@ -32,6 +32,10 @@ struct BreakpadInfo { void HandleCrashDump(const BreakpadInfo& info); size_t WriteLog(const char* buf, size_t nbytes); +size_t WriteNewline(); + +// Global variable storing the path of upload log. +extern char g_crash_log_path[256]; } // namespace crash_reporter diff --git a/atom/common/crash_reporter/win/crash_service.cc b/atom/common/crash_reporter/win/crash_service.cc index 5cb6f98351..ee77eb07f6 100644 --- a/atom/common/crash_reporter/win/crash_service.cc +++ b/atom/common/crash_reporter/win/crash_service.cc @@ -13,6 +13,8 @@ #include "base/command_line.h" #include "base/file_util.h" #include "base/logging.h" +#include "base/strings/string_number_conversions.h" +#include "base/time/time.h" #include "base/win/windows_version.h" #include "vendor/breakpad/src/client/windows/crash_generation/client_info.h" #include "vendor/breakpad/src/client/windows/crash_generation/crash_generation_server.h" @@ -66,6 +68,30 @@ bool WriteCustomInfoToFile(const std::wstring& dump_path, const CrashMap& map) { return true; } +bool WriteReportIDToFile(const std::wstring& dump_path, + const std::wstring& report_id) { + std::wstring file_path(dump_path); + size_t last_slash = file_path.rfind(L'\\'); + if (last_slash == std::wstring::npos) + return false; + file_path.resize(last_slash); + file_path += L"\\uploads.log"; + + std::wofstream file(file_path.c_str(), + std::ios_base::out | std::ios_base::app | std::ios::binary); + if (!file.is_open()) + return false; + + int64 seconds_since_epoch = + (base::Time::Now() - base::Time::UnixEpoch()).InSeconds(); + std::wstring line = base::Int64ToString16(seconds_since_epoch); + line += L','; + line += report_id; + line += L'\n'; + file.write(line.c_str(), static_cast(line.length())); + return true; +} + // The window procedure task is to handle when a) the user logs off. // b) the system shuts down or c) when the user closes the window. LRESULT __stdcall CrashSvcWndProc(HWND hwnd, UINT message, @@ -422,6 +448,7 @@ DWORD CrashService::AsyncSendDump(void* context) { ++info->self->requests_sent_; ++info->self->requests_handled_; retry_round = 0; + WriteReportIDToFile(info->dump_path, report_id); break; case google_breakpad::RESULT_THROTTLED: report_id = L""; diff --git a/docs/README.md b/docs/README.md index d22ef38e72..ed7d803b72 100644 --- a/docs/README.md +++ b/docs/README.md @@ -56,3 +56,4 @@ Modules for both sides: * [Build instructions (Mac)](development/build-instructions-mac.md) * [Build instructions (Windows)](development/build-instructions-windows.md) * [Build instructions (Linux)](development/build-instructions-linux.md) +* [Setting up symbol server in debugger](development/setting-up-symbol-server.md) diff --git a/docs/api/chrome-command-line-switches.md b/docs/api/chrome-command-line-switches.md index 158e147d46..6ac57ac97d 100644 --- a/docs/api/chrome-command-line-switches.md +++ b/docs/api/chrome-command-line-switches.md @@ -7,7 +7,7 @@ module is emitted: ```javascript var app = require('app'); -app.commandLine.appendSwitch('remote-debugging-port', '88315'); +app.commandLine.appendSwitch('remote-debugging-port', '8315'); app.commandLine.appendSwitch('host-rules', 'MAP * 127.0.0.1'); app.on('ready', function() { diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 233dd1f1d6..3d0cf5467b 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -27,6 +27,11 @@ crashReporter.start({ * Only string properties are send correctly. * Nested objects are not supported. +## crashReporter.getLastCrashReport() + +Returns the date and ID of last crash report, when there was no crash report +sent or the crash reporter is not started, `null` will be returned. + # crash-reporter payload The crash reporter will send the following data to the `submitUrl` as `POST`: diff --git a/docs/development/setting-up-symbol-server.md b/docs/development/setting-up-symbol-server.md new file mode 100644 index 0000000000..b69c0df028 --- /dev/null +++ b/docs/development/setting-up-symbol-server.md @@ -0,0 +1,56 @@ +# Setting up symbol server in debugger + +Debug symbols allow you to have better debugging sessions. They have information +about the functions contained in executables and dynamic libraries and provide +you with information to get clean call stacks. A Symbol Server allows the +debugger to load the correct symbols, binaries and sources automatically without +forcing users to download large debugging files. The server functions like +[Microsoft's symbol server](http://support.microsoft.com/kb/311503) so the +documentation there can be useful. + +Note that because released atom-shell builds are heavily optimized, debugging is +not always easy. The debugger will not be able to show you the content of all +variables and the execution path can seem strange because of inlining, tail +calls, and other compiler optimizations. The only workaround is to build an +unoptimized local build. + +The official symbol server URL for atom-shell is +http://54.249.141.255:8086/atom-shell/symbols. +You cannot visit this URL directly: you must add it to the symbol path of your +debugging tool. In the examples below, a local cache directory is used to avoid +repeatedly fetching the PDB from the server. Replace `c:\code\symbols` with an +appropriate cache directory on your machine. + +## Using the symbol server in Windbg + +The Windbg symbol path is configured with a string value delimited with asterisk +characters. To use only the atom-shell symbol server, add the following entry to +your symbol path (__note:__ you can replace `c:\code\symbols` with any writable +directory on your computer, if you'd prefer a different location for downloaded +symbols): + +``` +SRV*c:\code\symbols\*http://54.249.141.255:8086/atom-shell/symbols +``` + +Set this string as `_NT_SYMBOL_PATH` in the environment, using the Windbg menus, +or by typing the `.sympath` command. If you would like to get symbols from +Microsoft's symbol server as well, you should list that first: + +``` +SRV*c:\code\symbols\*http://msdl.microsoft.com/download/symbols;SRV*c:\code\symbols\*http://54.249.141.255:8086/atom-shell/symbols +``` + +## Using the symbol server in Visual Studio + + + + +## Troubleshooting: Symbols will not load + +Type the following commands in Windbg to print why symbols are not loading: + +``` +> !sym noisy +> .reload /f chromiumcontent.dll +``` diff --git a/script/lib/util.py b/script/lib/util.py index 83c3e05148..355fec027c 100644 --- a/script/lib/util.py +++ b/script/lib/util.py @@ -183,6 +183,7 @@ def s3put(bucket, access_key, secret_key, prefix, key_prefix, files): '--secret_key', secret_key, '--prefix', prefix, '--key_prefix', key_prefix, + '--no_overwrite', '--grant', 'public-read' ] + files diff --git a/script/upload-windows-pdb.py b/script/upload-windows-pdb.py new file mode 100755 index 0000000000..e03e2bcfac --- /dev/null +++ b/script/upload-windows-pdb.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python + +import os +import glob + +from lib.util import execute, rm_rf, safe_mkdir, s3put, s3_config + + +SOURCE_ROOT = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) +SYMBOLS_DIR = 'dist\\symbols' +DOWNLOAD_DIR = 'vendor\\brightray\\vendor\\download\\libchromiumcontent' +PDB_LIST = [ + 'out\\Release\\atom.exe.pdb', + DOWNLOAD_DIR + '\\Release\\chromiumcontent.dll.pdb', +] + + +def main(): + os.chdir(SOURCE_ROOT) + + rm_rf(SYMBOLS_DIR) + safe_mkdir(SYMBOLS_DIR) + for pdb in PDB_LIST: + run_symstore(pdb, SYMBOLS_DIR, 'AtomShell') + + bucket, access_key, secret_key = s3_config() + files = glob.glob(SYMBOLS_DIR + '/*.pdb/*/*.pdb') + files = [f.lower() for f in files] + upload_symbols(bucket, access_key, secret_key, files) + + +def run_symstore(pdb, dest, product): + execute(['symstore', 'add', '/r', '/f', pdb, '/s', dest, '/t', product]) + + +def upload_symbols(bucket, access_key, secret_key, files): + s3put(bucket, access_key, secret_key, SYMBOLS_DIR, 'atom-shell/symbols', + files) + + +if __name__ == '__main__': + import sys + sys.exit(main()) diff --git a/script/upload.py b/script/upload.py index 2ad7f5ffd8..7841017ed1 100755 --- a/script/upload.py +++ b/script/upload.py @@ -59,19 +59,24 @@ def main(): upload_atom_shell(github, release_id, os.path.join(DIST_DIR, CHROMEDRIVER_NAME)) - # Upload node's headers to S3. - bucket, access_key, secret_key = s3_config() - upload_node(bucket, access_key, secret_key, ATOM_SHELL_VERSION) - if args.publish_release: - # Press the publish button. - publish_release(github, release_id) + # Upload node's headers to S3. + bucket, access_key, secret_key = s3_config() + upload_node(bucket, access_key, secret_key, ATOM_SHELL_VERSION) # Upload the SHASUMS.txt. execute([sys.executable, os.path.join(SOURCE_ROOT, 'script', 'upload-checksums.py'), '-v', ATOM_SHELL_VERSION]) + # Upload PDBs to Windows symbol server. + if TARGET_PLATFORM == 'win32': + execute([sys.executable, + os.path.join(SOURCE_ROOT, 'script', 'upload-windows-pdb.py')]) + + # Press the publish button. + publish_release(github, release_id) + def parse_args(): parser = argparse.ArgumentParser(description='upload distribution file') diff --git a/spec/api-crash-reporter-spec.coffee b/spec/api-crash-reporter-spec.coffee index 2dd1499edb..43f7b2f035 100644 --- a/spec/api-crash-reporter-spec.coffee +++ b/spec/api-crash-reporter-spec.coffee @@ -32,7 +32,7 @@ describe 'crash-reporter module', -> assert.equal fields['_version'], require('remote').require('app').getVersion() assert files['upload_file_minidump']['name']? - res.end() + res.end('abc-123-def') server.close() done() server.listen 0, '127.0.0.1', ->