GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — inspector: avoid calling into JS from V8 interrupts · nodejs/node@b42d664

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • src
    • env-inl.h
    • env.cc
    • env.h
    • inspector_agent.cc
    • inspector_agent.h
  • test/parallel
    • test-inspector-async-hook-after-done.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/env-inl.h‎

Copy file name to clipboardExpand all lines: src/env-inl.h
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,10 @@ inline void Environment::set_can_call_into_js(bool can_call_into_js) {
615615
can_call_into_js_ = can_call_into_js;
616616
}
617617

618+
inline bool Environment::is_processing_v8_interrupt() const {
619+
return is_processing_v8_interrupt_;
620+
}
621+
618622
inline bool Environment::has_run_bootstrapping_code() const {
619623
return principal_realm_->has_run_bootstrapping_code();
620624
}
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1515,7 +1515,9 @@ void Environment::RequestInterruptFromV8() {
15151515
return;
15161516
}
15171517
env->interrupt_data_.store(nullptr);
1518+
env->is_processing_v8_interrupt_ = true;
15181519
env->RunAndClearInterrupts();
1520+
env->is_processing_v8_interrupt_ = false;
15191521
}, interrupt_data);
15201522
}
15211523

Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -791,6 +791,12 @@ class Environment final : public MemoryRetainer {
791791
inline bool can_call_into_js() const;
792792
inline void set_can_call_into_js(bool can_call_into_js);
793793

794+
// True while RequestInterrupt() callbacks are being invoked from the
795+
// v8::Isolate::RequestInterrupt() handler, i.e. potentially at an
796+
// arbitrary point during JS execution. Calling into JS must be avoided
797+
// in that case.
798+
inline bool is_processing_v8_interrupt() const;
799+
794800
// Increase or decrease a counter that manages whether this Environment
795801
// keeps the event loop alive on its own or not. The counter starts out at 0,
796802
// meaning it does not, and any positive value will make it keep the event
@@ -1247,6 +1253,7 @@ class Environment final : public MemoryRetainer {
12471253
bool task_queues_async_initialized_ = false;
12481254

12491255
std::atomic<Environment**> interrupt_data_ {nullptr};
1256+
bool is_processing_v8_interrupt_ = false;
12501257
void RequestInterruptFromV8();
12511258
static void CheckImmediate(uv_check_t* handle);
12521259

Collapse file

‎src/inspector_agent.cc‎

Copy file name to clipboardExpand all lines: src/inspector_agent.cc
+60-52Lines changed: 60 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -547,11 +547,7 @@ class NodeInspectorClient : public V8InspectorClient {
547547
return;
548548
}
549549
if (auto agent = env_->inspector_agent()) {
550-
if (depth == 0) {
551-
agent->DisableAsyncHook();
552-
} else {
553-
agent->EnableAsyncHook();
554-
}
550+
agent->SetAsyncHookTrackingEnabled(depth != 0);
555551
}
556552
}
557553

@@ -647,6 +643,7 @@ class NodeInspectorClient : public V8InspectorClient {
647643

648644
void installAdditionalCommandLineAPI(Local<Context> context,
649645
Local<Object> target) override {
646+
if (!env_->can_call_into_js()) return;
650647
Local<Function> installer = env_->inspector_console_extension_installer();
651648
if (!installer.IsEmpty()) {
652649
Local<Value> argv[] = {target};
@@ -1068,58 +1065,69 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
10681065
Local<Function> disable_function) {
10691066
parent_env_->set_inspector_enable_async_hooks(enable_function);
10701067
parent_env_->set_inspector_disable_async_hooks(disable_function);
1071-
if (pending_enable_async_hook_) {
1072-
CHECK(!pending_disable_async_hook_);
1073-
pending_enable_async_hook_ = false;
1074-
EnableAsyncHook();
1075-
} else if (pending_disable_async_hook_) {
1076-
CHECK(!pending_enable_async_hook_);
1077-
pending_disable_async_hook_ = false;
1078-
DisableAsyncHook();
1079-
}
1068+
SyncAsyncHookState();
10801069
}
10811070

1082-
void Agent::EnableAsyncHook() {
1083-
HandleScope scope(parent_env_->isolate());
1084-
Local<Function> enable = parent_env_->inspector_enable_async_hooks();
1085-
if (!enable.IsEmpty()) {
1086-
ToggleAsyncHook(parent_env_->isolate(), enable);
1087-
} else if (pending_disable_async_hook_) {
1088-
CHECK(!pending_enable_async_hook_);
1089-
pending_disable_async_hook_ = false;
1090-
} else {
1091-
pending_enable_async_hook_ = true;
1092-
}
1071+
void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
1072+
async_hook_wanted_ = enabled;
1073+
SyncAsyncHookState();
10931074
}
10941075

1095-
void Agent::DisableAsyncHook() {
1096-
HandleScope scope(parent_env_->isolate());
1097-
Local<Function> disable = parent_env_->inspector_disable_async_hooks();
1098-
if (!disable.IsEmpty()) {
1099-
ToggleAsyncHook(parent_env_->isolate(), disable);
1100-
} else if (pending_enable_async_hook_) {
1101-
CHECK(!pending_disable_async_hook_);
1102-
pending_enable_async_hook_ = false;
1103-
} else {
1104-
pending_disable_async_hook_ = true;
1105-
}
1106-
}
1076+
// Reconcile the state of the async hook used for async stack traces with the
1077+
// state last requested by the protocol. The hook is set up in JS land,
1078+
// (see inspector_async_hooks.js), which isn't safe to do when:
1079+
// 1. We are in early bootstrap and the setup functions aren't registered in
1080+
// C++ yet.
1081+
// 2. We are in a V8 interrupt requested by inspector protocol message
1082+
// dispatch e.g. from maxAsyncCallStackDepthChanged() notifications.
1083+
// When it's not safe to call into JS, this is a no-op and we'll try again in
1084+
// RegisterAsyncHook() (for 1) or from a scheduled immediate (for 2).
1085+
void Agent::SyncAsyncHookState() {
1086+
// The debugger can request an interrupt within the toggle JS function itself,
1087+
// A nested call only records the new requested state, the outermost call sees
1088+
// it when re-checking the loop condition after each toggle.
1089+
if (syncing_async_hook_state_) return;
1090+
syncing_async_hook_state_ = true;
1091+
auto on_exit = OnScopeLeave([this]() { syncing_async_hook_state_ = false; });
1092+
1093+
Isolate* isolate = parent_env_->isolate();
1094+
HandleScope scope(isolate);
1095+
while (async_hook_wanted_ != async_hook_enabled_) {
1096+
// Guard against running this during cleanup -- no async events will be
1097+
// emitted anyway at that point anymore, and calling into JS is not
1098+
// possible. This should probably not be something we're attempting in the
1099+
// first place,
1100+
// Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
1101+
if (!parent_env_->can_call_into_js()) return;
1102+
1103+
bool enable = async_hook_wanted_;
1104+
Local<Function> fn = enable ? parent_env_->inspector_enable_async_hooks()
1105+
: parent_env_->inspector_disable_async_hooks();
1106+
if (fn.IsEmpty()) return;
1107+
1108+
if (parent_env_->is_processing_v8_interrupt()) {
1109+
parent_env_->SetImmediate(
1110+
[](Environment* env) {
1111+
Agent* agent = env->inspector_agent();
1112+
if (agent != nullptr) agent->SyncAsyncHookState();
1113+
},
1114+
CallbackFlags::kUnrefed);
1115+
return;
1116+
}
11071117

1108-
void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
1109-
// Guard against running this during cleanup -- no async events will be
1110-
// emitted anyway at that point anymore, and calling into JS is not possible.
1111-
// This should probably not be something we're attempting in the first place,
1112-
// Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
1113-
if (!parent_env_->can_call_into_js()) return;
1114-
CHECK(parent_env_->has_run_bootstrapping_code());
1115-
HandleScope handle_scope(isolate);
1116-
CHECK(!fn.IsEmpty());
1117-
auto context = parent_env_->context();
1118-
v8::TryCatch try_catch(isolate);
1119-
USE(fn->Call(context, Undefined(isolate), 0, nullptr));
1120-
if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
1121-
PrintCaughtException(isolate, context, try_catch);
1122-
UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
1118+
CHECK(parent_env_->has_run_bootstrapping_code());
1119+
Local<Context> context = parent_env_->context();
1120+
v8::TryCatch try_catch(isolate);
1121+
USE(fn->Call(context, Undefined(isolate), 0, nullptr));
1122+
if (try_catch.HasCaught()) {
1123+
// Termination may abort the toggle invocation, retrying now would just
1124+
// be terminated again. Instead of recording the toggle that may not have
1125+
// taken effect, leave the states as-is so that a later sync retries.
1126+
if (try_catch.HasTerminated()) return;
1127+
PrintCaughtException(isolate, context, try_catch);
1128+
UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
1129+
}
1130+
async_hook_enabled_ = enable;
11231131
}
11241132
}
11251133

Collapse file

‎src/inspector_agent.h‎

Copy file name to clipboardExpand all lines: src/inspector_agent.h
+8-5Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,7 @@ class Agent {
9090
void RegisterAsyncHook(v8::Isolate* isolate,
9191
v8::Local<v8::Function> enable_function,
9292
v8::Local<v8::Function> disable_function);
93-
void EnableAsyncHook();
94-
void DisableAsyncHook();
93+
void SetAsyncHookTrackingEnabled(bool enabled);
9594

9695
void SetParentHandle(std::unique_ptr<ParentInspectorHandle> parent_handle);
9796
std::unique_ptr<ParentInspectorHandle> GetParentHandle(uint64_t thread_id,
@@ -132,7 +131,7 @@ class Agent {
132131
std::shared_ptr<NetworkResourceManager> GetNetworkResourceManager();
133132

134133
private:
135-
void ToggleAsyncHook(v8::Isolate* isolate, v8::Local<v8::Function> fn);
134+
void SyncAsyncHookState();
136135
void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local<v8::Function> fn);
137136

138137
node::Environment* parent_env_;
@@ -150,8 +149,12 @@ class Agent {
150149
DebugOptions debug_options_;
151150
std::shared_ptr<ExclusiveAccess<HostPort>> host_port_;
152151

153-
bool pending_enable_async_hook_ = false;
154-
bool pending_disable_async_hook_ = false;
152+
// The state of the async hook used for async stack traces that the protocol
153+
// last requested, and the state JS currently has. SyncAsyncHookState()
154+
// reconciles the two when it is possible and safe to call into JS.
155+
bool async_hook_wanted_ = false;
156+
bool async_hook_enabled_ = false;
157+
bool syncing_async_hook_state_ = false;
155158

156159
bool network_tracking_enabled_ = false;
157160
bool pending_enable_network_tracking = false;
Collapse file

‎test/parallel/test-inspector-async-hook-after-done.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-inspector-async-hook-after-done.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ function onAttachToWorker({ params: { sessionId } }) {
3434
session.once('NodeWorker.receivedMessageFromWorker', onMessageReceived);
3535
return;
3636
}
37-
// Force a call to node::inspector::Agent::ToggleAsyncHook by changing the
38-
// async call stack depth
37+
// Force a call to node::inspector::Agent::SyncAsyncHookState by changing
38+
// the async call stack depth
3939
postToWorkerInspector('Debugger.setAsyncCallStackDepth', { maxDepth: 1 });
4040
// This is were the original crash happened
4141
session.post('NodeWorker.detach', { sessionId }, () => {

0 commit comments

Comments
0 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.