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]>
nodejs · Sep 20, 2018 · 5d5c3fa · 5d5c3fa
1 parent 5c5d881
commit 5d5c3fa
Show file tree

Hide file tree

Showing 15 changed files with 82 additions and 38 deletions.
diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -681,12 +681,16 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
 
 
 async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
-  return Environment::GetCurrent(isolate)->execution_async_id();
+  Environment* env = Environment::GetCurrent(isolate);
+  if (env == nullptr) return -1;
+  return env->execution_async_id();
 }
 
 
 async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
-  return Environment::GetCurrent(isolate)->trigger_async_id();
+  Environment* env = Environment::GetCurrent(isolate);
+  if (env == nullptr) return -1;
+  return env->trigger_async_id();
 }
 
 
@@ -705,6 +709,7 @@ async_context EmitAsyncInit(Isolate* isolate,
                             v8::Local<v8::String> name,
                             async_id trigger_async_id) {
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);
 
   // Initialize async context struct
   if (trigger_async_id == -1)

diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
@@ -61,6 +61,7 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
   PromiseRejectEvent event = message.GetEvent();
 
   Environment* env = Environment::GetCurrent(isolate);
+  if (env == nullptr) return;
   Local<Function> callback;
   Local<Value> value;
 

diff --git a/src/callback_scope.cc b/src/callback_scope.cc
@@ -43,6 +43,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
     object_(object),
     callback_scope_(env) {
   CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty());
