From a0605275b91e9e0dc6d9a5c5bcd03960de26ca25 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 28 Feb 2017 09:56:09 +0900 Subject: [PATCH 1/6] Clean up node environment on exit in main process --- atom/browser/atom_browser_main_parts.cc | 1 + atom/browser/atom_browser_main_parts.h | 2 ++ atom/browser/javascript_environment.cc | 9 +++++++++ atom/browser/javascript_environment.h | 17 +++++++++++++++++ 4 files changed, 29 insertions(+) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index e386e678a47c..4031fb27cf30 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -132,6 +132,7 @@ void AtomBrowserMainParts::PostEarlyInitialization() { // Create the global environment. node::Environment* env = node_bindings_->CreateEnvironment(js_env_->context()); + node_env_.reset(new NodeEnvironment(env)); // Make sure node can get correct environment when debugging. if (node_debugger_->IsRunning()) diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index 0d8619f6865a..71adc43d5e65 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/atom/browser/atom_browser_main_parts.h @@ -22,6 +22,7 @@ class Browser; class JavascriptEnvironment; class NodeBindings; class NodeDebugger; +class NodeEnvironment; class BridgeTaskRunner; class AtomBrowserMainParts : public brightray::BrowserMainParts { @@ -79,6 +80,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { std::unique_ptr browser_; std::unique_ptr js_env_; + std::unique_ptr node_env_; std::unique_ptr node_bindings_; std::unique_ptr atom_bindings_; std::unique_ptr node_debugger_; diff --git a/atom/browser/javascript_environment.cc b/atom/browser/javascript_environment.cc index 3cdd2c771ee6..b3e01c1c3009 100644 --- a/atom/browser/javascript_environment.cc +++ b/atom/browser/javascript_environment.cc @@ -12,6 +12,8 @@ #include "gin/array_buffer.h" #include "gin/v8_initializer.h" +#include "atom/common/node_includes.h" + namespace atom { JavascriptEnvironment::JavascriptEnvironment() @@ -46,4 +48,11 @@ bool JavascriptEnvironment::Initialize() { return true; } +NodeEnvironment::NodeEnvironment(node::Environment* env) : env_(env) { +} + +NodeEnvironment::~NodeEnvironment() { + node::FreeEnvironment(env_); +} + } // namespace atom diff --git a/atom/browser/javascript_environment.h b/atom/browser/javascript_environment.h index 1f4d2f453478..046ba3e29c8f 100644 --- a/atom/browser/javascript_environment.h +++ b/atom/browser/javascript_environment.h @@ -8,8 +8,13 @@ #include "base/macros.h" #include "gin/public/isolate_holder.h" +namespace node { +class Environment; +} + namespace atom { +// Manage the V8 isolate and context automatically. class JavascriptEnvironment { public: JavascriptEnvironment(); @@ -37,6 +42,18 @@ class JavascriptEnvironment { DISALLOW_COPY_AND_ASSIGN(JavascriptEnvironment); }; +// Manage the Node Environment automatically. +class NodeEnvironment { + public: + NodeEnvironment(node::Environment* env); + ~NodeEnvironment(); + + private: + node::Environment* env_; + + DISALLOW_COPY_AND_ASSIGN(NodeEnvironment); +}; + } // namespace atom #endif // ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_ From d379b05890ef0a461dbc37c5026227b89f81f23f Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 28 Feb 2017 10:58:52 +0900 Subject: [PATCH 2/6] async handles should be closed on exit --- atom/common/api/atom_bindings.cc | 1 + atom/common/node_bindings.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/atom/common/api/atom_bindings.cc b/atom/common/api/atom_bindings.cc index 2b4bec6328d9..b747fabe9dee 100644 --- a/atom/common/api/atom_bindings.cc +++ b/atom/common/api/atom_bindings.cc @@ -84,6 +84,7 @@ AtomBindings::AtomBindings() { } AtomBindings::~AtomBindings() { + uv_close(reinterpret_cast(&call_next_tick_async_), nullptr); } void AtomBindings::BindTo(v8::Isolate* isolate, diff --git a/atom/common/node_bindings.cc b/atom/common/node_bindings.cc index 0737314d07f1..4cf2ba56e4c8 100644 --- a/atom/common/node_bindings.cc +++ b/atom/common/node_bindings.cc @@ -117,6 +117,7 @@ NodeBindings::~NodeBindings() { // Clear uv. uv_sem_destroy(&embed_sem_); + uv_close(reinterpret_cast(&dummy_uv_handle_), nullptr); } void NodeBindings::Initialize() { From 29278e500b7c5d289b9a48a85b7fd51a9fc44f96 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 2 Mar 2017 16:49:39 +0900 Subject: [PATCH 3/6] Destroy node environment when a JS context in renderer is destroyed --- atom/renderer/atom_renderer_client.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/atom/renderer/atom_renderer_client.cc b/atom/renderer/atom_renderer_client.cc index 9979aae9000b..318a9d38cdbd 100644 --- a/atom/renderer/atom_renderer_client.cc +++ b/atom/renderer/atom_renderer_client.cc @@ -381,6 +381,9 @@ void AtomRendererClient::WillReleaseScriptContext( node::Environment* env = node::Environment::GetCurrent(context); if (env) mate::EmitEvent(env->isolate(), env->process_object(), "exit"); + + // Destroy the node environment. + node::FreeEnvironment(env); } bool AtomRendererClient::ShouldFork(blink::WebLocalFrame* frame, From 24574f7299ae382b1b7a2623b8c409ef419702ed Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 2 Mar 2017 16:50:15 +0900 Subject: [PATCH 4/6] Avoid touch an Environemnt after it gets destroyed --- atom/common/api/atom_bindings.cc | 7 +++++++ atom/common/api/atom_bindings.h | 3 +++ atom/renderer/atom_renderer_client.cc | 1 + 3 files changed, 11 insertions(+) diff --git a/atom/common/api/atom_bindings.cc b/atom/common/api/atom_bindings.cc index b747fabe9dee..f1b56c675936 100644 --- a/atom/common/api/atom_bindings.cc +++ b/atom/common/api/atom_bindings.cc @@ -118,6 +118,13 @@ void AtomBindings::BindTo(v8::Isolate* isolate, } } +void AtomBindings::EnvironmentDestroyed(node::Environment* env) { + auto it = std::find(pending_next_ticks_.begin(), pending_next_ticks_.end(), + env); + if (it != pending_next_ticks_.end()) + pending_next_ticks_.erase(it); +} + void AtomBindings::ActivateUVLoop(v8::Isolate* isolate) { node::Environment* env = node::Environment::GetCurrent(isolate); if (std::find(pending_next_ticks_.begin(), pending_next_ticks_.end(), env) != diff --git a/atom/common/api/atom_bindings.h b/atom/common/api/atom_bindings.h index 58c2336c0e8b..3fd43cc8a145 100644 --- a/atom/common/api/atom_bindings.h +++ b/atom/common/api/atom_bindings.h @@ -27,6 +27,9 @@ class AtomBindings { // load native code from Electron instead. void BindTo(v8::Isolate* isolate, v8::Local process); + // Should be called when a node::Environment has been destroyed. + void EnvironmentDestroyed(node::Environment* env); + static void Log(const base::string16& message); static void Crash(); diff --git a/atom/renderer/atom_renderer_client.cc b/atom/renderer/atom_renderer_client.cc index 318a9d38cdbd..79aea0ee0b5f 100644 --- a/atom/renderer/atom_renderer_client.cc +++ b/atom/renderer/atom_renderer_client.cc @@ -384,6 +384,7 @@ void AtomRendererClient::WillReleaseScriptContext( // Destroy the node environment. node::FreeEnvironment(env); + atom_bindings_->EnvironmentDestroyed(env); } bool AtomRendererClient::ShouldFork(blink::WebLocalFrame* frame, From 1709e74958dfcbf070af046592e805cdbd152689 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 2 Mar 2017 17:18:00 +0900 Subject: [PATCH 5/6] Fix crash when the main frame is replaced --- atom/renderer/atom_renderer_client.cc | 15 +++++++++------ atom/renderer/atom_renderer_client.h | 3 +++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/atom/renderer/atom_renderer_client.cc b/atom/renderer/atom_renderer_client.cc index 79aea0ee0b5f..cbea7f7a5660 100644 --- a/atom/renderer/atom_renderer_client.cc +++ b/atom/renderer/atom_renderer_client.cc @@ -206,7 +206,8 @@ std::vector ParseSchemesCLISwitch(const char* switch_name) { } // namespace AtomRendererClient::AtomRendererClient() - : node_bindings_(NodeBindings::Create(false)), + : node_integration_initialized_(false), + node_bindings_(NodeBindings::Create(false)), atom_bindings_(new AtomBindings) { isolated_world_ = base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kContextIsolation); @@ -342,11 +343,9 @@ void AtomRendererClient::DidCreateScriptContext( if (!render_frame->IsMainFrame() && !IsDevToolsExtension(render_frame)) return; - // Whether the node binding has been initialized. - bool first_time = node_bindings_->uv_env() == nullptr; - // Prepare the node bindings. - if (first_time) { + if (!node_integration_initialized_) { + node_integration_initialized_ = true; node_bindings_->Initialize(); node_bindings_->PrepareMessageLoop(); } @@ -362,7 +361,7 @@ void AtomRendererClient::DidCreateScriptContext( // Load everything. node_bindings_->LoadEnvironment(env); - if (first_time) { + if (node_bindings_->uv_env() == nullptr) { // Make uv loop being wrapped by window context. node_bindings_->set_uv_env(env); @@ -382,6 +381,10 @@ void AtomRendererClient::WillReleaseScriptContext( if (env) mate::EmitEvent(env->isolate(), env->process_object(), "exit"); + // The main frame may be replaced. + if (env == node_bindings_->uv_env()) + node_bindings_->set_uv_env(nullptr); + // Destroy the node environment. node::FreeEnvironment(env); atom_bindings_->EnvironmentDestroyed(env); diff --git a/atom/renderer/atom_renderer_client.h b/atom/renderer/atom_renderer_client.h index a693262fed1b..c1d86cddf5e3 100644 --- a/atom/renderer/atom_renderer_client.h +++ b/atom/renderer/atom_renderer_client.h @@ -66,6 +66,9 @@ class AtomRendererClient : public content::ContentRendererClient { std::vector>* key_systems) override; + // Whether the node integration has been initialized. + bool node_integration_initialized_; + std::unique_ptr node_bindings_; std::unique_ptr atom_bindings_; std::unique_ptr preferences_manager_; From cf198904a4f7824252e4f9cbaef575f50c08fd66 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 2 Mar 2017 17:26:15 +0900 Subject: [PATCH 6/6] Fix cpplint warnings --- atom/browser/javascript_environment.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/browser/javascript_environment.h b/atom/browser/javascript_environment.h index 046ba3e29c8f..43a7295f9026 100644 --- a/atom/browser/javascript_environment.h +++ b/atom/browser/javascript_environment.h @@ -45,7 +45,7 @@ class JavascriptEnvironment { // Manage the Node Environment automatically. class NodeEnvironment { public: - NodeEnvironment(node::Environment* env); + explicit NodeEnvironment(node::Environment* env); ~NodeEnvironment(); private: