From 863199348f8a07cdff4dfa23fea0598ddb3dcf2b Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Nov 2015 18:27:13 +0800 Subject: [PATCH 1/4] Make process.exit() quit gracefully Instead of abrupting the whole program immediately, we should close all windows and release all native resources gracefully on exit. This avoids possible crashes. Fix #3350. --- atom/browser/api/atom_api_app.cc | 1 + atom/browser/api/lib/app.coffee | 1 - atom/browser/atom_browser_main_parts.cc | 14 ++++++++++++++ atom/browser/atom_browser_main_parts.h | 7 +++++++ atom/browser/browser.cc | 22 ++++++++++++++++++++++ atom/browser/browser.h | 3 +++ atom/browser/lib/init.coffee | 3 +++ docs/api/app.md | 9 +++++++++ 8 files changed, 59 insertions(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 04ed06e0138f..1c5c2c04f145 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -339,6 +339,7 @@ mate::ObjectTemplateBuilder App::GetObjectTemplateBuilder( auto browser = base::Unretained(Browser::Get()); return mate::ObjectTemplateBuilder(isolate) .SetMethod("quit", base::Bind(&Browser::Quit, browser)) + .SetMethod("exit", base::Bind(&Browser::Exit, browser)) .SetMethod("focus", base::Bind(&Browser::Focus, browser)) .SetMethod("getVersion", base::Bind(&Browser::GetVersion, browser)) .SetMethod("setVersion", base::Bind(&Browser::SetVersion, browser)) diff --git a/atom/browser/api/lib/app.coffee b/atom/browser/api/lib/app.coffee index ba68076177ae..4df7aef10bdf 100644 --- a/atom/browser/api/lib/app.coffee +++ b/atom/browser/api/lib/app.coffee @@ -50,7 +50,6 @@ app.getAppPath = -> # Be compatible with old API. app.once 'ready', -> @emit 'finish-launching' app.terminate = app.quit -app.exit = process.exit app.getHomeDir = -> @getPath 'home' app.getDataPath = -> @getPath 'userData' app.setDataPath = (path) -> @setPath 'userData', path diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 041c9bfc8fbe..0a8c16ca223f 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -30,6 +30,7 @@ AtomBrowserMainParts* AtomBrowserMainParts::self_ = NULL; AtomBrowserMainParts::AtomBrowserMainParts() : fake_browser_process_(new BrowserProcess), + exit_code_(nullptr), browser_(new Browser), node_bindings_(NodeBindings::Create(true)), atom_bindings_(new AtomBindings), @@ -47,6 +48,14 @@ AtomBrowserMainParts* AtomBrowserMainParts::Get() { return self_; } +bool AtomBrowserMainParts::SetExitCode(int code) { + if (!exit_code_) + return false; + + *exit_code_ = code; + return true; +} + void AtomBrowserMainParts::RegisterDestructionCallback( const base::Closure& callback) { destruction_callbacks_.push_back(callback); @@ -118,6 +127,11 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { #endif } +bool AtomBrowserMainParts::MainMessageLoopRun(int* result_code) { + exit_code_ = result_code; + return brightray::BrowserMainParts::MainMessageLoopRun(result_code); +} + void AtomBrowserMainParts::PostMainMessageLoopStart() { brightray::BrowserMainParts::PostMainMessageLoopStart(); #if defined(OS_POSIX) diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index 360b1cba1169..bb4b204669fd 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/atom/browser/atom_browser_main_parts.h @@ -31,6 +31,9 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { static AtomBrowserMainParts* Get(); + // Sets the exit code, will fail if the the message loop is not ready. + bool SetExitCode(int code); + // Register a callback that should be destroyed before JavaScript environment // gets destroyed. void RegisterDestructionCallback(const base::Closure& callback); @@ -42,6 +45,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { void PreEarlyInitialization() override; void PostEarlyInitialization() override; void PreMainMessageLoopRun() override; + bool MainMessageLoopRun(int* result_code) override; void PostMainMessageLoopStart() override; void PostMainMessageLoopRun() override; #if defined(OS_MACOSX) @@ -66,6 +70,9 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { // with a task runner that will post all work to main loop. scoped_refptr bridge_task_runner_; + // Pointer to exit code. + int* exit_code_; + scoped_ptr browser_; scoped_ptr js_env_; scoped_ptr node_bindings_; diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index e80cb4e60e20..57741786520d 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -7,6 +7,7 @@ #include #include "atom/browser/atom_browser_main_parts.h" +#include "atom/browser/native_window.h" #include "atom/browser/window_list.h" #include "base/message_loop/message_loop.h" #include "content/public/browser/client_certificate_delegate.h" @@ -45,6 +46,27 @@ void Browser::Quit() { window_list->CloseAllWindows(); } +void Browser::Exit(int code) { + if (!AtomBrowserMainParts::Get()->SetExitCode(code)) { + // Message loop is not ready, quit directly. + exit(code); + } else { + // Prepare to quit when all windows have been closed.. + is_quiting_ = true; + + // Must destroy windows before quitting, otherwise bad things can happen. + atom::WindowList* window_list = atom::WindowList::GetInstance(); + if (window_list->size() == 0) { + NotifyAndShutdown(); + } else { + // Unlike Quit(), we do not ask to close window, but destroy the window + // without asking. + for (NativeWindow* window : *window_list) + window->CloseContents(nullptr); // e.g. Destroy() + } + } +} + void Browser::Shutdown() { if (is_shutdown_) return; diff --git a/atom/browser/browser.h b/atom/browser/browser.h index 447a526de6df..e20db080b67a 100644 --- a/atom/browser/browser.h +++ b/atom/browser/browser.h @@ -42,6 +42,9 @@ class Browser : public WindowListObserver { // Try to close all windows and quit the application. void Quit(); + // Exit the application immediately and set exit code. + void Exit(int code); + // Cleanup everything and shutdown the application gracefully. void Shutdown(); diff --git a/atom/browser/lib/init.coffee b/atom/browser/lib/init.coffee index 9f92d700c73a..80d2da31b705 100644 --- a/atom/browser/lib/init.coffee +++ b/atom/browser/lib/init.coffee @@ -53,6 +53,9 @@ app = require 'app' app.on 'quit', -> process.emit 'exit' +# Map process.exit to app.exit, which quits gracefully. +process.exit = app.exit + # Load the RPC server. require './rpc-server' diff --git a/docs/api/app.md b/docs/api/app.md index 00aade7c54ab..fdb9f9980592 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -208,6 +208,15 @@ This method guarantees that all `beforeunload` and `unload` event handlers are correctly executed. It is possible that a window cancels the quitting by returning `false` in the `beforeunload` event handler. +### `app.exit(exitCode)` + +* `exitCode` Integer + +Exits immediately with `exitCode`. + +All windows will be closed immediately without asking user and the `before-quit` +and `will-quit` events will not be emitted. + ### `app.getAppPath()` Returns the current application directory. From acf4372cf7d8a07a9c55763ac2bb726e962aa251 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Nov 2015 18:51:34 +0800 Subject: [PATCH 2/4] win: Add ELECTRON_DEFAULT_ERROR_MODE env It is useful to help debugging crashes without opening debugger. --- atom/common/node_bindings.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/atom/common/node_bindings.cc b/atom/common/node_bindings.cc index 10da202da70d..dbd0bd8d96ee 100644 --- a/atom/common/node_bindings.cc +++ b/atom/common/node_bindings.cc @@ -14,6 +14,7 @@ #include "atom/common/node_includes.h" #include "base/command_line.h" #include "base/base_paths.h" +#include "base/environment.h" #include "base/files/file_path.h" #include "base/message_loop/message_loop.h" #include "base/path_service.h" @@ -141,6 +142,14 @@ void NodeBindings::Initialize() { // Init node. // (we assume node::Init would not modify the parameters under embedded mode). node::Init(nullptr, nullptr, nullptr, nullptr); + +#if defined(OS_WIN) + // uv_init overrides error mode to suppress the default crash dialog, bring + // it back if user wants to show it. + scoped_ptr env(base::Environment::Create()); + if (env->HasVar("ELECTRON_DEFAULT_ERROR_MODE")) + SetErrorMode(0); +#endif } node::Environment* NodeBindings::CreateEnvironment( From d70706f8765033138f4e19eef8c6ef723b217bc6 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Nov 2015 20:23:41 +0800 Subject: [PATCH 3/4] Make sure handles of callbacks are releases on exit Some callbacks are stored in native resources that not managed by JavaScript, so when those resources are destroyed the JavaScript context may already be destroyed, and releasing callbacks then would result in crash. --- .../common/native_mate_converters/callback.cc | 29 +++++++++++ atom/common/native_mate_converters/callback.h | 48 ++++++++++++++----- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/atom/common/native_mate_converters/callback.cc b/atom/common/native_mate_converters/callback.cc index 3bcb748689fb..87faa3df3cd5 100644 --- a/atom/common/native_mate_converters/callback.cc +++ b/atom/common/native_mate_converters/callback.cc @@ -4,6 +4,8 @@ #include "atom/common/native_mate_converters/callback.h" +#include "atom/browser/atom_browser_main_parts.h" + namespace mate { namespace internal { @@ -54,6 +56,33 @@ v8::Local BindFunctionWith(v8::Isolate* isolate, } // namespace +SafeV8Function::SafeV8Function(v8::Isolate* isolate, v8::Local value) + : v8_function_(new RefCountedPersistent(isolate, value)), + weak_factory_(this) { + Init(); +} + +SafeV8Function::SafeV8Function(const SafeV8Function& other) + : v8_function_(other.v8_function_), + weak_factory_(this) { + Init(); +} + +v8::Local SafeV8Function::NewHandle() const { + return v8_function_->NewHandle(); +} + +void SafeV8Function::Init() { + if (Locker::IsBrowserProcess() && atom::AtomBrowserMainParts::Get()) + atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback( + base::Bind(&SafeV8Function::FreeHandle, weak_factory_.GetWeakPtr())); +} + +void SafeV8Function::FreeHandle() { + Locker locker(v8_function_->isolate()); + v8_function_ = nullptr; +} + v8::Local CreateFunctionFromTranslater( v8::Isolate* isolate, const Translater& translater) { // The FunctionTemplate is cached. diff --git a/atom/common/native_mate_converters/callback.h b/atom/common/native_mate_converters/callback.h index 228fc0d3bb1d..5dd9d3cec9b1 100644 --- a/atom/common/native_mate_converters/callback.h +++ b/atom/common/native_mate_converters/callback.h @@ -10,6 +10,7 @@ #include "atom/common/api/locker.h" #include "base/bind.h" #include "base/callback.h" +#include "base/memory/weak_ptr.h" #include "native_mate/function_template.h" #include "native_mate/scoped_persistent.h" #include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h" @@ -18,7 +19,24 @@ namespace mate { namespace internal { -typedef scoped_refptr > SafeV8Function; +// Manages the V8 function with RAII, and automatically cleans the handle when +// JavaScript context is destroyed, even when the class is not destroyed. +class SafeV8Function { + public: + SafeV8Function(v8::Isolate* isolate, v8::Local value); + SafeV8Function(const SafeV8Function& other); + + bool is_alive() const { return v8_function_.get(); } + + v8::Local NewHandle() const; + + private: + void Init(); + void FreeHandle(); + + scoped_refptr> v8_function_; + base::WeakPtrFactory weak_factory_; +}; // Helper to invoke a V8 function with C++ parameters. template @@ -26,14 +44,17 @@ struct V8FunctionInvoker {}; template struct V8FunctionInvoker(ArgTypes...)> { - static v8::Local Go(v8::Isolate* isolate, SafeV8Function function, + static v8::Local Go(v8::Isolate* isolate, + const SafeV8Function& function, ArgTypes... raw) { Locker locker(isolate); v8::EscapableHandleScope handle_scope(isolate); + if (!function.is_alive()) + return v8::Null(isolate); scoped_ptr script_scope( Locker::IsBrowserProcess() ? nullptr : new blink::WebScopedRunV8Script(isolate)); - v8::Local holder = function->NewHandle(); + v8::Local holder = function.NewHandle(); v8::Local context = holder->CreationContext(); v8::Context::Scope context_scope(context); std::vector> args = { ConvertToV8(isolate, raw)... }; @@ -44,14 +65,17 @@ struct V8FunctionInvoker(ArgTypes...)> { template struct V8FunctionInvoker { - static void Go(v8::Isolate* isolate, SafeV8Function function, + static void Go(v8::Isolate* isolate, + const SafeV8Function& function, ArgTypes... raw) { Locker locker(isolate); v8::HandleScope handle_scope(isolate); + if (!function.is_alive()) + return; scoped_ptr script_scope( Locker::IsBrowserProcess() ? nullptr : new blink::WebScopedRunV8Script(isolate)); - v8::Local holder = function->NewHandle(); + v8::Local holder = function.NewHandle(); v8::Local context = holder->CreationContext(); v8::Context::Scope context_scope(context); std::vector> args = { ConvertToV8(isolate, raw)... }; @@ -61,17 +85,20 @@ struct V8FunctionInvoker { template struct V8FunctionInvoker { - static ReturnType Go(v8::Isolate* isolate, SafeV8Function function, + static ReturnType Go(v8::Isolate* isolate, + const SafeV8Function& function, ArgTypes... raw) { Locker locker(isolate); v8::HandleScope handle_scope(isolate); + ReturnType ret = ReturnType(); + if (!function.is_alive()) + return ret; scoped_ptr script_scope( Locker::IsBrowserProcess() ? nullptr : new blink::WebScopedRunV8Script(isolate)); - v8::Local holder = function->NewHandle(); + v8::Local holder = function.NewHandle(); v8::Local context = holder->CreationContext(); v8::Context::Scope context_scope(context); - ReturnType ret = ReturnType(); std::vector> args = { ConvertToV8(isolate, raw)... }; v8::Local result; auto maybe_result = @@ -119,9 +146,8 @@ struct Converter> { if (!val->IsFunction()) return false; - internal::SafeV8Function function( - new RefCountedPersistent(isolate, val)); - *out = base::Bind(&internal::V8FunctionInvoker::Go, isolate, function); + *out = base::Bind(&internal::V8FunctionInvoker::Go, + isolate, internal::SafeV8Function(isolate, val)); return true; } }; From c4ee8c1b1f8d335931f6a8fdce6a37ecab2e3d07 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Nov 2015 21:00:38 +0800 Subject: [PATCH 4/4] spec: Fix refreshing on Windows --- spec/api-crash-reporter-spec.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/api-crash-reporter-spec.coffee b/spec/api-crash-reporter-spec.coffee index 9aedb0c8ea1e..618adaaf9dec 100644 --- a/spec/api-crash-reporter-spec.coffee +++ b/spec/api-crash-reporter-spec.coffee @@ -53,5 +53,6 @@ describe 'crash-reporter module', -> protocol: 'file' pathname: path.join fixtures, 'api', 'crash.html' search: "?port=#{port}" - crashReporter.start {'submitUrl': 'http://127.0.0.1:' + port} + if process.platform is 'darwin' + crashReporter.start {'submitUrl': 'http://127.0.0.1:' + port} w.loadUrl url