Skip to content

inspector: avoid more JS calls from V8 interrupts #66740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
christianaurichzm wants to merge 1 commit into
base: main
Choose a base branch
from
+145 −102
Open

inspector: avoid more JS calls from V8 interrupts #66740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 99 additions & 84 deletions src/inspector_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,52 @@ static Mutex start_io_thread_agents_mutex;
static std::vector<Agent*> start_io_thread_agents;
static bool debug_signal_handler_started = false;

template <typename Defer, typename OnFailure>
void SyncJavaScriptHookState(Environment* env,
JavaScriptHookState* state,
Local<Function> enable_function,
Local<Function> disable_function,
bool require_bootstrapping,
Defer defer,
OnFailure on_failure) {
// The debugger can request an interrupt from within a hook's JS toggle
// function. A nested sync only records the new requested state; the
// outermost sync sees it when re-checking the loop after each toggle.
if (state->syncing) return;
state->syncing = true;
auto on_exit = OnScopeLeave([state]() { state->syncing = false; });

Isolate* isolate = env->isolate();
while (state->wanted != state->enabled) {
// No hook events will be emitted during cleanup, and calling into JS is no
// longer possible.
// Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
if (!env->can_call_into_js()) return;

bool enable = state->wanted;
Local<Function> function = enable ? enable_function : disable_function;
if (function.IsEmpty()) return;

if (env->is_processing_v8_interrupt()) {
defer();
return;
}

if (require_bootstrapping) CHECK(env->has_run_bootstrapping_code());
Local<Context> context = env->context();
v8::TryCatch try_catch(isolate);
USE(function->Call(context, Undefined(isolate), 0, nullptr));
if (try_catch.HasCaught()) {
// Termination may abort the toggle invocation. Do not record a state that
// may not have taken effect, so a later sync can retry it.
if (try_catch.HasTerminated()) return;
PrintCaughtException(isolate, context, try_catch);
on_failure();
}
state->enabled = enable;
}
}

