diff --git a/lib/internal/inspector/network.js b/lib/internal/inspector/network.js index e9ebd05c6627..d2043b606968 100644 --- a/lib/internal/inspector/network.js +++ b/lib/internal/inspector/network.js @@ -10,6 +10,7 @@ const { const dc = require('diagnostics_channel'); const { now } = require('internal/perf/utils'); const { MIMEType } = require('internal/mime'); +const { isNetworkDomainEnabled } = internalBinding('inspector'); const kInspectorRequestId = Symbol('kInspectorRequestId'); @@ -87,6 +88,7 @@ module.exports = { kResourceType, getMonotonicTime, getNextRequestId, + isNetworkDomainEnabled, registerDiagnosticChannels, sniffMimeType, }; diff --git a/lib/internal/inspector/network_http.js b/lib/internal/inspector/network_http.js index 1bb08762bdb4..d1c98d7cdf25 100644 --- a/lib/internal/inspector/network_http.js +++ b/lib/internal/inspector/network_http.js @@ -14,6 +14,7 @@ const { kResourceType, getMonotonicTime, getNextRequestId, + isNetworkDomainEnabled, registerDiagnosticChannels, sniffMimeType, } = require('internal/inspector/network'); @@ -74,6 +75,7 @@ const convertHeaderObject = (headers = kEmptyObject) => { * @param {{ request: import('http').ClientRequest }} event */ function onClientRequestCreated({ request }) { + if (!isNetworkDomainEnabled()) return; request[kInspectorRequestId] = getNextRequestId(); const { 0: headers, 1: host, 2: charset } = convertHeaderObject(request.getHeaders()); diff --git a/lib/internal/inspector/network_http2.js b/lib/internal/inspector/network_http2.js index 0f4aabf59cfd..22f2efe19268 100644 --- a/lib/internal/inspector/network_http2.js +++ b/lib/internal/inspector/network_http2.js @@ -13,6 +13,7 @@ const { kResourceType, getMonotonicTime, getNextRequestId, + isNetworkDomainEnabled, registerDiagnosticChannels, sniffMimeType, } = require('internal/inspector/network'); @@ -88,6 +89,7 @@ function convertHeaderObject(headers = kEmptyObject) { * @param {{ stream: import('http2').ClientHttp2Stream, headers: object }} event */ function onClientStreamCreated({ stream, headers }) { + if (!isNetworkDomainEnabled()) return; stream[kInspectorRequestId] = getNextRequestId(); const { 0: convertedHeaderObject, 1: url, 2: method, 4: charset } = convertHeaderObject(headers); diff --git a/lib/internal/inspector/network_undici.js b/lib/internal/inspector/network_undici.js index a80af196587d..35bbfcc660de 100644 --- a/lib/internal/inspector/network_undici.js +++ b/lib/internal/inspector/network_undici.js @@ -10,6 +10,7 @@ const { kResourceType, getMonotonicTime, getNextRequestId, + isNetworkDomainEnabled, registerDiagnosticChannels, sniffMimeType, } = require('internal/inspector/network'); @@ -70,6 +71,7 @@ function responseHeadersArrayToDictionary(headers) { * @param {{ request: undici.Request }} event */ function onClientRequestStart({ request }) { + if (!isNetworkDomainEnabled()) return; const url = `${request.origin}${request.path}`; request[kInspectorRequestId] = getNextRequestId(); @@ -210,6 +212,7 @@ function onClientResponseFinish({ request }) { // undici:websocket:open fires when the connection is established, but this results // in an inaccurate stack trace. function onWebSocketOpen({ websocket, handshakeResponse }) { + if (!isNetworkDomainEnabled()) return; websocket[kInspectorRequestId] = getNextRequestId(); const url = websocket.url.toString(); Network.webSocketCreated({ diff --git a/lib/internal/inspector_network_tracking.js b/lib/internal/inspector_network_tracking.js index 4044edbddb53..84eabba0a2a1 100644 --- a/lib/internal/inspector_network_tracking.js +++ b/lib/internal/inspector_network_tracking.js @@ -1,18 +1,29 @@ 'use strict'; -function enable() { - require('internal/inspector/network_http').enable(); - require('internal/inspector/network_http2').enable(); - require('internal/inspector/network_undici').enable(); -} +// Network tracking is loaded and enabled at startup, when +// --experimental-network-inspection is set, rather than lazily from the +// inspector agent when a `Network.enable` message arrives. That message is +// dispatched via an interrupt that can land in the middle of arbitrary +// JavaScript execution -- including while another module is still being +// loaded by require(). Calling into JS at that point (the tracking modules +// pull in `inspector`, `worker_threads` and `stream`) can observe a +// half-initialized module and throw, which the inspector agent turns into a +// fatal, unrecoverable error that aborts the process. +// +// With the channels subscribed eagerly, `Network.enable`/`Network.disable` +// only toggle a flag in C++ (NetworkInspector::enabled_) that gates event +// emission, and never call into JS from the interrupt. +// Refs: https://github.com/nodejs/node/issues/64308 +const networkHttp = require('internal/inspector/network_http'); +const networkHttp2 = require('internal/inspector/network_http2'); +const networkUndici = require('internal/inspector/network_undici'); -function disable() { - require('internal/inspector/network_http').disable(); - require('internal/inspector/network_http2').disable(); - require('internal/inspector/network_undici').disable(); +function enable() { + networkHttp.enable(); + networkHttp2.enable(); + networkUndici.enable(); } module.exports = { enable, - disable, }; diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 0f5aaf857aeb..cc3a3cc2676e 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -614,11 +614,11 @@ function setupInspectorHooks() { function setupNetworkInspection() { if (internalBinding('config').hasInspector && getOptionValue('--experimental-network-inspection')) { - const { - enable, - disable, - } = require('internal/inspector_network_tracking'); - internalBinding('inspector').setupNetworkTracking(enable, disable); + // Subscribe the network tracking diagnostics channels right away, so that + // a `Network.enable` inspector message never has to call into JS from an + // interrupt. Event emission is gated natively until `Network.enable` is + // received. + require('internal/inspector_network_tracking').enable(); } } diff --git a/src/env_properties.h b/src/env_properties.h index 087193faba98..727c832b8f1d 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -509,9 +509,7 @@ V(immediate_callback_function, v8::Function) \ V(inspector_console_extension_installer, v8::Function) \ V(inspector_disable_async_hooks, v8::Function) \ - V(inspector_disable_network_tracking, v8::Function) \ V(inspector_enable_async_hooks, v8::Function) \ - V(inspector_enable_network_tracking, v8::Function) \ V(maybe_cache_generated_source_map, v8::Function) \ V(messaging_deserialize_create_object, v8::Function) \ V(message_port, v8::Object) \ diff --git a/src/inspector/network_inspector.cc b/src/inspector/network_inspector.cc index 3a79ba988571..1536de3659b2 100644 --- a/src/inspector/network_inspector.cc +++ b/src/inspector/network_inspector.cc @@ -36,17 +36,18 @@ void NetworkInspector::emitNotification(v8::Local context, } } +// The network tracking diagnostics channels are subscribed at startup when +// --experimental-network-inspection is set (see +// lib/internal/process/pre_execution.js), so enabling and disabling only +// toggles the flag that gates event emission. This must not call into JS: +// Network.enable/Network.disable are dispatched via an interrupt that can +// land in the middle of arbitrary JavaScript execution. +// Refs: https://github.com/nodejs/node/issues/64308 void NetworkInspector::Enable() { - if (auto agent = env_->inspector_agent()) { - agent->EnableNetworkTracking(); - } enabled_ = true; } void NetworkInspector::Disable() { - if (auto agent = env_->inspector_agent()) { - agent->DisableNetworkTracking(); - } enabled_ = false; } diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 22506e26b32c..161db2c1551e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -374,6 +374,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, return retaining_context_; } + bool isNetworkDomainEnabled() { + return network_inspector_ && network_inspector_->IsEnabled(); + } + private: // v8_inspector::V8Inspector::Channel void sendResponse( @@ -780,6 +784,13 @@ class NodeInspectorClient : public V8InspectorClient { return !channels_.empty(); } + bool isNetworkDomainEnabled() { + for (const auto& id_channel : channels_) { + if (id_channel.second->isNetworkDomainEnabled()) return true; + } + return false; + } + private: bool shouldRunMessageLoop() { if (waiting_for_frontend_) @@ -996,60 +1007,6 @@ void Agent::EmitProtocolEvent(v8::Local context, client_->emitNotification(context, event, params); } -void Agent::SetupNetworkTracking(Local enable_function, - Local 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(); - } -} - -void Agent::EnableNetworkTracking() { - if (network_tracking_enabled_) { - return; - } - HandleScope scope(parent_env_->isolate()); - Local 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; - } -} - -void Agent::DisableNetworkTracking() { - if (!network_tracking_enabled_) { - return; - } - HandleScope scope(parent_env_->isolate()); - Local 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; - } -} - -void Agent::ToggleNetworkTracking(Isolate* isolate, Local fn) { - if (!parent_env_->can_call_into_js()) return; - auto context = parent_env_->context(); - 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."); - } -} - void Agent::WaitForDisconnect() { THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_, permission::PermissionScope::kInspector, @@ -1219,6 +1176,10 @@ bool Agent::IsActive() { return io_ != nullptr || client_->IsActive(); } +bool Agent::IsNetworkDomainEnabled() { + return client_ != nullptr && client_->isNetworkDomainEnabled(); +} + void Agent::SetParentHandle( std::unique_ptr parent_handle) { parent_handle_ = std::move(parent_handle); diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 5a1d1de26547..c5535500df49 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -74,11 +74,8 @@ class Agent { void EmitProtocolEvent(v8::Local context, const v8_inspector::StringView& event, v8::Local params); - - void SetupNetworkTracking(v8::Local enable_function, - v8::Local disable_function); - void EnableNetworkTracking(); - void DisableNetworkTracking(); + // Returns true if any session has enabled the Network domain. + bool IsNetworkDomainEnabled(); // Async stack traces instrumentation. void AsyncTaskScheduled(const v8_inspector::StringView& taskName, void* task, @@ -134,7 +131,6 @@ class Agent { private: void SyncAsyncHookState(); - void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local fn); node::Environment* parent_env_; // Encapsulates majority of the Inspector functionality @@ -164,9 +160,6 @@ class Agent { void StopAcceptingIoThreadStarts(); static void StopAcceptingIoThreadStartsHook(void* agent); - bool network_tracking_enabled_ = false; - bool pending_enable_network_tracking = false; - bool pending_disable_network_tracking = false; std::shared_ptr network_resource_manager_; }; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 336930f18679..4b6d8f3aed61 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -359,21 +359,14 @@ void EmitProtocolEvent(const FunctionCallbackInfo& args) { params); } -void SetupNetworkTracking(const FunctionCallbackInfo& args) { +void IsEnabled(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - - CHECK(args[0]->IsFunction()); - Local enable_function = args[0].As(); - CHECK(args[1]->IsFunction()); - Local disable_function = args[1].As(); - - env->inspector_agent()->SetupNetworkTracking(enable_function, - disable_function); + args.GetReturnValue().Set(env->inspector_agent()->IsListening()); } -void IsEnabled(const FunctionCallbackInfo& args) { +void IsNetworkDomainEnabled(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - args.GetReturnValue().Set(env->inspector_agent()->IsListening()); + args.GetReturnValue().Set(env->inspector_agent()->IsNetworkDomainEnabled()); } void Open(const FunctionCallbackInfo& args) { @@ -473,8 +466,9 @@ void Initialize(Local target, Local unused, SetMethod(context, target, "registerAsyncHook", RegisterAsyncHookWrapper); SetMethodNoSideEffect(context, target, "isEnabled", IsEnabled); + SetMethodNoSideEffect( + context, target, "isNetworkDomainEnabled", IsNetworkDomainEnabled); SetMethod(context, target, "emitProtocolEvent", EmitProtocolEvent); - SetMethod(context, target, "setupNetworkTracking", SetupNetworkTracking); SetMethod(context, target, "putNetworkResource", PutNetworkResource); Local console_string = FIXED_ONE_BYTE_STRING(isolate, "console"); @@ -509,8 +503,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(RegisterAsyncHookWrapper); registry->Register(IsEnabled); + registry->Register(IsNetworkDomainEnabled); registry->Register(EmitProtocolEvent); - registry->Register(SetupNetworkTracking); registry->Register(JSBindingsConnection::New); registry->Register(JSBindingsConnection::Dispatch); diff --git a/test/parallel/test-inspector-network-pre-enable-request.js b/test/parallel/test-inspector-network-pre-enable-request.js new file mode 100644 index 000000000000..958f6ea2d805 --- /dev/null +++ b/test/parallel/test-inspector-network-pre-enable-request.js @@ -0,0 +1,96 @@ +// Flags: --inspect=0 --experimental-network-inspection +'use strict'; +const common = require('../common'); + +common.skipIfInspectorDisabled(); + +// Network tracking is subscribed at startup. Requests that start before +// Network.enable must not produce events after Network.enable, since the +// frontend never saw their Network.requestWillBeSent. + +const assert = require('node:assert'); +const { once } = require('node:events'); +const http = require('node:http'); +const inspector = require('node:inspector/promises'); + +const session = new inspector.Session(); +session.connect(); + +const events = []; +for (const method of [ + 'requestWillBeSent', + 'responseReceived', + 'loadingFinished', + 'loadingFailed', +]) { + session.on(`Network.${method}`, ({ params }) => { + events.push({ method, requestId: params.requestId, url: params.request?.url }); + }); +} + +const held = new Map(); +const server = http.createServer((req, res) => { + if (req.url === '/after') { + res.end('after'); + } else { + held.set(req.url, res); + } +}); + +async function waitForHeld(url) { + while (!held.has(url)) { + await new Promise((resolve) => setImmediate(resolve)); + } + return held.get(url); +} + +async function main() { + server.listen(0, '127.0.0.1'); + await once(server, 'listening'); + const base = `http://127.0.0.1:${server.address().port}`; + + const httpResponse = new Promise((resolve) => { + http.get(`${base}/http`, (res) => { + res.resume(); + res.on('end', resolve); + }); + }); + const httpError = new Promise((resolve) => { + http.get(`${base}/http-error`).on('error', resolve); + }); + const fetchResponse = fetch(`${base}/fetch`).then((res) => res.text()); + + const resHttp = await waitForHeld('/http'); + const resHttpError = await waitForHeld('/http-error'); + const resFetch = await waitForHeld('/fetch'); + + await session.post('Network.enable'); + + resHttp.end('http'); + resHttpError.socket.destroy(); + resFetch.end('fetch'); + await Promise.all([httpResponse, httpError, fetchResponse]); + + assert.deepStrictEqual(events, []); + + await new Promise((resolve) => { + http.get(`${base}/after`, (res) => { + res.resume(); + res.on('end', resolve); + }); + }); + await new Promise((resolve) => setImmediate(resolve)); + + assert.deepStrictEqual(events.map((e) => e.method), [ + 'requestWillBeSent', + 'responseReceived', + 'loadingFinished', + ]); + assert.strictEqual(events[0].url, `${base}/after`); + assert(events.every((e) => e.requestId === events[0].requestId)); + + session.disconnect(); + server.close(); +} + +main().then(common.mustCall()); diff --git a/test/parallel/test-inspector-network-tracking-eager-load.js b/test/parallel/test-inspector-network-tracking-eager-load.js new file mode 100644 index 000000000000..59c07a5e1792 --- /dev/null +++ b/test/parallel/test-inspector-network-tracking-eager-load.js @@ -0,0 +1,45 @@ +// Flags: --inspect=0 --experimental-network-inspection +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/64308. +// +// A `Network.enable` inspector message is dispatched to the main thread via an +// interrupt that can land in the middle of arbitrary JavaScript execution, +// including while another module is still being loaded by require(). If +// enabling network tracking calls into JS at that point (the tracking modules +// pull in `inspector`, `worker_threads` and `stream`), it can observe a +// half-initialized module and throw, which the inspector agent treats as an +// unrecoverable fatal error and aborts the whole process. +// +// With --experimental-network-inspection, the tracking modules must therefore +// be loaded and their diagnostics channels subscribed at startup, so that +// `Network.enable`/`Network.disable` never call into JS from the interrupt. + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); + +// The tracking modules must already be loaded: they are required and enabled +// during bootstrap when --experimental-network-inspection is set. +for (const mod of [ + 'NativeModule internal/inspector_network_tracking', + 'NativeModule internal/inspector/network_http', + 'NativeModule internal/inspector/network_http2', + 'NativeModule internal/inspector/network_undici', +]) { + assert(process.moduleLoadList.includes(mod), + `${mod} should be loaded eagerly at startup, but was not`); +} + +// The diagnostics channels must already be subscribed at startup, so toggling +// Network.enable does not need to run any JS. +const dc = require('diagnostics_channel'); +for (const channel of [ + 'http.client.request.created', + 'http2.client.stream.created', + 'undici:request:create', +]) { + assert(dc.channel(channel).hasSubscribers, + `${channel} should have a subscriber at startup, but does not`); +} diff --git a/typings/internalBinding/inspector.d.ts b/typings/internalBinding/inspector.d.ts index c0ee57774d87..46161bc03931 100644 --- a/typings/internalBinding/inspector.d.ts +++ b/typings/internalBinding/inspector.d.ts @@ -28,8 +28,8 @@ export interface InspectorBinding { asyncTaskFinished(taskId: number): void; registerAsyncHook(enable: () => void, disable: () => void): void; isEnabled(): boolean; + isNetworkDomainEnabled(): boolean; emitProtocolEvent(eventName: string, params: object): void; - setupNetworkTracking(enable: () => void, disable: () => void): void; console: Console; Connection: InspectorConnectionConstructor; MainThreadConnection: InspectorConnectionConstructor;