Skip to content

Commit

Permalink
src: disallow JS execution inside FreeEnvironment
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and jasnell committed Jun 25, 2020
1 parent 409fdba commit 0fb91ac
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 6 deletions.
8 changes: 6 additions & 2 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand Down
4 changes: 0 additions & 4 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down
15 changes: 15 additions & 0 deletions test/addons/worker-addon/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down

0 comments on commit 0fb91ac

Please sign in to comment.