From ac5ee86d23e8715b6488b5832a4294f84775aabe Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 19 Dec 2022 06:31:20 +0000 Subject: [PATCH] src: distinguish env stopping flags `Environment::FreeEnvironment` creates a `DisallowJavascriptExecutionScope`, so the flag `Environment::can_call_into_js()` should also be set as `false`. As `Environment::can_call_into_js_` is a simple boolean flag compared to the atomic `Environment::is_stopping_`, call sites that are determined to be on the same thread of the JavaScript execution should prefer `Environment::can_call_into_js_`. --- src/api/environment.cc | 6 ++++-- src/env-inl.h | 2 +- src/env.cc | 9 ++++++--- src/env.h | 3 +++ src/module_wrap.cc | 3 +-- src/node_contextify.cc | 3 +-- src/node_http2.cc | 2 +- src/stream_base.cc | 2 +- 8 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 0cbbe3fb048517..b1e5c9cfe76320 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -52,8 +52,7 @@ bool AllowWasmCodeGenerationCallback(Local context, bool ShouldAbortOnUncaughtException(Isolate* isolate) { DebugSealHandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); - return env != nullptr && - (env->is_main_thread() || !env->is_stopping()) && + return env != nullptr && (env->is_main_thread() || env->can_call_into_js()) && env->abort_on_uncaught_exception() && env->should_abort_on_uncaught_toggle()[0] && !env->inside_should_not_abort_on_uncaught_scope(); @@ -404,6 +403,9 @@ void FreeEnvironment(Environment* env) { Context::Scope context_scope(env->context()); SealHandleScope seal_handle_scope(isolate); + // Set the flag in accordance with the DisallowJavascriptExecutionScope + // above. + env->set_can_call_into_js(false); env->set_stopping(true); env->stop_sub_worker_contexts(); env->RunCleanup(); diff --git a/src/env-inl.h b/src/env-inl.h index a512d175a29cfe..26e5e62c8fd5fa 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -613,7 +613,7 @@ void Environment::RequestInterrupt(Fn&& cb) { } inline bool Environment::can_call_into_js() const { - return can_call_into_js_ && !is_stopping(); + return can_call_into_js_; } inline void Environment::set_can_call_into_js(bool can_call_into_js) { diff --git a/src/env.cc b/src/env.cc index 5172d71ad6a673..3236ec7bd80f22 100644 --- a/src/env.cc +++ b/src/env.cc @@ -809,7 +809,7 @@ Environment::~Environment() { } // FreeEnvironment() should have set this. - CHECK(is_stopping()); + CHECK(!can_call_into_js()); if (heapsnapshot_near_heap_limit_callback_added_) { RemoveHeapSnapshotNearHeapLimitCallback(0); @@ -905,10 +905,13 @@ void Environment::InitializeLibuv() { } void Environment::ExitEnv() { - set_can_call_into_js(false); + // Should not access non-thread-safe methods here. set_stopping(true); isolate_->TerminateExecution(); - SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); }); + SetImmediateThreadsafe([](Environment* env) { + env->set_can_call_into_js(false); + uv_stop(env->event_loop()); + }); } void Environment::RegisterHandleCleanups() { diff --git a/src/env.h b/src/env.h index 396416b42ae348..bdf20edb46b4bc 100644 --- a/src/env.h +++ b/src/env.h @@ -781,6 +781,9 @@ class Environment : public MemoryRetainer { void stop_sub_worker_contexts(); template inline void ForEachWorker(Fn&& iterator); + // Determine if the environment is stopping. This getter is thread-safe. + // If the check is not performed off thread, the flag + // `env->can_call_into_js()` should be preferred. inline bool is_stopping() const; inline void set_stopping(bool value); inline std::list* extra_linked_bindings(); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index d6c766244fd416..7065b176f6d95f 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -410,8 +410,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo& args) { // Convert the termination exception into a regular exception. if (timed_out || received_signal) { - if (!env->is_main_thread() && env->is_stopping()) - return; + if (!env->is_main_thread() && !env->can_call_into_js()) return; env->isolate()->CancelTerminateExecution(); // It is possible that execution was terminated by another timeout in // which this timeout is nested, so check whether one of the watchdogs diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 53b947fd1bcf2d..d9e1095be3c972 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1028,8 +1028,7 @@ bool ContextifyScript::EvalMachine(Local context, // Convert the termination exception into a regular exception. if (timed_out || received_signal) { - if (!env->is_main_thread() && env->is_stopping()) - return false; + if (!env->is_main_thread() && !env->can_call_into_js()) return false; env->isolate()->CancelTerminateExecution(); // It is possible that execution was terminated by another timeout in // which this timeout is nested, so check whether one of the watchdogs diff --git a/src/node_http2.cc b/src/node_http2.cc index 032625f1e4f199..bb4057c9925676 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1127,7 +1127,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, // Don't close synchronously in case there's pending data to be written. This // may happen when writing trailing headers. if (code == NGHTTP2_NO_ERROR && nghttp2_session_want_write(handle) && - !env->is_stopping()) { + env->can_call_into_js()) { env->SetImmediate([handle, id, code, user_data](Environment* env) { OnStreamClose(handle, id, code, user_data); }); diff --git a/src/stream_base.cc b/src/stream_base.cc index 389aab28c5adff..4b78ffd17df3ad 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -608,7 +608,7 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished( StreamReq* req_wrap, int status) { StreamBase* stream = static_cast(stream_); Environment* env = stream->stream_env(); - if (env->is_stopping()) return; + if (!env->can_call_into_js()) return; AsyncWrap* async_wrap = req_wrap->GetAsyncWrap(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context());