inspector: avoid more JS calls from V8 interrupts

[christianaurichzm]

This lands the Node.js side of #65397, which was reverted in #65907, without enabling the V8 check.

This is @deepak1556's inspector: defer JS execution from V8 interrupts from #65397, unchanged apart from a conflict with #65877. As before, messages for an in-process inspector.Session that arrive while Node.js handles another client's message in an interrupt are delivered at the next microtask checkpoint instead of synchronously.

DOMStorage still has paths that can call into JavaScript from an interrupt; the remaining changes are being discussed below. Enabling the check needs v8/v8@35bd1582fe0a, which is in V8 15.8, so that will be a separate PR once deps/v8 includes it.

Refs: #65397
Refs: #65907
Refs: #66710

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/inspector
• @nodejs/security-wg
• @nodejs/v8-update

[christianaurichzm]

@legendecas @joyeecheung since you reviewed #65397, could you take a look at the re-land when you get a chance? It also needs a node-test-commit-v8-linux run, since that is what caught the futex failure last time. Thanks!

[codecov]

Codecov Report

❌ Patch coverage is 61.05263% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (77609be) to head (3090ade).
⚠️ Report is 42 commits behind head on main.

Files with missing lines	Patch %	Lines
src/inspector_agent.cc	52.94%	24 Missing and 8 partials ⚠️
src/inspector_js_api.cc	78.26%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66740      +/-   ##
==========================================
- Coverage   92.85%   90.50%   -2.35%     
==========================================
  Files         428      797     +369     
  Lines      194693   278246   +83553     
  Branches    30091    53451   +23360     
==========================================
+ Hits       180774   251837   +71063     
- Misses      13593    16790    +3197     
- Partials      326     9619    +9293     

Files with missing lines	Coverage Δ
src/inspector_agent.h	100.00% <100.00%> (ø)
src/inspector_js_api.cc	84.26% <78.26%> (ø)
src/inspector_agent.cc	84.27% <52.94%> (ø)

... and 506 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.

[deepak1556]

Oh another case found,

1. DOMStorageAgent::getWebStorage() (inspector: auto collect webstorage data #62145) looks up globalThis.localStorage / globalThis.sessionStorage via lazy JS accessors.
2. When Storage::GetAll() opens a malformed database file (src: throw on a malformed localStorage file #65879), Storage::Open() creates a JS Error only for DOMStorageAgent::getDOMStorageItems() to catch it with v8::TryCatch and extract the message string.

Rather than deferring DOMStorage.getDOMStorageItems (which would prevent inspecting storage while paused in NodeInspectorClient::runMessageLoopOnPause), could try something like
0001-inspector-avoid-JS-execution-in-DOMStorage.getDOMSto.patch that keeps everything in C++

[joyeecheung]

Note that we are drafting a new policy for updating V8 in Node.js - for V8 commit backports that are justified to be fast tracked and do not have to wait for the regular V8 upgrade, it's better to target release staging branches, instead of main: #66710

[christianaurichzm]

Thanks @joyeecheung. I dropped the backport and the commit that enables the check. I'll enable it in a separate PR once a V8 update brings in v8/v8@35bd1582fe0a.

@deepak1556, you're right about the second case: with a code setter on Error.prototype, reading a malformed file from an interrupt aborts, since ERR_INVALID_STATE sets code with Object::Set.

I tried your patch with the check enabled and both cases work. One thing I hit: Storage::New tells the stores apart by location, but --localstorage-file=:memory: is also valid, so if sessionStorage is created first, getDOMStorageItems for sessionStorage returns the localStorage items, even outside an interrupt. Using the third constructor argument that InspectorLocalStorage and InspectorSessionStorage already pass fixes it:

BindingData* binding_data = realm->GetBindingData<BindingData>();
if (binding_data != nullptr && args[2]->IsBoolean()) {
  auto& slot = args[2]->IsTrue() ? binding_data->local_storage
                                 : binding_data->session_storage;
  slot = BaseObjectWeakPtr<Storage>(storage);
}

If you're OK with that change, I'll add your patch here with you as the author, plus regression tests.