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. PR-URL: #33874 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
nodejs · Jun 25, 2020 · 0fb91acedff792463dd4f8a67c60cc645e6aa5c3 · 0fb91ac
1 parent 409fdba
commit 0fb91acedff792463dd4f8a67c60cc645e6aa5c3
Showing with 21 additions and 6 deletions.

+6 −2 src/api/environment.cc

+0 −4 src/node_worker.cc

+15 −0 test/addons/worker-addon/binding.cc
diff --git 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
@@ -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
@@ -6,10 +6,13 @@
 #include <uv.h>
 
 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<const char*>(str));
+
+  // Check that calling into JS fails.
+  Isolate* isolate = Isolate::GetCurrent();
+  HandleScope handle_scope(isolate);
+  assert(isolate->InContext());
+  Local<Context> context = isolate->GetCurrentContext();
+  MaybeLocal<Value> call_result =
+      context->Global()->Get(
+          context, String::NewFromUtf8Literal(isolate, "Object"))
+              .ToLocalChecked().As<Function>()->Call(
+                  context, v8::Null(isolate), 0, nullptr);
+  assert(call_result.IsEmpty());
 }
 
 void Initialize(Local<Object> exports,