From 6912d226c9112537e45cd029c2111aaa153eb0aa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 14 Jun 2020 17:04:13 +0200 Subject: [PATCH] src: disallow JS execution inside FreeEnvironment This addresses a TODO comment, and aligns the behavior between worker threads and the main thread. The primary motivation for this change is to more strictly enforce the invariant that no JS runs after the `'exit'` event is emitted. --- src/api/environment.cc | 8 ++++++-- src/node_worker.cc | 4 ---- test/addons/worker-addon/binding.cc | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index a574b916fe6f00..210abb94295f53 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -27,6 +27,7 @@ using v8::Object; using v8::ObjectTemplate; using v8::Private; using v8::PropertyDescriptor; +using v8::SealHandleScope; using v8::String; using v8::Value; @@ -378,10 +379,13 @@ Environment* CreateEnvironment( } void FreeEnvironment(Environment* env) { + Isolate::DisallowJavascriptExecutionScope disallow_js(env->isolate(), + Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); { - // TODO(addaleax): This should maybe rather be in a SealHandleScope. - HandleScope handle_scope(env->isolate()); + HandleScope handle_scope(env->isolate()); // For env->context(). Context::Scope context_scope(env->context()); + SealHandleScope seal_handle_scope(env->isolate()); + env->set_stopping(true); env->stop_sub_worker_contexts(); env->RunCleanup(); diff --git a/src/node_worker.cc b/src/node_worker.cc index 403c054ab21e77..fa68fe88dc89f4 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -274,10 +274,6 @@ void Worker::Run() { this->env_ = nullptr; } - // TODO(addaleax): Try moving DisallowJavascriptExecutionScope into - // FreeEnvironment(). - Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, - Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); env_.reset(); }); diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index 01c857c43ebcfc..63728fa9a4b76d 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -6,10 +6,13 @@ #include using v8::Context; +using v8::Function; using v8::HandleScope; using v8::Isolate; using v8::Local; +using v8::MaybeLocal; using v8::Object; +using v8::String; using v8::Value; size_t count = 0; @@ -31,6 +34,18 @@ void Dummy(void*) { void Cleanup(void* str) { printf("%s ", static_cast(str)); + + // Check that calling into JS fails. + Isolate* isolate = Isolate::GetCurrent(); + HandleScope handle_scope(isolate); + assert(isolate->InContext()); + Local context = isolate->GetCurrentContext(); + MaybeLocal call_result = + context->Global()->Get( + context, String::NewFromUtf8Literal(isolate, "Object")) + .ToLocalChecked().As()->Call( + context, v8::Null(isolate), 0, nullptr); + assert(call_result.IsEmpty()); } void Initialize(Local exports,