From 40aedc158c72810718a79a4d3e925533a9933df8 Mon Sep 17 00:00:00 2001 From: Shumaf Lovpache Date: Thu, 28 May 2026 11:49:39 +0200 Subject: [PATCH 1/2] deps: V8: cherry-pick 383a6d80e857 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [inspector] Fix instrumentation breakpoint on wrapped scripts SetInstrumentationBreakpoint iterates the SFIs in a Script and picks the toplevel SFI to attach the instrumentation breakpoint to. For scripts compiled via ScriptCompiler::CompileFunction (e.g. Node.js CJS modules), the resulting Script contains two SFIs: a synthetic toplevel that just returns the wrapped function, and the wrapped SFI that the embedder actually invokes. Picking the toplevel for such a script means the breakpoint never fires when execution enters the user's code. Select the wrapped SFI for wrapped Scripts so that the breakpoint is attached where execution will actually pause. Bug: 516452552 Change-Id: I424c709655a6d5809d527f19f90dfb89e8acb7a8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7883039 Commit-Queue: Simon Zünd Reviewed-by: Simon Zünd Reviewed-by: Eric Leese Cr-Commit-Position: refs/heads/main@{#107643} Refs: https://github.com/v8/v8/commit/383a6d80e8571bed168c53865a064165b96433db --- common.gypi | 2 +- deps/v8/AUTHORS | 1 + deps/v8/src/debug/debug-interface.cc | 8 +++- ...et-instrumentation-breakpoint-expected.txt | 31 ++++++++++++++ .../set-instrumentation-breakpoint.js | 42 +++++++++++++++++++ deps/v8/test/inspector/inspector-test.cc | 17 ++++++++ deps/v8/test/inspector/tasks.cc | 19 +++++++++ deps/v8/test/inspector/tasks.h | 21 ++++++++++ 8 files changed, 139 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index e6a17d3505a5..563259e21ea7 100644 --- a/common.gypi +++ b/common.gypi @@ -43,7 +43,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.32', + 'v8_embedder_string': '-node.33', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/AUTHORS b/deps/v8/AUTHORS index ef0aeedaca51..823fd85d63f7 100644 --- a/deps/v8/AUTHORS +++ b/deps/v8/AUTHORS @@ -286,6 +286,7 @@ Sergey Markelov Shawn Anastasio Shawn Presser Sho Miyamoto +Shumaf Lovpache Stefan Penner Stefan Stojanovic Stephan Hartmann diff --git a/deps/v8/src/debug/debug-interface.cc b/deps/v8/src/debug/debug-interface.cc index ee13330c84e0..9e9b1060e0b0 100644 --- a/deps/v8/src/debug/debug-interface.cc +++ b/deps/v8/src/debug/debug-interface.cc @@ -774,7 +774,13 @@ bool Script::SetInstrumentationBreakpoint(BreakpointId* id) const { i::SharedFunctionInfo::ScriptIterator it(isolate, *script); for (i::Tagged sfi = it.Next(); !sfi.is_null(); sfi = it.Next()) { - if (sfi->is_toplevel()) { + // Node.js compiles CJS modules via ScriptCompiler::CompileFunction so that + // module-local bindings like __filename can be injected as function + // parameters without leaking into the global scope. The resulting Script + // carries two SFIs: a synthetic toplevel that just returns the wrapped + // function, and the wrapped SFI that the embedder actually invokes. For + // such scripts we should pick the wrapped SFI. + if (script->is_wrapped() ? sfi->is_wrapped() : sfi->is_toplevel()) { return isolate->debug()->SetBreakpointForFunction( handle(sfi, isolate), isolate->factory()->empty_string(), id, internal::Debug::kInstrumentation); diff --git a/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint-expected.txt b/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint-expected.txt index c0fb6f18a488..c1506a1d35ff 100644 --- a/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint-expected.txt +++ b/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint-expected.txt @@ -61,6 +61,37 @@ remove breakpoint.. evaluate script without sourceMappingURL.. evaluate script with sourceMappingURL.. +Running test: testWrappedScriptInstrumentation +set breakpoint and run wrapped script.. +paused with reason: instrumentation +{ + scriptId : + url : cjs-module.js +} +remove breakpoint.. +{ + id : + result : { + } +} + +Running test: testWrappedScriptWithSourceMap +set breakpoint for scriptWithSourceMapParsed.. +run wrapped script without sourceMappingURL.. +run wrapped script with sourceMappingURL.. +paused with reason: instrumentation +{ + scriptId : + sourceMapURL : cjs.js.map + url : cjs-sourcemapped.js +} +remove breakpoint.. +{ + id : + result : { + } +} + Running test: testBlackboxing set breakpoint and evaluate blackboxed script.. evaluate not blackboxed script.. diff --git a/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint.js b/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint.js index c5939bdab0a3..17a04f6fbccb 100644 --- a/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint.js +++ b/deps/v8/test/inspector/debugger/set-instrumentation-breakpoint.js @@ -79,6 +79,48 @@ InspectorTest.runAsyncTestSuite([ await Protocol.Debugger.disable(); }, + async function testWrappedScriptInstrumentation() { + await Protocol.Debugger.enable(); + InspectorTest.log('set breakpoint and run wrapped script..'); + const { result : firstResult } = await Protocol.Debugger.setInstrumentationBreakpoint({ + instrumentation: 'beforeScriptExecution' + }); + utils.compileAndRunWrapped(contextGroup.id, '1 + 2', 'cjs-module.js'); + { + const { params: { reason, data } } = await Protocol.Debugger.oncePaused(); + InspectorTest.log(`paused with reason: ${reason}`); + InspectorTest.logMessage(data); + } + await Protocol.Debugger.resume(); + InspectorTest.log('remove breakpoint..'); + InspectorTest.logMessage(await Protocol.Debugger.removeBreakpoint({ + breakpointId: firstResult.breakpointId + })); + await Protocol.Debugger.disable(); + }, + + async function testWrappedScriptWithSourceMap() { + await Protocol.Debugger.enable(); + InspectorTest.log('set breakpoint for scriptWithSourceMapParsed..'); + const { result : firstResult } = await Protocol.Debugger.setInstrumentationBreakpoint({ + instrumentation: 'beforeScriptWithSourceMapExecution' + }); + InspectorTest.log('run wrapped script without sourceMappingURL..'); + utils.compileAndRunWrapped(contextGroup.id, '1 + 2', 'cjs-plain.js'); + InspectorTest.log('run wrapped script with sourceMappingURL..'); + utils.compileAndRunWrapped(contextGroup.id, '1 + 2\n//# sourceMappingURL=cjs.js.map', 'cjs-sourcemapped.js'); + { + const { params: { reason, data } } = await Protocol.Debugger.oncePaused(); + InspectorTest.log(`paused with reason: ${reason}`); + InspectorTest.logMessage(data); + } + InspectorTest.log('remove breakpoint..'); + InspectorTest.logMessage(await Protocol.Debugger.removeBreakpoint({ + breakpointId: firstResult.breakpointId + })); + await Protocol.Debugger.disable(); + }, + async function testBlackboxing() { await Protocol.Debugger.enable(); await Protocol.Debugger.setBlackboxPatterns({patterns: ['foo\.js']}); diff --git a/deps/v8/test/inspector/inspector-test.cc b/deps/v8/test/inspector/inspector-test.cc index 6b67b785a145..05babe326a5c 100644 --- a/deps/v8/test/inspector/inspector-test.cc +++ b/deps/v8/test/inspector/inspector-test.cc @@ -72,6 +72,9 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask { utils->Set(isolate, "compileAndRunWithOrigin", v8::FunctionTemplate::New( isolate, &UtilsExtension::CompileAndRunWithOrigin)); + utils->Set(isolate, "compileAndRunWrapped", + v8::FunctionTemplate::New( + isolate, &UtilsExtension::CompileAndRunWrapped)); utils->Set(isolate, "setCurrentTimeMSForTest", v8::FunctionTemplate::New( isolate, &UtilsExtension::SetCurrentTimeMSForTest)); @@ -233,6 +236,20 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask { info[4].As(), info[5].As())); } + static void CompileAndRunWrapped( + const v8::FunctionCallbackInfo& info) { + if (info.Length() != 3 || !info[0]->IsInt32() || !info[1]->IsString() || + !info[2]->IsString()) { + FATAL( + "Internal error: compileAndRunWrapped(context_group_id, source, " + "url)."); + } + backend_runner_->Append(std::make_unique( + info.GetIsolate(), info[0].As()->Value(), + ToVector(info.GetIsolate(), info[1].As()), + info[2].As())); + } + static void SetCurrentTimeMSForTest( const v8::FunctionCallbackInfo& info) { if (info.Length() != 1 || !info[0]->IsNumber()) { diff --git a/deps/v8/test/inspector/tasks.cc b/deps/v8/test/inspector/tasks.cc index ce8be5562a67..f098b62db061 100644 --- a/deps/v8/test/inspector/tasks.cc +++ b/deps/v8/test/inspector/tasks.cc @@ -130,5 +130,24 @@ void ExecuteStringTask::Run(InspectorIsolateData* data) { } } +void ExecuteWrappedStringTask::Run(InspectorIsolateData* data) { + v8::HandleScope handle_scope(data->isolate()); + v8::Local context = data->GetDefaultContext(context_group_id_); + v8::MicrotasksScope microtasks_scope(context, + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope(context); + v8::ScriptOrigin origin(ToV8String(data->isolate(), name_)); + v8::Local source = ToV8String(data->isolate(), expression_); + v8::ScriptCompiler::Source scriptSource(source, origin); + v8::Local function; + if (!v8::ScriptCompiler::CompileFunction(context, &scriptSource) + .ToLocal(&function)) { + return; + } + v8::MaybeLocal result = + function->Call(context, context->Global(), 0, nullptr); + USE(result); +} + } // namespace internal } // namespace v8 diff --git a/deps/v8/test/inspector/tasks.h b/deps/v8/test/inspector/tasks.h index 3b3e68e2e05f..600f7ccc5a3b 100644 --- a/deps/v8/test/inspector/tasks.h +++ b/deps/v8/test/inspector/tasks.h @@ -101,6 +101,27 @@ class ExecuteStringTask : public TaskRunner::Task { int context_group_id_; }; +class ExecuteWrappedStringTask : public TaskRunner::Task { + public: + ExecuteWrappedStringTask(v8::Isolate* isolate, int context_group_id, + const std::vector& expression, + v8::Local name) + : expression_(expression), + name_(ToVector(isolate, name)), + context_group_id_(context_group_id) {} + + ~ExecuteWrappedStringTask() override = default; + ExecuteWrappedStringTask(const ExecuteWrappedStringTask&) = delete; + ExecuteWrappedStringTask& operator=(const ExecuteWrappedStringTask&) = delete; + bool is_priority_task() override { return false; } + void Run(InspectorIsolateData* data) override; + + private: + std::vector expression_; + std::vector name_; + int context_group_id_; +}; + class SetTimeoutTask : public TaskRunner::Task { public: SetTimeoutTask(int context_group_id, v8::Isolate* isolate, From aa24eb72d6bfc09a43a0bc3b9e70ae4c904d0d46 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Fri, 11 Sep 2026 15:43:38 -0400 Subject: [PATCH 2/2] test: remove known issue test Signed-off-by: Aviv Keller --- ...st-inspector-instrumentation-breakpoint.js | 54 ----------------- ...st-inspector-instrumentation-breakpoint.js | 59 +++++++++++++++++++ 2 files changed, 59 insertions(+), 54 deletions(-) delete mode 100644 test/known_issues/test-inspector-instrumentation-breakpoint.js create mode 100644 test/parallel/test-inspector-instrumentation-breakpoint.js diff --git a/test/known_issues/test-inspector-instrumentation-breakpoint.js b/test/known_issues/test-inspector-instrumentation-breakpoint.js deleted file mode 100644 index 0105097f2baf..000000000000 --- a/test/known_issues/test-inspector-instrumentation-breakpoint.js +++ /dev/null @@ -1,54 +0,0 @@ -// This test validates inspector's Debugger.setInstrumentationBreakpoint method. -// Refs: https://github.com/nodejs/node/issues/31138 - -'use strict'; -const common = require('../common'); - -common.skipIfInspectorDisabled(); - -const assert = require('assert'); -const { resolve: UrlResolve } = require('url'); -const fixtures = require('../common/fixtures'); -const { NodeInstance } = require('../common/inspector-helper.js'); - -async function testBreakpointBeforeScriptExecution(session) { - console.log('[test]', - 'Verifying debugger stops on start of each script ' + - '(Debugger.setInstrumentationBreakpoint with beforeScriptExecution)'); - const commands = [ - { 'method': 'Runtime.enable' }, - { 'method': 'Debugger.enable' }, - { 'method': 'Debugger.setInstrumentationBreakpoint', - 'params': { 'instrumentation': 'beforeScriptExecution' } }, - { 'method': 'Runtime.runIfWaitingForDebugger' }, - ]; - - await session.send(commands); - - // Break on start - await session.waitForBreakOnLine( - 0, UrlResolve(session.scriptURL().toString(), 'main.js')); - await session.send([{ 'method': 'Debugger.resume' }]); - - // Script loaded - await session.waitForBreakOnLine( - 0, UrlResolve(session.scriptURL().toString(), 'main.js')); - await session.send([{ 'method': 'Debugger.resume' }]); - - // Script loaded - await session.waitForBreakOnLine( - 0, UrlResolve(session.scriptURL().toString(), 'dep.js')); - await session.send([{ 'method': 'Debugger.resume' }]); -} - -async function runTest() { - const main = fixtures.path('inspector-instrumentation-breakpoint', 'main.js'); - const child = new NodeInstance(['--inspect-brk=0'], '', main); - - const session = await child.connectInspectorSession(); - await testBreakpointBeforeScriptExecution(session); - await session.runToCompletion(); - assert.strictEqual((await child.expectShutdown()).exitCode, 0); -} - -runTest(); diff --git a/test/parallel/test-inspector-instrumentation-breakpoint.js b/test/parallel/test-inspector-instrumentation-breakpoint.js new file mode 100644 index 000000000000..3a905a58ec0b --- /dev/null +++ b/test/parallel/test-inspector-instrumentation-breakpoint.js @@ -0,0 +1,59 @@ +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('node:assert/strict'); + +const fixtures = require('../common/fixtures'); +const { NodeInstance } = require('../common/inspector-helper'); + +async function testBreakpointBeforeScriptExecution(session) { + console.log( + '[test]', + 'Verifying debugger stops on start of each script ' + + '(Debugger.setInstrumentationBreakpoint with beforeScriptExecution)', + ); + + const commands = [ + { method: 'Runtime.enable' }, + { method: 'Debugger.enable' }, + { + method: 'Debugger.setInstrumentationBreakpoint', + params: { instrumentation: 'beforeScriptExecution' }, + }, + { method: 'Runtime.runIfWaitingForDebugger' }, + ]; + + await session.send(commands); + + const mainURL = new URL('main.js', session.scriptURL()).href; + + // Break on start. + await session.waitForBreakOnLine(3, 'node:internal/main/run_main_module'); + await session.send([{ method: 'Debugger.resume' }]); + + // Script loaded. + await session.waitForBreakOnLine(0, mainURL); + await session.send([{ method: 'Debugger.resume' }]); + + // Dependency loaded. + await session.waitForBreakOnLine(0, mainURL); +} + +async function runTest() { + const main = fixtures.path( + 'inspector-instrumentation-breakpoint', + 'main.js', + ); + + const child = new NodeInstance(['--inspect-brk=0'], '', main); + const session = await child.connectInspectorSession(); + + await testBreakpointBeforeScriptExecution(session); + await session.runToCompletion(); + + assert.strictEqual((await child.expectShutdown()).exitCode, 0); +} + +runTest();