Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 0fb91ac

Browse filesBrowse files
addaleaxjasnell
authored andcommitted
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>
1 parent 409fdba commit 0fb91ac
Copy full SHA for 0fb91ac

File tree

Expand file treeCollapse file tree

3 files changed

+21
-6
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+21
-6
lines changed
Open diff view settings
Collapse file

‎src/api/environment.cc‎

Copy file name to clipboardExpand all lines: src/api/environment.cc
+6-2Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ using v8::Object;
2727
using v8::ObjectTemplate;
2828
using v8::Private;
2929
using v8::PropertyDescriptor;
30+
using v8::SealHandleScope;
3031
using v8::String;
3132
using v8::Value;
3233

@@ -378,10 +379,13 @@ Environment* CreateEnvironment(
378379
}
379380

380381
void FreeEnvironment(Environment* env) {
382+
Isolate::DisallowJavascriptExecutionScope disallow_js(env->isolate(),
383+
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
381384
{
382-
// TODO(addaleax): This should maybe rather be in a SealHandleScope.
383-
HandleScope handle_scope(env->isolate());
385+
HandleScope handle_scope(env->isolate()); // For env->context().
384386
Context::Scope context_scope(env->context());
387+
SealHandleScope seal_handle_scope(env->isolate());
388+
385389
env->set_stopping(true);
386390
env->stop_sub_worker_contexts();
387391
env->RunCleanup();
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
-4Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,10 +274,6 @@ void Worker::Run() {
274274
this->env_ = nullptr;
275275
}
276276

277-
// TODO(addaleax): Try moving DisallowJavascriptExecutionScope into
278-
// FreeEnvironment().
279-
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_,
280-
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
281277
env_.reset();
282278
});
283279

Collapse file

‎test/addons/worker-addon/binding.cc‎

Copy file name to clipboardExpand all lines: test/addons/worker-addon/binding.cc
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,13 @@
66
#include <uv.h>
77

88
using v8::Context;
9+
using v8::Function;
910
using v8::HandleScope;
1011
using v8::Isolate;
1112
using v8::Local;
13+
using v8::MaybeLocal;
1214
using v8::Object;
15+
using v8::String;
1316
using v8::Value;
1417

1518
size_t count = 0;
@@ -31,6 +34,18 @@ void Dummy(void*) {
3134

3235
void Cleanup(void* str) {
3336
printf("%s ", static_cast<const char*>(str));
37+
38+
// Check that calling into JS fails.
39+
Isolate* isolate = Isolate::GetCurrent();
40+
HandleScope handle_scope(isolate);
41+
assert(isolate->InContext());
42+
Local<Context> context = isolate->GetCurrentContext();
43+
MaybeLocal<Value> call_result =
44+
context->Global()->Get(
45+
context, String::NewFromUtf8Literal(isolate, "Object"))
46+
.ToLocalChecked().As<Function>()->Call(
47+
context, v8::Null(isolate), 0, nullptr);
48+
assert(call_result.IsEmpty());
3449
}
3550

3651
void Initialize(Local<Object> exports,

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.