Skip to content

inspector: disconnect handles before releasing interfaces - #67281

Open
fatihguzeldev wants to merge 1 commit into
nodejs:mainfrom
fatihguzeldev:inspector-worker-teardown
Open

fatihguzeldev wants to merge 1 commit into
nodejs:mainfrom
fatihguzeldev:inspector-worker-teardown

Conversation

@fatihguzeldev

Copy link
Copy Markdown
Contributor

the handle currently gets reset in the interface destructor. by then, the last strong reference is already gone, so a concurrent post can reach shared_from_this() and fail with std::bad_weak_ptr.

this disconnects the handle while the inspector client still owns the interface. also adds a regression test that keeps the interface alive after client teardown and checks that the handle has expired and rejects further posts.

fixes: #67079

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/inspector

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run. labels Nov 5, 2026
Disconnect the thread handle while NodeInspectorClient still owns the
interface. This prevents cross-thread posts from calling
shared_from_this() after the interface's last strong reference has been
released.

Add a regression test verifying handle expiration during owner teardown.

Fixes: nodejs#67079
Signed-off-by: Fatih Güzel <fatihguzeldev@gmail.com>
@fatihguzeldev
fatihguzeldev force-pushed the inspector-worker-teardown branch from c2339b4 to 4331dc0 Compare November 5, 2026 19:02
@fatihguzeldev

Copy link
Copy Markdown
Contributor Author

i haven't been able to reproduce the original crash. the test deliberately keeps the interface alive during owner teardown so its destructor cannot close the handle and hide the missing disconnect. this lets us check that the owner closes the handle without depending on hitting the race.

@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.67%. Comparing base (20daf9a) to head (4331dc0).
⚠️ Report is 55 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67281      +/-   ##
==========================================
+ Coverage   90.66%   90.67%   +0.01%     
==========================================
  Files         797      797              
  Lines      279382   279513     +131     
  Branches    53746    53772      +26     
==========================================
+ Hits       253306   253453     +147     
+ Misses      16357    16354       -3     
+ Partials     9719     9706      -13     
Files with missing lines Coverage Δ
src/inspector/main_thread_interface.cc 94.62% <100.00%> (+1.18%) ⬆️
src/inspector/main_thread_interface.h 95.00% <ø> (ø)
src/inspector_agent.cc 83.20% <100.00%> (-0.09%) ⬇️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inspector: abort (no message) when the main thread posts to an exiting Worker's connectToMainThread() session

2 participants