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.

The first commit 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.

The second commit is @deepak1556's patch from the discussion below. DOMStorage.getDOMStorageItems now reads the stores without calling into JavaScript: from an interrupt it no longer goes through the globalThis.localStorage and globalThis.sessionStorage getters, and a store that cannot be opened is reported without creating a JavaScript error. The third commit adds test-inspector-dom-storage-interrupt, which reads both stores from an interrupt and fails if any JavaScript runs there. It counts calls to the storage getters and to an Error.prototype.code setter, so it does not depend on the V8 check. 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.

Each commit stands on its own, so this can land with commit-queue-rebase instead of being squashed.

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 66.81614% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.51%. Comparing base (77609be) to head (4866149).
⚠️ Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
src/inspector_agent.cc	52.94%	24 Missing and 8 partials ⚠️
src/node_webstorage.cc	72.00%	14 Missing and 7 partials ⚠️
src/inspector/dom_storage_agent.cc	66.66%	10 Missing and 6 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.51%   -2.34%     
==========================================
  Files         428      797     +369     
  Lines      194693   278377   +83684     
  Branches    30091    53489   +23398     
==========================================
+ Hits       180774   251970   +71196     
- Misses      13593    16794    +3201     
- Partials      326     9613    +9287     

Files with missing lines	Coverage Δ
src/inspector_agent.h	100.00% <100.00%> (ø)
src/node_webstorage.h	87.50% <100.00%> (ø)
src/path.cc	78.64% <100.00%> (ø)
src/path.h	100.00% <ø> (ø)
src/inspector_js_api.cc	84.26% <78.26%> (ø)
src/inspector/dom_storage_agent.cc	81.88% <66.66%> (ø)
src/node_webstorage.cc	77.82% <72.00%> (ø)
src/inspector_agent.cc	84.27% <52.94%> (ø)

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

[deepak1556]

Thats a better check, thanks for catching it.

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

SGTM

[christianaurichzm]

Thanks @deepak1556, I added it as the second commit with the args[2] change, and the test is in the third one. I also made the untouched-store path skip ToNamespacedPath() for :memory:, like Storage::New does, since on Windows it would resolve :memory: against the cwd and open it as a file.

One thing: your patch didn't have a Signed-off-by, and the guidelines ask for one from each author, including the commit author. Could you reply with yours so I can amend it in?