inspector: avoid calling into JS from V8 interrupts - #65028
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65028 +/- ##
==========================================
- Coverage 92.04% 90.29% -1.75%
==========================================
Files 399 759 +360
Lines 175810 247628 +71818
Branches 27119 46679 +19560
==========================================
+ Hits 161816 223604 +61788
- Misses 13682 15464 +1782
- Partials 312 8560 +8248
🚀 New features to boost your workflow:
|
Our inspector implementation dispatches inspector messages from a V8 interrupt handler, so they could be handled during an arbitrary point of JS execution where re-calling into another irrelevant JS code is not safe. This patch tracks V8 interrupt state in this case and rewrite the async hook toggling as state reconciliation, so requests only record the desired state, which is applied once calling into JS is possible and safe, and the actual invocation is deferred to an immediate when inside an interrupt. This simplifies the previous mechanism and makes re-entracy and early termination safer. Drive-by: skip installing command line API extensions during teardown when calling into JS is no longer safe. Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
bf00114 to
a4b4587
Compare
|
Fyi, with https://chromium-review.googlesource.com/c/v8/v8/+/8173727 you can now try building with |
| return; | ||
| } | ||
| env->interrupt_data_.store(nullptr); | ||
| env->is_processing_v8_interrupt_ = true; |
There was a problem hiding this comment.
Would it be more reliable if V8 allows querying an isolate interrupt state? This flag at the moment only covers node's own interrupt requests.
There was a problem hiding this comment.
I think for fixing inspector, only tracking what the interrupts requested from the Environment is sufficient - we only have this clash because Node.js dispatches inspector messages in an interrupt (that is trackable in the Environment). Technically that's not strictly necessary, at least Chromium only does that conditionally. But without rewriting the entire inspector message machinery, avoiding JS invocation in the interrupt is the simplest safeguard we can put in. Other interrupts are harmless, as they don't lead to the inspector message dispatch. For stricter checks, we could go v8_disallow_js_in_api_interrupts_is_checked and maybe an isolate-wide state check, but since we don't have any other sites that invoke JS in an interrupt currently (not that I could find), I think we can leave that to a followup.
4 participants
Our inspector implementation dispatches inspector messages from a V8 interrupt handler, so they could be handled during an arbitrary point of JS execution where re-calling into another irrelevant JS code (which we currently do for creating the async hooks used for the async stack trace tracking) is not safe.
This patch tracks V8 interrupt state in this case and rewrite the async hook toggling as state reconciliation, so requests only record the desired state, which is applied once calling into JS is possible and safe, and the actual invocation is deferred to an immediate when inside an interrupt. This simplifies the previous mechanism and makes re-entrancy and early termination safer.
Drive-by: skip installing command line API extensions during teardown when calling into JS is no longer safe.
Refs: https://issues.chromium.org/u/1/issues/42212250
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
Refs: #26935
Previously in https://issues.chromium.org/u/1/issues/42212250 V8 was open to support arbitrary JS execution in an interrupt, but that was a long time ago. In a recent CL V8 is trying to disallow arbitrary JS execution in an interrupt again, which prompted this PR to make it safer.