Repository navigation
deps: re-land V8 API interrupt JS check - #66740
Open
christianaurichzm wants to merge 4 commits into
Open
christianaurichzm wants to merge 4 commits into
christianaurichzm wants to merge 4 commits into
Conversation
Original commit message:
[inspector] Allow JS in pause notifications from API interrupts
Debugger.pause and OOM breaks are scheduled with RequestInterrupt(), so
handleProgramBreak() can run inside InvokeApiInterruptCallbacks() with
JavaScript execution disallowed. b36cf9f6ccd2 allowed JavaScript around
runMessageLoopOnPause(), but Debugger.paused is sent right before that
scope and Debugger.resumed right after it. An embedder channel that
delivers notifications to JavaScript on the same isolate, like the
in-process inspector.Session in Node.js, crashes with "Invoke in
DisallowJavascriptExecutionScope" when such a break happens.
Extend the existing scope over both notifications. d8's InspectorFrontend
opens its own AllowJavascriptExecutionScope, so no existing test covered
this; add a unittest with a channel that calls into JavaScript.
R=olivf@chromium.org, szuend@chromium.org
Refs: nodejs#65914
Bug: 40067940, 540137721
Change-Id: I0b60fb40cf9e753f7444d69ee8c40b4dd5d843ec
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8459373
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Olivier Flückiger <olivf@chromium.org>
Reviewed-by: Olivier Flückiger <olivf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#110692}
V8 15.5 has no FunctionCallbackInfo::DataV2(), so the unittest reads
the session pointer through Data() instead.
Refs: v8/v8@35bd158
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Inspector messages for a main thread connection are dispatched from a V8 interrupt handler. Defer frontend messages in this case using a microtask. This preserves the existing timing contract for main thread inspector connections, where notifications are observable after a microtask checkpoint. Starting the inspector I/O thread and toggling network tracking can also call into JS from an interrupt. Signed-off-by: deepak1556 <hop2deep@gmail.com> Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Signed-off-by: deepak1556 <hop2deep@gmail.com> Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Debugger.pause breaks from an API interrupt, and V8 sends Debugger.paused and Debugger.resumed to every session from there. An in-process inspector.Session receives them by calling into JavaScript, which aborts with "Invoke in DisallowJavascriptExecutionScope" unless V8 allows JavaScript around both notifications. Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Contributor
Author
|
@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! |
legendecas
added
v8 engine
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
4 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.
This re-lands #65397 together with the V8 fix for the inspector crash it exposed.
#65907 reverted it because
debugger/debug/futex-reentrant-waitfailed onnode-test-commit-v8-linux. That test depends on https://chromium-review.googlesource.com/c/v8/v8/+/8086043, which V8 14.6 didn't have.deps/v8is on 15.5 now, which has it.The check also made
test-inspector-multisession-wsabort withInvoke in DisallowJavascriptExecutionScope(nodejs/reliability#1654). When V8 pauses from an API interrupt, it sentDebugger.pausedandDebugger.resumedoutside theAllowJavascriptExecutionScopeit opens for the pause loop, and an in-processinspector.Sessionhandles both in JavaScript. v8/v8@35bd1582fe0a fixes that, and the first commit backports it. V8 15.5 has noFunctionCallbackInfo::DataV2(), so the unittest usesData()instead.@deepak1556's two commits are unchanged apart from a conflict with #65877 in
src/inspector_agent.{cc,h}. As in #65397, messages for an in-processinspector.Sessionthat arrive while Node handles another client's message in an interrupt are delivered at the next microtask checkpoint instead of synchronously.test-inspector-pause-with-js-sessionaborts 5/5 without the V8 fix and passes 100/100 with it.test-inspector-multisession-wspasses 100/100 as well, but it also passed without the fix here, so it only hits the crash by timing.make lint,parallelandsequentialpass on Linux x64. This should also get anode-test-commit-v8-linuxrun.Refs: #65397
Refs: #65907
Refs: #65914
Refs: nodejs/reliability#1654