static void RequestIoThreadStartOnAgents() {
Mutex::ScopedLock lock(start_io_thread_agents_mutex);
for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart();
Expand Down Expand Up @@ -483,6 +529,16 @@ class SameThreadInspectorSession : public InspectorSession {

void NotifyClusterWorkersDebugEnabled(Environment* env) {
Isolate* isolate = env->isolate();

// Starting the inspector from an interrupt is safe, but emitting the cluster
// notification calls into JS.
if (env->is_processing_v8_interrupt()) {
env->SetImmediate(
[](Environment* env) { NotifyClusterWorkersDebugEnabled(env); },
CallbackFlags::kUnrefed);
return;
}

HandleScope handle_scope(isolate);

// Send message to enable debug in cluster workers
Expand Down Expand Up @@ -1000,54 +1056,40 @@ void Agent::SetupNetworkTracking(Local<Function> enable_function,
Local<Function> disable_function) {
parent_env_->set_inspector_enable_network_tracking(enable_function);
parent_env_->set_inspector_disable_network_tracking(disable_function);
if (pending_enable_network_tracking) {
pending_enable_network_tracking = false;
EnableNetworkTracking();
} else if (pending_disable_network_tracking) {
pending_disable_network_tracking = false;
DisableNetworkTracking();
}
SyncNetworkTrackingState();
}

void Agent::EnableNetworkTracking() {
if (network_tracking_enabled_) {
return;
}
HandleScope scope(parent_env_->isolate());
Local<Function> enable = parent_env_->inspector_enable_network_tracking();
if (enable.IsEmpty()) {
pending_enable_network_tracking = true;
} else {
ToggleNetworkTracking(parent_env_->isolate(), enable);
network_tracking_enabled_ = true;
}
network_tracking_state_.wanted = true;
SyncNetworkTrackingState();
}

void Agent::DisableNetworkTracking() {
if (!network_tracking_enabled_) {
return;
}
HandleScope scope(parent_env_->isolate());
Local<Function> disable = parent_env_->inspector_disable_network_tracking();
if (disable.IsEmpty()) {
pending_disable_network_tracking = true;
} else if (!client_->hasConnectedSessions()) {
ToggleNetworkTracking(parent_env_->isolate(), disable);
network_tracking_enabled_ = false;
}
if (client_->hasConnectedSessions()) return;
network_tracking_state_.wanted = false;
SyncNetworkTrackingState();
}

void Agent::ToggleNetworkTracking(Isolate* isolate, Local<Function> fn) {
if (!parent_env_->can_call_into_js()) return;
auto context = parent_env_->context();
void Agent::SyncNetworkTrackingState() {
Isolate* isolate = parent_env_->isolate();
HandleScope scope(isolate);
CHECK(!fn.IsEmpty());
v8::TryCatch try_catch(isolate);
USE(fn->Call(context, Undefined(isolate), 0, nullptr));
if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
PrintCaughtException(isolate, context, try_catch);
UNREACHABLE("Cannot toggle network tracking, please report this.");
}
SyncJavaScriptHookState(
parent_env_,
&network_tracking_state_,
parent_env_->inspector_enable_network_tracking(),
parent_env_->inspector_disable_network_tracking(),
false,
[this]() {
parent_env_->SetImmediate(
[](Environment* env) {
Agent* agent = env->inspector_agent();
if (agent != nullptr) agent->SyncNetworkTrackingState();
},
CallbackFlags::kUnrefed);
},
[]() {
UNREACHABLE("Cannot toggle network tracking, please report this.");
});
}

void Agent::WaitForDisconnect() {
Expand Down Expand Up @@ -1098,7 +1140,7 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
}

void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
async_hook_wanted_ = enabled;
async_hook_state_.wanted = enabled;
SyncAsyncHookState();
}

Expand All @@ -1112,52 +1154,25 @@ void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
// When it's not safe to call into JS, this is a no-op and we'll try again in
// RegisterAsyncHook() (for 1) or from a scheduled immediate (for 2).
void Agent::SyncAsyncHookState() {
// The debugger can request an interrupt within the toggle JS function itself,
// A nested call only records the new requested state, the outermost call sees
// it when re-checking the loop condition after each toggle.
if (syncing_async_hook_state_) return;
syncing_async_hook_state_ = true;
auto on_exit = OnScopeLeave([this]() { syncing_async_hook_state_ = false; });

Isolate* isolate = parent_env_->isolate();
HandleScope scope(isolate);
while (async_hook_wanted_ != async_hook_enabled_) {
// Guard against running this during cleanup -- no async events will be
// emitted anyway at that point anymore, and calling into JS is not
// possible. This should probably not be something we're attempting in the
// first place,
// Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
if (!parent_env_->can_call_into_js()) return;

bool enable = async_hook_wanted_;
Local<Function> fn = enable ? parent_env_->inspector_enable_async_hooks()
: parent_env_->inspector_disable_async_hooks();
if (fn.IsEmpty()) return;

if (parent_env_->is_processing_v8_interrupt()) {
parent_env_->SetImmediate(
[](Environment* env) {
Agent* agent = env->inspector_agent();
if (agent != nullptr) agent->SyncAsyncHookState();
},
CallbackFlags::kUnrefed);
return;
}

CHECK(parent_env_->has_run_bootstrapping_code());
Local<Context> context = parent_env_->context();
v8::TryCatch try_catch(isolate);
USE(fn->Call(context, Undefined(isolate), 0, nullptr));
if (try_catch.HasCaught()) {
// Termination may abort the toggle invocation, retrying now would just
// be terminated again. Instead of recording the toggle that may not have
// taken effect, leave the states as-is so that a later sync retries.
if (try_catch.HasTerminated()) return;
PrintCaughtException(isolate, context, try_catch);
UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
}
async_hook_enabled_ = enable;
}
SyncJavaScriptHookState(
parent_env_,
&async_hook_state_,
parent_env_->inspector_enable_async_hooks(),
parent_env_->inspector_disable_async_hooks(),
true,
[this]() {
parent_env_->SetImmediate(
[](Environment* env) {
Agent* agent = env->inspector_agent();
if (agent != nullptr) agent->SyncAsyncHookState();
},
CallbackFlags::kUnrefed);
},
[]() {
UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
});
}

void Agent::AsyncTaskScheduled(const StringView& task_name, void* task,
Expand Down
18 changes: 11 additions & 7 deletions src/inspector_agent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ class ParentInspectorHandle;
class NodeInspectorClient;
class WorkerManager;

struct JavaScriptHookState {
bool wanted = false;
bool enabled = false;
bool syncing = false;
};

class InspectorSession {
public:
virtual ~InspectorSession() = default;
Expand Down Expand Up @@ -134,7 +140,7 @@ class Agent {

private:
void SyncAsyncHookState();
void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local<v8::Function> fn);
void SyncNetworkTrackingState();

node::Environment* parent_env_;
// Encapsulates majority of the Inspector functionality
Expand All @@ -154,19 +160,17 @@ class Agent {
// The state of the async hook used for async stack traces that the protocol
// last requested, and the state JS currently has. SyncAsyncHookState()
// reconciles the two when it is possible and safe to call into JS.
bool async_hook_wanted_ = false;
bool async_hook_enabled_ = false;
bool syncing_async_hook_state_ = false;
JavaScriptHookState async_hook_state_;

// Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(),
// whichever runs first, and freed by its close callback.
uv_async_t* start_io_thread_async_ = nullptr;
void StopAcceptingIoThreadStarts();
static void StopAcceptingIoThreadStartsHook(void* agent);

bool network_tracking_enabled_ = false;
bool pending_enable_network_tracking = false;
bool pending_disable_network_tracking = false;
// Network tracking uses JS hooks. Reconcile the protocol requested and
// applied states after leaving a V8 interrupt.
JavaScriptHookState network_tracking_state_;
std::shared_ptr<NetworkResourceManager> network_resource_manager_;
};

Expand Down
46 changes: 35 additions & 11 deletions src/inspector_js_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,21 +62,38 @@ struct MainThreadConnection {
template <typename ConnectionType>
class JSBindingsConnection : public BaseObject {
public:
struct PendingMicrotask {
explicit PendingMicrotask(JSBindingsConnection* connection)
: env(connection->env()), connection(connection) {
env->AddCleanupHook(Delete, this);
}

static void Delete(void* data) {
delete static_cast<PendingMicrotask*>(data);
}

static void Run(void* data) {
std::unique_ptr<PendingMicrotask> pending(
static_cast<PendingMicrotask*>(data));
pending->env->RemoveCleanupHook(Delete, pending.get());
pending->connection->FlushPendingMessages();
}

Environment* env;
BaseObjectPtr<JSBindingsConnection> connection;
};

class JSBindingsSessionDelegate : public InspectorSessionDelegate {
public:
JSBindingsSessionDelegate(Environment* env,
JSBindingsConnection* connection)
: env_(env),
connection_(connection) {
}
explicit JSBindingsSessionDelegate(JSBindingsConnection* connection)
: connection_(connection) {}

void SendMessageToFrontend(const v8_inspector::StringView& message)
override {
connection_->SendMessageToFrontend(message);
}

private:
Environment* env_;
BaseObjectPtr<JSBindingsConnection> connection_;
};

Expand All @@ -86,10 +103,10 @@ class JSBindingsConnection : public BaseObject {
: BaseObject(env, wrap), callback_(env->isolate(), callback) {
Agent* inspector = env->inspector_agent();
session_ = ConnectionType::Connect(
inspector, std::make_unique<JSBindingsSessionDelegate>(env, this));
// Inspector responses may be produced from a GC weak callback, where
// invoking the JavaScript session callback is forbidden. Defer delivery
// until the GC has completed.
inspector, std::make_unique<JSBindingsSessionDelegate>(this));
// Inspector responses may be produced from a GC weak callback or a V8
// interrupt, where invoking the JavaScript session callback is forbidden.
// Defer delivery until it is safe to call into JavaScript.
env->isolate()->AddGCPrologueCallback(GCPrologueCallback, this);
env->isolate()->AddGCEpilogueCallback(GCEpilogueCallback, this);
}
Expand All @@ -100,7 +117,8 @@ class JSBindingsConnection : public BaseObject {
}

void SendMessageToFrontend(const v8_inspector::StringView& message) {
if (in_gc_ || delivering_ || !pending_messages_.empty()) {
if (in_gc_ || env()->is_processing_v8_interrupt() || delivering_ ||
!pending_messages_.empty()) {
pending_messages_.emplace_back(
message.is8Bit()
? std::u16string(message.characters8(),
Expand Down Expand Up @@ -128,6 +146,12 @@ class JSBindingsConnection : public BaseObject {
void ScheduleFlush() {
if (flush_scheduled_ || delivering_) return;
flush_scheduled_ = true;
if (!in_gc_ && env()->is_processing_v8_interrupt()) {
auto* pending = new PendingMicrotask(this);
env()->context()->GetMicrotaskQueue()->EnqueueMicrotask(
env()->isolate(), PendingMicrotask::Run, pending);
return;
}
BaseObjectPtr<JSBindingsConnection> strong_ref{this};
env()->SetImmediate(
[strong_ref](Environment*) { strong_ref->FlushPendingMessages(); },
Expand Down
Loading