From bdd2f919137546a560ce808fd386994823628c57 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 4 Oct 2015 19:20:52 +0800 Subject: [PATCH 1/4] Make Browser::Quit more robust --- atom/browser/browser.cc | 15 +++++++++++++-- atom/browser/browser.h | 3 +++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index 9d2a9fc1effb..d8bb94103cd9 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -16,7 +16,8 @@ namespace atom { Browser::Browser() : is_quiting_(false), - is_ready_(false) { + is_ready_(false), + is_shutdown_(false) { WindowList::AddObserver(this); } @@ -30,6 +31,9 @@ Browser* Browser::Get() { } void Browser::Quit() { + if (is_quiting_) + return; + is_quiting_ = HandleBeforeQuit(); if (!is_quiting_) return; @@ -42,9 +46,13 @@ void Browser::Quit() { } void Browser::Shutdown() { - FOR_EACH_OBSERVER(BrowserObserver, observers_, OnQuit()); + if (is_shutdown_) + return; + is_shutdown_ = true; is_quiting_ = true; + + FOR_EACH_OBSERVER(BrowserObserver, observers_, OnQuit()); base::MessageLoop::current()->PostTask( FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); } @@ -121,6 +129,9 @@ void Browser::ClientCertificateSelector( } void Browser::NotifyAndShutdown() { + if (is_shutdown_) + return; + bool prevent_default = false; FOR_EACH_OBSERVER(BrowserObserver, observers_, OnWillQuit(&prevent_default)); diff --git a/atom/browser/browser.h b/atom/browser/browser.h index d135556b8760..3c5abd2f0405 100644 --- a/atom/browser/browser.h +++ b/atom/browser/browser.h @@ -159,6 +159,9 @@ class Browser : public WindowListObserver { // Whether "ready" event has been emitted. bool is_ready_; + // The browse is being shutdown. + bool is_shutdown_; + std::string version_override_; std::string name_override_; From 0e131f760b63ad199c5fffa13b2d422cf3bb5e26 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 4 Oct 2015 19:21:36 +0800 Subject: [PATCH 2/4] Quit gracefully when Ctrl-C is pressed --- atom/browser/atom_browser_main_parts.cc | 12 + atom/browser/atom_browser_main_parts.h | 8 + atom/browser/atom_browser_main_parts_posix.cc | 225 ++++++++++++++++++ filenames.gypi | 1 + 4 files changed, 246 insertions(+) create mode 100644 atom/browser/atom_browser_main_parts_posix.cc diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 1b46247c4918..e70d42fc768a 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -52,6 +52,12 @@ void AtomBrowserMainParts::RegisterDestructionCallback( destruction_callbacks_.push_back(callback); } +void AtomBrowserMainParts::PreEarlyInitialization() { +#if defined(OS_POSIX) + HandleSIGCHLD(); +#endif +} + void AtomBrowserMainParts::PostEarlyInitialization() { brightray::BrowserMainParts::PostEarlyInitialization(); @@ -112,6 +118,12 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { #endif } +void AtomBrowserMainParts::PostMainMessageLoopStart() { +#if defined(OS_POSIX) + HandleShutdownSignals(); +#endif +} + void AtomBrowserMainParts::PostMainMessageLoopRun() { brightray::BrowserMainParts::PostMainMessageLoopRun(); diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index 9fefa486a1dc..65b142157dc1 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/atom/browser/atom_browser_main_parts.h @@ -39,8 +39,10 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { protected: // content::BrowserMainParts: + void PreEarlyInitialization() override; void PostEarlyInitialization() override; void PreMainMessageLoopRun() override; + void PostMainMessageLoopStart() override; void PostMainMessageLoopRun() override; #if defined(OS_MACOSX) void PreMainMessageLoopStart() override; @@ -48,6 +50,12 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { #endif private: +#if defined(OS_POSIX) + // Set signal handlers. + void HandleSIGCHLD(); + void HandleShutdownSignals(); +#endif + // A fake BrowserProcess object that used to feed the source code from chrome. scoped_ptr fake_browser_process_; diff --git a/atom/browser/atom_browser_main_parts_posix.cc b/atom/browser/atom_browser_main_parts_posix.cc new file mode 100644 index 000000000000..ea0f87b171d4 --- /dev/null +++ b/atom/browser/atom_browser_main_parts_posix.cc @@ -0,0 +1,225 @@ +// Copyright (c) 2015 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +// Most code came from: chrome/browser/chrome_browser_main_posix.cc. + +#include "atom/browser/atom_browser_main_parts.h" + +#include +#include +#include +#include +#include +#include + +#include "atom/browser/browser.h" +#include "base/posix/eintr_wrapper.h" +#include "content/public/browser/browser_thread.h" + +using content::BrowserThread; + +namespace atom { + +namespace { + +// See comment in |PreEarlyInitialization()|, where sigaction is called. +void SIGCHLDHandler(int signal) { +} + +// The OSX fork() implementation can crash in the child process before +// fork() returns. In that case, the shutdown pipe will still be +// shared with the parent process. To prevent child crashes from +// causing parent shutdowns, |g_pipe_pid| is the pid for the process +// which registered |g_shutdown_pipe_write_fd|. +// See . +pid_t g_pipe_pid = -1; +int g_shutdown_pipe_write_fd = -1; +int g_shutdown_pipe_read_fd = -1; + +// Common code between SIG{HUP, INT, TERM}Handler. +void GracefulShutdownHandler(int signal) { + // Reinstall the default handler. We had one shot at graceful shutdown. + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = SIG_DFL; + RAW_CHECK(sigaction(signal, &action, NULL) == 0); + + RAW_CHECK(g_pipe_pid == getpid()); + RAW_CHECK(g_shutdown_pipe_write_fd != -1); + RAW_CHECK(g_shutdown_pipe_read_fd != -1); + size_t bytes_written = 0; + do { + int rv = HANDLE_EINTR( + write(g_shutdown_pipe_write_fd, + reinterpret_cast(&signal) + bytes_written, + sizeof(signal) - bytes_written)); + RAW_CHECK(rv >= 0); + bytes_written += rv; + } while (bytes_written < sizeof(signal)); +} + +// See comment in |PostMainMessageLoopStart()|, where sigaction is called. +void SIGHUPHandler(int signal) { + RAW_CHECK(signal == SIGHUP); + GracefulShutdownHandler(signal); +} + +// See comment in |PostMainMessageLoopStart()|, where sigaction is called. +void SIGINTHandler(int signal) { + RAW_CHECK(signal == SIGINT); + GracefulShutdownHandler(signal); +} + +// See comment in |PostMainMessageLoopStart()|, where sigaction is called. +void SIGTERMHandler(int signal) { + RAW_CHECK(signal == SIGTERM); + GracefulShutdownHandler(signal); +} + +class ShutdownDetector : public base::PlatformThread::Delegate { + public: + explicit ShutdownDetector(int shutdown_fd); + + void ThreadMain() override; + + private: + const int shutdown_fd_; + + DISALLOW_COPY_AND_ASSIGN(ShutdownDetector); +}; + +ShutdownDetector::ShutdownDetector(int shutdown_fd) + : shutdown_fd_(shutdown_fd) { + CHECK_NE(shutdown_fd_, -1); +} + +// These functions are used to help us diagnose crash dumps that happen +// during the shutdown process. +NOINLINE void ShutdownFDReadError() { + // Ensure function isn't optimized away. + asm(""); + sleep(UINT_MAX); +} + +NOINLINE void ShutdownFDClosedError() { + // Ensure function isn't optimized away. + asm(""); + sleep(UINT_MAX); +} + +NOINLINE void ExitPosted() { + // Ensure function isn't optimized away. + asm(""); + sleep(UINT_MAX); +} + +void ShutdownDetector::ThreadMain() { + base::PlatformThread::SetName("CrShutdownDetector"); + + int signal; + size_t bytes_read = 0; + ssize_t ret; + do { + ret = HANDLE_EINTR( + read(shutdown_fd_, + reinterpret_cast(&signal) + bytes_read, + sizeof(signal) - bytes_read)); + if (ret < 0) { + NOTREACHED() << "Unexpected error: " << strerror(errno); + ShutdownFDReadError(); + break; + } else if (ret == 0) { + NOTREACHED() << "Unexpected closure of shutdown pipe."; + ShutdownFDClosedError(); + break; + } + bytes_read += ret; + } while (bytes_read < sizeof(signal)); + VLOG(1) << "Handling shutdown for signal " << signal << "."; + base::Closure task = + base::Bind(&Browser::Quit, base::Unretained(Browser::Get())); + + if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, task)) { + // Without a UI thread to post the exit task to, there aren't many + // options. Raise the signal again. The default handler will pick it up + // and cause an ungraceful exit. + RAW_LOG(WARNING, "No UI thread, exiting ungracefully."); + kill(getpid(), signal); + + // The signal may be handled on another thread. Give that a chance to + // happen. + sleep(3); + + // We really should be dead by now. For whatever reason, we're not. Exit + // immediately, with the exit status set to the signal number with bit 8 + // set. On the systems that we care about, this exit status is what is + // normally used to indicate an exit by this signal's default handler. + // This mechanism isn't a de jure standard, but even in the worst case, it + // should at least result in an immediate exit. + RAW_LOG(WARNING, "Still here, exiting really ungracefully."); + _exit(signal | (1 << 7)); + } + ExitPosted(); +} + +} // namespace + +void AtomBrowserMainParts::HandleSIGCHLD() { + // We need to accept SIGCHLD, even though our handler is a no-op because + // otherwise we cannot wait on children. (According to POSIX 2001.) + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = SIGCHLDHandler; + CHECK(sigaction(SIGCHLD, &action, NULL) == 0); +} + +void AtomBrowserMainParts::HandleShutdownSignals() { + int pipefd[2]; + int ret = pipe(pipefd); + if (ret < 0) { + PLOG(DFATAL) << "Failed to create pipe"; + } else { + g_pipe_pid = getpid(); + g_shutdown_pipe_read_fd = pipefd[0]; + g_shutdown_pipe_write_fd = pipefd[1]; +#if !defined(ADDRESS_SANITIZER) && !defined(KEEP_SHADOW_STACKS) + const size_t kShutdownDetectorThreadStackSize = PTHREAD_STACK_MIN * 2; +#else + // ASan instrumentation and -finstrument-functions (used for keeping the + // shadow stacks) bloat the stack frames, so we need to increase the stack + // size to avoid hitting the guard page. + const size_t kShutdownDetectorThreadStackSize = PTHREAD_STACK_MIN * 4; +#endif + // TODO(viettrungluu,willchan): crbug.com/29675 - This currently leaks, so + // if you change this, you'll probably need to change the suppression. + if (!base::PlatformThread::CreateNonJoinable( + kShutdownDetectorThreadStackSize, + new ShutdownDetector(g_shutdown_pipe_read_fd))) { + LOG(DFATAL) << "Failed to create shutdown detector task."; + } + } + // Setup signal handlers for shutdown AFTER shutdown pipe is setup because + // it may be called right away after handler is set. + + // If adding to this list of signal handlers, note the new signal probably + // needs to be reset in child processes. See + // base/process_util_posix.cc:LaunchProcess. + + // We need to handle SIGTERM, because that is how many POSIX-based distros ask + // processes to quit gracefully at shutdown time. + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = SIGTERMHandler; + CHECK(sigaction(SIGTERM, &action, NULL) == 0); + // Also handle SIGINT - when the user terminates the browser via Ctrl+C. If + // the browser process is being debugged, GDB will catch the SIGINT first. + action.sa_handler = SIGINTHandler; + CHECK(sigaction(SIGINT, &action, NULL) == 0); + // And SIGHUP, for when the terminal disappears. On shutdown, many Linux + // distros send SIGHUP, SIGTERM, and then SIGKILL. + action.sa_handler = SIGHUPHandler; + CHECK(sigaction(SIGHUP, &action, NULL) == 0); +} + +} // namespace atom diff --git a/filenames.gypi b/filenames.gypi index eb94973e018c..04b82992bc33 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -125,6 +125,7 @@ 'atom/browser/atom_browser_main_parts.cc', 'atom/browser/atom_browser_main_parts.h', 'atom/browser/atom_browser_main_parts_mac.mm', + 'atom/browser/atom_browser_main_parts_posix.cc', 'atom/browser/atom_javascript_dialog_manager.cc', 'atom/browser/atom_javascript_dialog_manager.h', 'atom/browser/atom_quota_permission_context.cc', From a2a4970f5f6601c6e8b713e6fe342ea63d78b022 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 4 Oct 2015 19:36:41 +0800 Subject: [PATCH 3/4] Fix cpplint warnings --- atom/browser/atom_browser_main_parts_posix.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/atom/browser/atom_browser_main_parts_posix.cc b/atom/browser/atom_browser_main_parts_posix.cc index ea0f87b171d4..2a0dddc47483 100644 --- a/atom/browser/atom_browser_main_parts_posix.cc +++ b/atom/browser/atom_browser_main_parts_posix.cc @@ -171,7 +171,7 @@ void AtomBrowserMainParts::HandleSIGCHLD() { struct sigaction action; memset(&action, 0, sizeof(action)); action.sa_handler = SIGCHLDHandler; - CHECK(sigaction(SIGCHLD, &action, NULL) == 0); + CHECK_EQ(sigaction(SIGCHLD, &action, NULL), 0); } void AtomBrowserMainParts::HandleShutdownSignals() { @@ -211,15 +211,15 @@ void AtomBrowserMainParts::HandleShutdownSignals() { struct sigaction action; memset(&action, 0, sizeof(action)); action.sa_handler = SIGTERMHandler; - CHECK(sigaction(SIGTERM, &action, NULL) == 0); + CHECK_EQ(sigaction(SIGTERM, &action, NULL), 0); // Also handle SIGINT - when the user terminates the browser via Ctrl+C. If // the browser process is being debugged, GDB will catch the SIGINT first. action.sa_handler = SIGINTHandler; - CHECK(sigaction(SIGINT, &action, NULL) == 0); + CHECK_EQ(sigaction(SIGINT, &action, NULL), 0); // And SIGHUP, for when the terminal disappears. On shutdown, many Linux // distros send SIGHUP, SIGTERM, and then SIGKILL. action.sa_handler = SIGHUPHandler; - CHECK(sigaction(SIGHUP, &action, NULL) == 0); + CHECK_EQ(sigaction(SIGHUP, &action, NULL), 0); } } // namespace atom From d4bfeff6ad1686feb4a815096bb7890230a73a01 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 4 Oct 2015 20:08:19 +0800 Subject: [PATCH 4/4] Fix crashing on Linux --- atom/browser/atom_browser_main_parts.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index e70d42fc768a..4c11176997ca 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -53,6 +53,7 @@ void AtomBrowserMainParts::RegisterDestructionCallback( } void AtomBrowserMainParts::PreEarlyInitialization() { + brightray::BrowserMainParts::PreEarlyInitialization(); #if defined(OS_POSIX) HandleSIGCHLD(); #endif @@ -119,6 +120,7 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { } void AtomBrowserMainParts::PostMainMessageLoopStart() { + brightray::BrowserMainParts::PostMainMessageLoopStart(); #if defined(OS_POSIX) HandleShutdownSignals(); #endif