Skip to content

Commit 4286dcf

Browse files
addaleaxaddaleax
committed
src: refactor Environment::GetCurrent() usage
Make `Environment::GetCurrent()` return `nullptr` if the current `Context` is not a Node.js context, and for the relevant usage of this function, either: - Switch to the better `GetCurrent(args)` variant - Turn functions in to no-ops where it makes sense - Make it a `CHECK`, i.e. an API requirement, where it make sense - Leave a `TODO` comment for verifying what, if anything, is to be done PR-URL: #22819 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
1 parent c33e27d commit 4286dcf

15 files changed

+81
-37
lines changed

‎src/async_wrap.cc

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -687,12 +687,16 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
687687

688688

689689
async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
690-
return Environment::GetCurrent(isolate)->execution_async_id();
690+
Environment* env = Environment::GetCurrent(isolate);
691+
if (env == nullptr) return -1;
692+
return env->execution_async_id();
691693
}
692694

693695

694696
async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
695-
return Environment::GetCurrent(isolate)->trigger_async_id();
697+
Environment* env = Environment::GetCurrent(isolate);
698+
if (env == nullptr) return -1;
699+
return env->trigger_async_id();
696700
}
697701

698702

@@ -711,6 +715,7 @@ async_context EmitAsyncInit(Isolate* isolate,
711715
v8::Local<v8::String> name,
712716
async_id trigger_async_id) {
713717
Environment* env = Environment::GetCurrent(isolate);
718+
CHECK_NOT_NULL(env);
714719

715720
// Initialize async context struct
716721
if (trigger_async_id == -1)

‎src/bootstrapper.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
6767
PromiseRejectEvent event = message.GetEvent();
6868

6969
Environment* env = Environment::GetCurrent(isolate);
70+
if (env == nullptr) return;
7071
Local<Function> callback;
7172
Local<Value> value;
7273

‎src/callback_scope.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
4343
object_(object),
4444
callback_scope_(env) {
4545
CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty());
46+
CHECK_NOT_NULL(env);
4647

4748
if (!env->can_call_into_js()) {
4849
failed_ = true;

‎src/env-inl.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,15 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {
294294
}
295295

296296
inline Environment* Environment::GetCurrent(v8::Local<v8::Context> context) {
297+
if (UNLIKELY(context.IsEmpty() ||
298+
context->GetNumberOfEmbedderDataFields() <
299+
ContextEmbedderIndex::kContextTag ||
300+
context->GetAlignedPointerFromEmbedderData(
301+
ContextEmbedderIndex::kContextTag) !=
302+
Environment::kNodeContextTagPtr)) {
303+
return nullptr;
304+
}
305+
297306
return static_cast<Environment*>(
298307
context->GetAlignedPointerFromEmbedderData(
299308
ContextEmbedderIndex::kEnvironment));

‎src/env.cc

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -491,18 +491,9 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type,
491491
v8::Local<v8::Value> parent) {
492492
Local<v8::Context> context = promise->CreationContext();
493493

494-
// Grow the embedder data if necessary to make sure we are not out of bounds
495-
// when reading the magic number.
496-
context->SetAlignedPointerInEmbedderData(
497-
ContextEmbedderIndex::kContextTagBoundary, nullptr);
498-
int* magicNumberPtr = reinterpret_cast<int*>(
499-
context->GetAlignedPointerFromEmbedderData(
500-
ContextEmbedderIndex::kContextTag));
501-
if (magicNumberPtr != Environment::kNodeContextTagPtr) {
502-
return;
503-
}
504-
505494
Environment* env = Environment::GetCurrent(context);
495+
if (env == nullptr) return;
496+
506497
for (const PromiseHookCallback& hook : env->promise_hooks_) {
507498
hook.cb_(type, promise, parent, hook.arg_);
508499
}

‎src/inspector_js_api.cc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -149,12 +149,11 @@ void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
149149
}
150150

151151
void InspectorConsoleCall(const FunctionCallbackInfo<Value>& info) {
152-
Isolate* isolate = info.GetIsolate();
153-
HandleScope handle_scope(isolate);
152+
Environment* env = Environment::GetCurrent(info);
153+
Isolate* isolate = env->isolate();
154154
Local<Context> context = isolate->GetCurrentContext();
155155
CHECK_LT(2, info.Length());
156156
SlicedArguments call_args(info, /* start */ 3);
157-
Environment* env = Environment::GetCurrent(isolate);
158157
if (InspectorEnabled(env)) {
159158
Local<Value> inspector_method = info[0];
160159
CHECK(inspector_method->IsFunction());

‎src/module_wrap.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,7 @@ MaybeLocal<Module> ModuleWrap::ResolveCallback(Local<Context> context,
362362
Local<String> specifier,
363363
Local<Module> referrer) {
364364
Environment* env = Environment::GetCurrent(context);
365+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
365366
Isolate* isolate = env->isolate();
366367
if (env->module_map.count(referrer->GetIdentityHash()) == 0) {
367368
env->ThrowError("linking error, unknown module");
@@ -700,6 +701,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(
700701
Local<String> specifier) {
701702
Isolate* iso = context->GetIsolate();
702703
Environment* env = Environment::GetCurrent(context);
704+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
703705
v8::EscapableHandleScope handle_scope(iso);
704706

705707
if (env->context() != context) {
@@ -750,8 +752,8 @@ void ModuleWrap::SetImportModuleDynamicallyCallback(
750752

751753
void ModuleWrap::HostInitializeImportMetaObjectCallback(
752754
Local<Context> context, Local<Module> module, Local<Object> meta) {
753-
Isolate* isolate = context->GetIsolate();
754755
Environment* env = Environment::GetCurrent(context);
756+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
755757
ModuleWrap* module_wrap = GetFromModule(env, module);
756758

757759
if (module_wrap == nullptr) {
@@ -762,7 +764,7 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback(
762764
Local<Function> callback =
763765
env->host_initialize_import_meta_object_callback();
764766
Local<Value> args[] = { wrap, meta };
765-
callback->Call(context, Undefined(isolate), arraysize(args), args)
767+
callback->Call(context, Undefined(env->isolate()), arraysize(args), args)
766768
.ToLocalChecked();
767769
}
768770

‎src/node.cc

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,8 @@ namespace {
630630
bool ShouldAbortOnUncaughtException(Isolate* isolate) {
631631
HandleScope scope(isolate);
632632
Environment* env = Environment::GetCurrent(isolate);
633-
return env->should_abort_on_uncaught_toggle()[0] &&
633+
return env != nullptr &&
634+
env->should_abort_on_uncaught_toggle()[0] &&
634635
!env->inside_should_not_abort_on_uncaught_scope();
635636
}
636637

@@ -639,13 +640,15 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
639640

640641
void AddPromiseHook(Isolate* isolate, promise_hook_func fn, void* arg) {
641642
Environment* env = Environment::GetCurrent(isolate);
643+
CHECK_NOT_NULL(env);
642644
env->AddPromiseHook(fn, arg);
643645
}
644646

645647
void AddEnvironmentCleanupHook(Isolate* isolate,
646648
void (*fun)(void* arg),
647649
void* arg) {
648650
Environment* env = Environment::GetCurrent(isolate);
651+
CHECK_NOT_NULL(env);
649652
env->AddCleanupHook(fun, arg);
650653
}
651654

@@ -654,6 +657,7 @@ void RemoveEnvironmentCleanupHook(Isolate* isolate,
654657
void (*fun)(void* arg),
655658
void* arg) {
656659
Environment* env = Environment::GetCurrent(isolate);
660+
CHECK_NOT_NULL(env);
657661
env->RemoveCleanupHook(fun, arg);
658662
}
659663

@@ -738,6 +742,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
738742
// Because of the AssignToContext() call in src/node_contextify.cc,
739743
// the two contexts need not be the same.
740744
Environment* env = Environment::GetCurrent(callback->CreationContext());
745+
CHECK_NOT_NULL(env);
741746
Context::Scope context_scope(env->context());
742747
MaybeLocal<Value> ret = InternalMakeCallback(env, recv, callback,
743748
argc, argv, asyncContext);
@@ -1377,6 +1382,7 @@ void FatalException(Isolate* isolate,
13771382
HandleScope scope(isolate);
13781383

13791384
Environment* env = Environment::GetCurrent(isolate);
1385+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
13801386
Local<Object> process_object = env->process_object();
13811387
Local<String> fatal_exception_string = env->fatal_exception_string();
13821388
Local<Value> fatal_exception_function =
@@ -1602,7 +1608,7 @@ static void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
16021608
}
16031609

16041610
static void GetLinkedBinding(const FunctionCallbackInfo<Value>& args) {
1605-
Environment* env = Environment::GetCurrent(args.GetIsolate());
1611+
Environment* env = Environment::GetCurrent(args);
16061612

16071613
CHECK(args[0]->IsString());
16081614

@@ -2706,10 +2712,13 @@ void RunAtExit(Environment* env) {
27062712

27072713
uv_loop_t* GetCurrentEventLoop(Isolate* isolate) {
27082714
HandleScope handle_scope(isolate);
2709-
auto context = isolate->GetCurrentContext();
2715+
Local<Context> context = isolate->GetCurrentContext();
27102716
if (context.IsEmpty())
27112717
return nullptr;
2712-
return Environment::GetCurrent(context)->event_loop();
2718+
Environment* env = Environment::GetCurrent(context);
2719+
if (env == nullptr)
2720+
return nullptr;
2721+
return env->event_loop();
27132722
}
27142723

27152724

‎src/node_api.cc

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -946,7 +946,7 @@ class ThreadSafeFunction : public node::AsyncResource {
946946
return napi_ok;
947947
}
948948

949-
node::Environment::GetCurrent(env->isolate)->CloseHandle(
949+
NodeEnv()->CloseHandle(
950950
reinterpret_cast<uv_handle_t*>(&async),
951951
[](uv_handle_t* handle) -> void {
952952
ThreadSafeFunction* ts_fn =
@@ -1036,9 +1036,12 @@ class ThreadSafeFunction : public node::AsyncResource {
10361036
}
10371037

10381038
node::Environment* NodeEnv() {
1039-
// For some reason grabbing the Node.js environment requires a handle scope.
1039+
// Grabbing the Node.js environment requires a handle scope because it
1040+
// looks up fields on the current context.
10401041
v8::HandleScope scope(env->isolate);
1041-
return node::Environment::GetCurrent(env->isolate);
1042+
node::Environment* node_env = node::Environment::GetCurrent(env->isolate);
1043+
CHECK_NOT_NULL(node_env);
1044+
return node_env;
10421045
}
10431046

10441047
void MaybeStartIdle() {
@@ -1234,7 +1237,9 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
12341237
v8::Local<v8::Context> context,
12351238
napi_addon_register_func init) {
12361239
if (init == nullptr) {
1237-
node::Environment::GetCurrent(context)->ThrowError(
1240+
node::Environment* node_env = node::Environment::GetCurrent(context);
1241+
CHECK_NOT_NULL(node_env);
1242+
node_env->ThrowError(
12381243
"Module has no declared entry point.");
12391244
return;
12401245
}

‎src/node_buffer.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,9 @@ MaybeLocal<Object> New(Isolate* isolate,
274274
MaybeLocal<Object> New(Isolate* isolate, size_t length) {
275275
EscapableHandleScope handle_scope(isolate);
276276
Local<Object> obj;
277-
if (Buffer::New(Environment::GetCurrent(isolate), length).ToLocal(&obj))
277+
Environment* env = Environment::GetCurrent(isolate);
278+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
279+
if (Buffer::New(env, length).ToLocal(&obj))
278280
return handle_scope.Escape(obj);
279281
return Local<Object>();
280282
}
@@ -316,6 +318,7 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
316318
MaybeLocal<Object> Copy(Isolate* isolate, const char* data, size_t length) {
317319
EscapableHandleScope handle_scope(isolate);
318320
Environment* env = Environment::GetCurrent(isolate);
321+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
319322
Local<Object> obj;
320323
if (Buffer::Copy(env, data, length).ToLocal(&obj))
321324
return handle_scope.Escape(obj);
@@ -365,6 +368,7 @@ MaybeLocal<Object> New(Isolate* isolate,
365368
void* hint) {
366369
EscapableHandleScope handle_scope(isolate);
367370
Environment* env = Environment::GetCurrent(isolate);
371+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
368372
Local<Object> obj;
369373
if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
370374
return handle_scope.Escape(obj);
@@ -403,6 +407,7 @@ MaybeLocal<Object> New(Environment* env,
403407
MaybeLocal<Object> New(Isolate* isolate, char* data, size_t length) {
404408
EscapableHandleScope handle_scope(isolate);
405409
Environment* env = Environment::GetCurrent(isolate);
410+
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
406411
Local<Object> obj;
407412
if (Buffer::New(env, data, length).ToLocal(&obj))
408413
return handle_scope.Escape(obj);

‎src/node_context_data.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,11 @@ namespace node {
2525
#define NODE_CONTEXT_TAG 35
2626
#endif
2727

28-
#ifndef NODE_CONTEXT_TAG_BOUNDARY
29-
#define NODE_CONTEXT_TAG_BOUNDARY 36
30-
#endif
31-
3228
enum ContextEmbedderIndex {
3329
kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
3430
kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
3531
kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX,
3632
kContextTag = NODE_CONTEXT_TAG,
37-
kContextTagBoundary = NODE_CONTEXT_TAG_BOUNDARY,
3833
};
3934

4035
} // namespace node

‎src/node_file.cc

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ void FSReqCallback::SetReturnValue(const FunctionCallbackInfo<Value>& args) {
441441

442442
void NewFSReqCallback(const FunctionCallbackInfo<Value>& args) {
443443
CHECK(args.IsConstructCall());
444-
Environment* env = Environment::GetCurrent(args.GetIsolate());
444+
Environment* env = Environment::GetCurrent(args);
445445
new FSReqCallback(env, args.This(), args[0]->IsTrue());
446446
}
447447

@@ -782,7 +782,7 @@ inline FSReqBase* GetReqWrap(Environment* env, Local<Value> value,
782782
}
783783

784784
void Access(const FunctionCallbackInfo<Value>& args) {
785-
Environment* env = Environment::GetCurrent(args.GetIsolate());
785+
Environment* env = Environment::GetCurrent(args);
786786
HandleScope scope(env->isolate());
787787

788788
const int argc = args.Length();
@@ -2234,8 +2234,7 @@ void Initialize(Local<Object> target,
22342234
StatWatcher::Initialize(env, target);
22352235

22362236
// Create FunctionTemplate for FSReqCallback
2237-
Local<FunctionTemplate> fst =
2238-
FunctionTemplate::New(env->isolate(), NewFSReqCallback);
2237+
Local<FunctionTemplate> fst = env->NewFunctionTemplate(NewFSReqCallback);
22392238
fst->InstanceTemplate()->SetInternalFieldCount(1);
22402239
AsyncWrap::AddWrapMethods(env, fst);
22412240
Local<String> wrapString =

‎src/node_internals.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,9 @@ class InternalCallbackScope {
501501

502502
class ThreadPoolWork {
503503
public:
504-
explicit inline ThreadPoolWork(Environment* env) : env_(env) {}
504+
explicit inline ThreadPoolWork(Environment* env) : env_(env) {
505+
CHECK_NOT_NULL(env);
506+
}
505507
inline virtual ~ThreadPoolWork() = default;
506508

507509
inline void ScheduleWork();

‎src/node_perf.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ void TimerFunctionCall(const FunctionCallbackInfo<Value>& args) {
310310
Isolate* isolate = args.GetIsolate();
311311
HandleScope scope(isolate);
312312
Environment* env = Environment::GetCurrent(isolate);
313+
CHECK_NOT_NULL(env); // TODO(addaleax): Verify that this is correct.
313314
Local<Context> context = env->context();
314315
Local<Function> fn = args.Data().As<Function>();
315316
size_t count = args.Length();

‎test/cctest/test_environment.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,26 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
7070
EXPECT_TRUE(called_cb_2);
7171
}
7272

73+
TEST_F(EnvironmentTest, NonNodeJSContext) {
74+
const v8::HandleScope handle_scope(isolate_);
75+
const Argv argv;
76+
Env test_env {handle_scope, argv};
77+
78+
EXPECT_EQ(node::Environment::GetCurrent(v8::Local<v8::Context>()), nullptr);
79+
80+
node::Environment* env = *test_env;
81+
EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);
82+
EXPECT_EQ(node::Environment::GetCurrent(env->context()), env);
83+
84+
v8::Local<v8::Context> context = v8::Context::New(isolate_);
85+
EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
86+
EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);
87+
88+
v8::Context::Scope context_scope(context);
89+
EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
90+
EXPECT_EQ(node::Environment::GetCurrent(isolate_), nullptr);
91+
}
92+
7393
static void at_exit_callback1(void* arg) {
7494
called_cb_1 = true;
7595
if (arg) {

0 commit comments

Comments
 (0)