+  CHECK_NOT_NULL(env);
 
   if (!env->can_call_into_js()) {
     failed_ = true;

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -302,6 +302,15 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {
 }
 
 inline Environment* Environment::GetCurrent(v8::Local<v8::Context> context) {
+  if (UNLIKELY(context.IsEmpty() ||
+      context->GetNumberOfEmbedderDataFields() <
+          ContextEmbedderIndex::kContextTag ||
+      context->GetAlignedPointerFromEmbedderData(
+          ContextEmbedderIndex::kContextTag) !=
+          Environment::kNodeContextTagPtr)) {
+    return nullptr;
+  }
+
   return static_cast<Environment*>(
       context->GetAlignedPointerFromEmbedderData(
           ContextEmbedderIndex::kEnvironment));

diff --git a/src/env.cc b/src/env.cc
@@ -449,18 +449,9 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type,
                                  v8::Local<v8::Value> parent) {
   Local<v8::Context> context = promise->CreationContext();
 
-  // Grow the embedder data if necessary to make sure we are not out of bounds
-  // when reading the magic number.
-  context->SetAlignedPointerInEmbedderData(
-      ContextEmbedderIndex::kContextTagBoundary, nullptr);
-  int* magicNumberPtr = reinterpret_cast<int*>(
-      context->GetAlignedPointerFromEmbedderData(
-          ContextEmbedderIndex::kContextTag));
-  if (magicNumberPtr != Environment::kNodeContextTagPtr) {
-    return;
-  }
-
   Environment* env = Environment::GetCurrent(context);
+  if (env == nullptr) return;
+
   for (const PromiseHookCallback& hook : env->promise_hooks_) {
     hook.cb_(type, promise, parent, hook.arg_);
   }

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -149,12 +149,11 @@ void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
 }
 
 void InspectorConsoleCall(const FunctionCallbackInfo<Value>& info) {
-  Isolate* isolate = info.GetIsolate();
-  HandleScope handle_scope(isolate);
+  Environment* env = Environment::GetCurrent(info);
+  Isolate* isolate = env->isolate();
   Local<Context> context = isolate->GetCurrentContext();
   CHECK_LT(2, info.Length());
   SlicedArguments call_args(info, /* start */ 3);
-  Environment* env = Environment::GetCurrent(isolate);
   if (InspectorEnabled(env)) {
     Local<Value> inspector_method = info[0];
     CHECK(inspector_method->IsFunction());

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -362,6 +362,7 @@ MaybeLocal<Module> ModuleWrap::ResolveCallback(Local<Context> context,
                                                Local<String> specifier,
                                                Local<Module> referrer) {
   Environment* env = Environment::GetCurrent(context);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   Isolate* isolate = env->isolate();
   if (env->module_map.count(referrer->GetIdentityHash()) == 0) {
     env->ThrowError("linking error, unknown module");
@@ -700,6 +701,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     Local<String> specifier) {
   Isolate* iso = context->GetIsolate();
   Environment* env = Environment::GetCurrent(context);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   v8::EscapableHandleScope handle_scope(iso);
 
   if (env->context() != context) {
@@ -750,8 +752,8 @@ void ModuleWrap::SetImportModuleDynamicallyCallback(
 
 void ModuleWrap::HostInitializeImportMetaObjectCallback(
     Local<Context> context, Local<Module> module, Local<Object> meta) {
-  Isolate* isolate = context->GetIsolate();
   Environment* env = Environment::GetCurrent(context);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   ModuleWrap* module_wrap = GetFromModule(env, module);
 
   if (module_wrap == nullptr) {
@@ -762,7 +764,7 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback(
   Local<Function> callback =
       env->host_initialize_import_meta_object_callback();
   Local<Value> args[] = { wrap, meta };
-  callback->Call(context, Undefined(isolate), arraysize(args), args)
+  callback->Call(context, Undefined(env->isolate()), arraysize(args), args)
       .ToLocalChecked();
 }
 

diff --git a/src/node.cc b/src/node.cc
@@ -630,7 +630,8 @@ namespace {
 bool ShouldAbortOnUncaughtException(Isolate* isolate) {
   HandleScope scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
-  return env->should_abort_on_uncaught_toggle()[0] &&
+  return env != nullptr &&
+         env->should_abort_on_uncaught_toggle()[0] &&
          !env->inside_should_not_abort_on_uncaught_scope();
 }
 
@@ -639,13 +640,15 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
 
 void AddPromiseHook(Isolate* isolate, promise_hook_func fn, void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);
   env->AddPromiseHook(fn, arg);
 }
 
 void AddEnvironmentCleanupHook(Isolate* isolate,
                                void (*fun)(void* arg),
                                void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);
   env->AddCleanupHook(fun, arg);
 }
 
@@ -654,6 +657,7 @@ void RemoveEnvironmentCleanupHook(Isolate* isolate,
                                   void (*fun)(void* arg),
                                   void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);
   env->RemoveCleanupHook(fun, arg);
 }
 
@@ -738,6 +742,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
   // Because of the AssignToContext() call in src/node_contextify.cc,
   // the two contexts need not be the same.
   Environment* env = Environment::GetCurrent(callback->CreationContext());
+  CHECK_NOT_NULL(env);
   Context::Scope context_scope(env->context());
   MaybeLocal<Value> ret = InternalMakeCallback(env, recv, callback,
                                                argc, argv, asyncContext);
@@ -1376,6 +1381,7 @@ void FatalException(Isolate* isolate,
   HandleScope scope(isolate);
 
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   Local<Object> process_object = env->process_object();
   Local<String> fatal_exception_string = env->fatal_exception_string();
   Local<Value> fatal_exception_function =
@@ -1601,7 +1607,7 @@ static void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
 }
 
 static void GetLinkedBinding(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args.GetIsolate());
+  Environment* env = Environment::GetCurrent(args);
 
   CHECK(args[0]->IsString());
 
@@ -2705,10 +2711,13 @@ void RunAtExit(Environment* env) {
 
 uv_loop_t* GetCurrentEventLoop(Isolate* isolate) {
   HandleScope handle_scope(isolate);
-  auto context = isolate->GetCurrentContext();
+  Local<Context> context = isolate->GetCurrentContext();
   if (context.IsEmpty())
     return nullptr;
-  return Environment::GetCurrent(context)->event_loop();
+  Environment* env = Environment::GetCurrent(context);
+  if (env == nullptr)
+    return nullptr;
+  return env->event_loop();
 }
 
 

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -946,7 +946,7 @@ class ThreadSafeFunction : public node::AsyncResource {
         return napi_ok;
       }
 
-      node::Environment::GetCurrent(env->isolate)->CloseHandle(
+      NodeEnv()->CloseHandle(
           reinterpret_cast<uv_handle_t*>(&async),
           [](uv_handle_t* handle) -> void {
             ThreadSafeFunction* ts_fn =
@@ -1036,9 +1036,12 @@ class ThreadSafeFunction : public node::AsyncResource {
   }
 
   node::Environment* NodeEnv() {
-    // For some reason grabbing the Node.js environment requires a handle scope.
+    // Grabbing the Node.js environment requires a handle scope because it
+    // looks up fields on the current context.
     v8::HandleScope scope(env->isolate);
-    return node::Environment::GetCurrent(env->isolate);
+    node::Environment* node_env = node::Environment::GetCurrent(env->isolate);
+    CHECK_NOT_NULL(node_env);
+    return node_env;
   }
 
   void MaybeStartIdle() {
@@ -1234,7 +1237,9 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
                                     v8::Local<v8::Context> context,
                                     napi_addon_register_func init) {
   if (init == nullptr) {
-    node::Environment::GetCurrent(context)->ThrowError(
+    node::Environment* node_env = node::Environment::GetCurrent(context);
+    CHECK_NOT_NULL(node_env);
+    node_env->ThrowError(
         "Module has no declared entry point.");
     return;
   }

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -274,7 +274,9 @@ MaybeLocal<Object> New(Isolate* isolate,
 MaybeLocal<Object> New(Isolate* isolate, size_t length) {
   EscapableHandleScope handle_scope(isolate);
   Local<Object> obj;
-  if (Buffer::New(Environment::GetCurrent(isolate), length).ToLocal(&obj))
+  Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
+  if (Buffer::New(env, length).ToLocal(&obj))
     return handle_scope.Escape(obj);
   return Local<Object>();
 }
@@ -316,6 +318,7 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
 MaybeLocal<Object> Copy(Isolate* isolate, const char* data, size_t length) {
   EscapableHandleScope handle_scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   Local<Object> obj;
   if (Buffer::Copy(env, data, length).ToLocal(&obj))
     return handle_scope.Escape(obj);
@@ -365,6 +368,7 @@ MaybeLocal<Object> New(Isolate* isolate,
                        void* hint) {
   EscapableHandleScope handle_scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   Local<Object> obj;
   if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
     return handle_scope.Escape(obj);
@@ -403,6 +407,7 @@ MaybeLocal<Object> New(Environment* env,
 MaybeLocal<Object> New(Isolate* isolate, char* data, size_t length) {
   EscapableHandleScope handle_scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Handle nullptr here.
   Local<Object> obj;
   if (Buffer::New(env, data, length).ToLocal(&obj))
     return handle_scope.Escape(obj);

diff --git a/src/node_context_data.h b/src/node_context_data.h
@@ -25,16 +25,11 @@ namespace node {
 #define NODE_CONTEXT_TAG 35
 #endif
 
-#ifndef NODE_CONTEXT_TAG_BOUNDARY
-#define NODE_CONTEXT_TAG_BOUNDARY 36
-#endif
-
 enum ContextEmbedderIndex {
   kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
   kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
   kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX,
   kContextTag = NODE_CONTEXT_TAG,
-  kContextTagBoundary = NODE_CONTEXT_TAG_BOUNDARY,
 };
 
 }  // namespace node

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -431,7 +431,7 @@ void FSReqWrap::SetReturnValue(const FunctionCallbackInfo<Value>& args) {
 
 void NewFSReqWrap(const FunctionCallbackInfo<Value>& args) {
   CHECK(args.IsConstructCall());
-  Environment* env = Environment::GetCurrent(args.GetIsolate());
+  Environment* env = Environment::GetCurrent(args);
   new FSReqWrap(env, args.This(), args[0]->IsTrue());
 }
 
@@ -772,7 +772,7 @@ inline FSReqBase* GetReqWrap(Environment* env, Local<Value> value,
 }
 
 void Access(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args.GetIsolate());
+  Environment* env = Environment::GetCurrent(args);
   HandleScope scope(env->isolate());
 
   const int argc = args.Length();
@@ -2089,9 +2089,8 @@ void Initialize(Local<Object> target,
 
   StatWatcher::Initialize(env, target);
 
-  // Create FunctionTemplate for FSReqWrap
-  Local<FunctionTemplate> fst =
-      FunctionTemplate::New(env->isolate(), NewFSReqWrap);
+  // Create FunctionTemplate for FSReqCallback
+  Local<FunctionTemplate> fst = env->NewFunctionTemplate(NewFSReqWrap);
   fst->InstanceTemplate()->SetInternalFieldCount(1);
   AsyncWrap::AddWrapMethods(env, fst);
   Local<String> wrapString =

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -501,7 +501,9 @@ class InternalCallbackScope {
 
 class ThreadPoolWork {
  public:
-  explicit inline ThreadPoolWork(Environment* env) : env_(env) {}
+  explicit inline ThreadPoolWork(Environment* env) : env_(env) {
+    CHECK_NOT_NULL(env);
+  }
   inline virtual ~ThreadPoolWork() = default;
 
   inline void ScheduleWork();

diff --git a/src/node_perf.cc b/src/node_perf.cc
@@ -310,6 +310,7 @@ void TimerFunctionCall(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);  // TODO(addaleax): Verify that this is correct.
   Local<Context> context = env->context();
   Local<Function> fn = args.Data().As<Function>();
   size_t count = args.Length();

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -70,6 +70,26 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
   EXPECT_TRUE(called_cb_2);
 }
 
+TEST_F(EnvironmentTest, NonNodeJSContext) {
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+  Env test_env {handle_scope, argv};
+
+  EXPECT_EQ(node::Environment::GetCurrent(v8::Local<v8::Context>()), nullptr);
+
+  node::Environment* env = *test_env;
+  EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);
+  EXPECT_EQ(node::Environment::GetCurrent(env->context()), env);
+
+  v8::Local<v8::Context> context = v8::Context::New(isolate_);
+  EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
+  EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);
+
+  v8::Context::Scope context_scope(context);
+  EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
+  EXPECT_EQ(node::Environment::GetCurrent(isolate_), nullptr);
+}
+
 static void at_exit_callback1(void* arg) {
   called_cb_1 = true;
   if (arg) {