inspector: load network tracking modules eagerly#64379
Open
harjothkhara wants to merge 1 commit into
Open
Conversation
`Network.enable` is dispatched to the main thread through a V8 interrupt that can run in the middle of arbitrary JavaScript execution, including while another module is still being loaded by require(). The network tracking enable()/disable() helpers lazily require()d their modules, which pull in `inspector`, `worker_threads` and `stream`. When the interrupt landed mid-require(), require() returned a half-initialized module and threw (for example "Class extends value undefined" from internal/worker/io, or "require(...).enable is not a function"). The inspector agent treats any exception thrown while toggling network tracking as unrecoverable and aborts the whole process. Load the three network tracking modules eagerly at setup time so that enable()/disable() never call require() from an interrupt. They now run during setupNetworkInspection() at bootstrap, before the inspector can dispatch Network.enable. Fixes: nodejs#64308
Collaborator
|
Review requested:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--experimental-network-inspectioncan abort the whole Node.js process when a debugger attaches. Reported in #64308.Network.enableis delivered to the main thread via a V8 interrupt that can run in the middle of arbitrary JavaScript execution — including while another module is still being loaded byrequire(). The network-trackingenable()/disable()helpers lazilyrequire()d their modules (network_http,network_http2,network_undici), which transitively pull ininspector→worker_threads→internal/worker/io→stream. When the interrupt lands mid-require(),require()returns a half-initialized module and throws — for exampleClass extends value undefinedfrominternal/worker/io, orrequire(...).enable is not a function.Agent::ToggleNetworkTracking(src/inspector_agent.cc) treats any exception thrown while toggling network tracking as unrecoverable and callsUNREACHABLE(), aborting the process.Fix
Load the three network-tracking modules eagerly at module scope, so
enable()/disable()never callrequire()from an interrupt. The requires now run duringsetupNetworkInspection()at bootstrap, before the inspector can dispatchNetwork.enable. This only affects the opt-in--experimental-network-inspectionflag, where an inspector is already active.Verification
Minimal repro from the issue:
node --inspect-wait --experimental-network-inspection -e "require('net')"then attach a debugger and send
Network.enable.Driving the real inspector path locally (attach over WebSocket,
Runtime.runIfWaitingForDebugger, thenNetwork.enablewith jitter to spray the interrupt across the require window):mainbuild: aborts (Cannot toggle network tracking→UNREACHABLE), reproduced at iteration 1Adds
test/parallel/test-inspector-network-tracking-eager-load.js, which asserts the network-tracking modules are loaded at setup and thatenable()/disable()do not lazily load any module.Note
A deeper hardening — not calling
UNREACHABLE()inToggleNetworkTrackingwhen the toggle callback throws — would defend against this class of failure at the C++ layer as well. I kept this PR to the minimal JS change that fixes the reported crash, but I'm happy to add that as a follow-up if maintainers prefer.Fixes: #64308