From 283fb5f06fbfb9ae3ef0350c8c03fbdd6ca8d70e Mon Sep 17 00:00:00 2001 From: Jaroslav Sevcik Date: Tue, 8 Nov 2022 07:11:16 +0100 Subject: [PATCH] [inspector] Trigger requested pause after instrumentation pause If a CDP client requests Debugger.pause during instrumentation pause, the requests is currently ignored. With this patch, the debugger will take note of a pause request during instrumentation pause and enter the pause once the instrumentation pause resumes. Bug: chromium:1381967 Change-Id: I4d0337a92fa31d0666ab02b54f95aba4d89592b3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4008379 Commit-Queue: Jaroslav Sevcik Reviewed-by: Kim-Anh Tran Cr-Commit-Position: refs/heads/main@{#84138} --- src/debug/debug-interface.h | 10 +++- src/debug/debug.cc | 24 +++++--- src/debug/debug.h | 2 +- src/inspector/v8-debugger-agent-impl.cc | 11 +++- src/inspector/v8-debugger.cc | 25 +++++++-- src/inspector/v8-debugger.h | 8 ++- .../pause-on-instrumentation-expected.txt | 10 ++++ .../debugger/pause-on-instrumentation.js | 56 +++++++++++++++++++ .../set-breakpoint-on-instrumentation.js | 2 +- 9 files changed, 129 insertions(+), 19 deletions(-) create mode 100644 test/inspector/debugger/pause-on-instrumentation-expected.txt create mode 100644 test/inspector/debugger/pause-on-instrumentation.js diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h index e0725d33cd..3d9f7c5c9f 100644 --- a/src/debug/debug-interface.h +++ b/src/debug/debug-interface.h @@ -287,9 +287,15 @@ class DebugDelegate { v8::Local paused_context, const std::vector& inspector_break_points_hit, base::EnumSet break_reasons = {}) {} - virtual void BreakOnInstrumentation( + enum PauseAfterInstrumentation { + kPauseAfterInstrumentationRequested, + kNoPauseAfterInstrumentationRequested + }; + virtual PauseAfterInstrumentation BreakOnInstrumentation( v8::Local paused_context, - const debug::BreakpointId instrumentationId) {} + const debug::BreakpointId instrumentationId) { + return kNoPauseAfterInstrumentationRequested; + } virtual void ExceptionThrown(v8::Local paused_context, v8::Local exception, v8::Local promise, bool is_uncaught, diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 08db807f7c..9c6c5f4ca5 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -480,16 +480,19 @@ void Debug::Unload() { debug_delegate_ = nullptr; } -void Debug::OnInstrumentationBreak() { +debug::DebugDelegate::PauseAfterInstrumentation +Debug::OnInstrumentationBreak() { RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger); - if (!debug_delegate_) return; + if (!debug_delegate_) { + return debug::DebugDelegate::kNoPauseAfterInstrumentationRequested; + } DCHECK(in_debug_scope()); HandleScope scope(isolate_); DisableBreak no_recursive_break(this); Handle native_context(isolate_->native_context()); - debug_delegate_->BreakOnInstrumentation(v8::Utils::ToLocal(native_context), - kInstrumentationId); + return debug_delegate_->BreakOnInstrumentation( + v8::Utils::ToLocal(native_context), kInstrumentationId); } void Debug::Break(JavaScriptFrame* frame, Handle break_target) { @@ -512,21 +515,28 @@ void Debug::Break(JavaScriptFrame* frame, Handle break_target) { BreakLocation location = BreakLocation::FromFrame(debug_info, frame); const bool hitInstrumentationBreak = IsBreakOnInstrumentation(debug_info, location); + bool shouldPauseAfterInstrumentation = false; if (hitInstrumentationBreak) { - OnInstrumentationBreak(); + debug::DebugDelegate::PauseAfterInstrumentation pauseDuringInstrumentation = + OnInstrumentationBreak(); + shouldPauseAfterInstrumentation = + pauseDuringInstrumentation == + debug::DebugDelegate::kPauseAfterInstrumentationRequested; } // Find actual break points, if any, and trigger debug break event. bool has_break_points; + bool scheduled_break = + scheduled_break_on_function_call() || shouldPauseAfterInstrumentation; MaybeHandle break_points_hit = CheckBreakPoints(debug_info, &location, &has_break_points); if (!break_points_hit.is_null() || break_on_next_function_call() || - scheduled_break_on_function_call()) { + scheduled_break) { StepAction lastStepAction = last_step_action(); DCHECK_IMPLIES(scheduled_break_on_function_call(), lastStepAction == StepNone); debug::BreakReasons break_reasons; - if (scheduled_break_on_function_call()) { + if (scheduled_break) { break_reasons.Add(debug::BreakReason::kScheduled); } // Clear all current stepping setup. diff --git a/src/debug/debug.h b/src/debug/debug.h index 3dd97d6b1d..a47b693b5e 100644 --- a/src/debug/debug.h +++ b/src/debug/debug.h @@ -222,7 +222,7 @@ class V8_EXPORT_PRIVATE Debug { // Debug event triggers. void OnDebugBreak(Handle break_points_hit, StepAction stepAction, debug::BreakReasons break_reasons = {}); - void OnInstrumentationBreak(); + debug::DebugDelegate::PauseAfterInstrumentation OnInstrumentationBreak(); base::Optional OnThrow(Handle exception) V8_WARN_UNUSED_RESULT; diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc index 8fa1be4575..e3c1444fbc 100644 --- a/src/inspector/v8-debugger-agent-impl.cc +++ b/src/inspector/v8-debugger-agent-impl.cc @@ -1328,9 +1328,16 @@ void V8DebuggerAgentImpl::cancelPauseOnNextStatement() { Response V8DebuggerAgentImpl::pause() { if (!enabled()) return Response::ServerError(kDebuggerNotEnabled); - if (isPaused()) return Response::Success(); - if (m_debugger->canBreakProgram()) { + if (m_debugger->isInInstrumentationPause()) { + // If we are inside an instrumentation pause, remember the pause request + // so that we can enter the requested pause once we are done + // with the instrumentation. + m_debugger->requestPauseAfterInstrumentation(); + } else if (isPaused()) { + // Ignore the pause request if we are already paused. + return Response::Success(); + } else if (m_debugger->canBreakProgram()) { m_debugger->interruptAndBreak(m_session->contextGroupId()); } else { pushBreakDetails(protocol::Debugger::Paused::ReasonEnum::Other, nullptr); diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc index cef1b9ffd0..aed85fa7cd 100644 --- a/src/inspector/v8-debugger.cc +++ b/src/inspector/v8-debugger.cc @@ -202,6 +202,10 @@ bool V8Debugger::canBreakProgram() { return v8::debug::CanBreakProgram(m_isolate); } +bool V8Debugger::isInInstrumentationPause() const { + return m_instrumentationPause; +} + void V8Debugger::breakProgram(int targetContextGroupId) { DCHECK(canBreakProgram()); // Don't allow nested breaks. @@ -225,6 +229,10 @@ void V8Debugger::interruptAndBreak(int targetContextGroupId) { nullptr); } +void V8Debugger::requestPauseAfterInstrumentation() { + m_requestedPauseAfterInstrumentation = true; +} + void V8Debugger::continueProgram(int targetContextGroupId, bool terminateOnResume) { if (m_pausedContextGroupId != targetContextGroupId) return; @@ -510,11 +518,11 @@ void V8Debugger::ScriptCompiled(v8::Local script, }); } -void V8Debugger::BreakOnInstrumentation( +V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation( v8::Local pausedContext, v8::debug::BreakpointId instrumentationId) { // Don't allow nested breaks. - if (isPaused()) return; + if (isPaused()) return kNoPauseAfterInstrumentationRequested; int contextGroupId = m_inspector->contextGroupId(pausedContext); bool hasAgents = false; @@ -523,9 +531,10 @@ void V8Debugger::BreakOnInstrumentation( if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */)) hasAgents = true; }); - if (!hasAgents) return; + if (!hasAgents) return kNoPauseAfterInstrumentationRequested; m_pausedContextGroupId = contextGroupId; + m_instrumentationPause = true; m_inspector->forEachSession( contextGroupId, [instrumentationId](V8InspectorSessionImpl* session) { if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */)) { @@ -536,14 +545,22 @@ void V8Debugger::BreakOnInstrumentation( { v8::Context::Scope scope(pausedContext); m_inspector->client()->runMessageLoopOnInstrumentationPause(contextGroupId); - m_pausedContextGroupId = 0; } + bool requestedPauseAfterInstrumentation = + m_requestedPauseAfterInstrumentation; + + m_requestedPauseAfterInstrumentation = false; + m_pausedContextGroupId = 0; + m_instrumentationPause = false; m_inspector->forEachSession(contextGroupId, [](V8InspectorSessionImpl* session) { if (session->debuggerAgent()->enabled()) session->debuggerAgent()->didContinue(); }); + return requestedPauseAfterInstrumentation + ? kPauseAfterInstrumentationRequested + : kNoPauseAfterInstrumentationRequested; } void V8Debugger::BreakProgramRequested( diff --git a/src/inspector/v8-debugger.h b/src/inspector/v8-debugger.h index a44fd335c9..fda2ce0f17 100644 --- a/src/inspector/v8-debugger.h +++ b/src/inspector/v8-debugger.h @@ -58,8 +58,10 @@ class V8Debugger : public v8::debug::DebugDelegate, v8::debug::ExceptionBreakState getPauseOnExceptionsState(); void setPauseOnExceptionsState(v8::debug::ExceptionBreakState); bool canBreakProgram(); + bool isInInstrumentationPause() const; void breakProgram(int targetContextGroupId); void interruptAndBreak(int targetContextGroupId); + void requestPauseAfterInstrumentation(); void continueProgram(int targetContextGroupId, bool terminateOnResume = false); void breakProgramOnAssert(int targetContextGroupId); @@ -191,8 +193,8 @@ class V8Debugger : public v8::debug::DebugDelegate, v8::Local paused_context, const std::vector& break_points_hit, v8::debug::BreakReasons break_reasons) override; - void BreakOnInstrumentation(v8::Local paused_context, - v8::debug::BreakpointId) override; + PauseAfterInstrumentation BreakOnInstrumentation( + v8::Local paused_context, v8::debug::BreakpointId) override; void ExceptionThrown(v8::Local paused_context, v8::Local exception, v8::Local promise, bool is_uncaught, @@ -218,6 +220,8 @@ class V8Debugger : public v8::debug::DebugDelegate, bool m_scheduledOOMBreak = false; int m_targetContextGroupId = 0; int m_pausedContextGroupId = 0; + bool m_instrumentationPause = false; + bool m_requestedPauseAfterInstrumentation = false; int m_continueToLocationBreakpointId; String16 m_continueToLocationTargetCallFrames; std::unique_ptr m_continueToLocationStack; diff --git a/test/inspector/debugger/pause-on-instrumentation-expected.txt b/test/inspector/debugger/pause-on-instrumentation-expected.txt new file mode 100644 index 0000000000..0a24b69c8f --- /dev/null +++ b/test/inspector/debugger/pause-on-instrumentation-expected.txt @@ -0,0 +1,10 @@ +Test if pauses break execution when requested during instrumentation pause in js + +Running test: testPauseDuringInstrumentationPause +Set instrumentation breakpoints and requested script evaluation. +Paused at foo.js with reason "instrumentation". +Requested debugger pause. +Resumed. +Paused at foo.js with reason "other". +Resumed. +Done. diff --git a/test/inspector/debugger/pause-on-instrumentation.js b/test/inspector/debugger/pause-on-instrumentation.js new file mode 100644 index 0000000000..d3b22eaaf8 --- /dev/null +++ b/test/inspector/debugger/pause-on-instrumentation.js @@ -0,0 +1,56 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +const {session, contextGroup, Protocol} = InspectorTest.start( + 'Test if pauses break execution when requested during instrumentation pause in js'); + +session.setupScriptMap(); + +function logPause(msg) { + const top_frame = msg.params.callFrames[0]; + const reason = msg.params.reason; + const url = session.getCallFrameUrl(top_frame); + InspectorTest.log(`Paused at ${url} with reason "${reason}".`); +}; + +// Test that requests to pause during instrumentation pause cause the debugger +// to pause after it is done with the instrumentation pause. +async function testPauseDuringInstrumentationPause() { + await Protocol.Runtime.enable(); + await Protocol.Debugger.enable(); + + await Protocol.Debugger.setInstrumentationBreakpoint( + {instrumentation: 'beforeScriptExecution'}); + const runPromise = Protocol.Runtime.evaluate( + {expression: 'console.log("Hi");\n//# sourceURL=foo.js'}); + InspectorTest.log( + 'Set instrumentation breakpoints and requested script evaluation.'); + + // First pause: instrumentation + { + const pause = await Protocol.Debugger.oncePaused(); + logPause(pause); + // Request debugger pause from within the instrumentation pause. + await Protocol.Debugger.pause({}); + InspectorTest.log('Requested debugger pause.'); + await Protocol.Debugger.resume(); + InspectorTest.log('Resumed.'); + } + + // Second pause: explicit pause request + { + const pause = await Protocol.Debugger.oncePaused(); + logPause(pause); + await Protocol.Debugger.resume(); + InspectorTest.log('Resumed.'); + } + + await runPromise; + + InspectorTest.log('Done.'); + await Protocol.Runtime.disable(); + await Protocol.Debugger.disable(); +} + +InspectorTest.runAsyncTestSuite([testPauseDuringInstrumentationPause]); diff --git a/test/inspector/debugger/set-breakpoint-on-instrumentation.js b/test/inspector/debugger/set-breakpoint-on-instrumentation.js index 41f885a06e..f1cc8ad4e8 100644 --- a/test/inspector/debugger/set-breakpoint-on-instrumentation.js +++ b/test/inspector/debugger/set-breakpoint-on-instrumentation.js @@ -31,7 +31,7 @@ function handlePause(msg) { const url = session.getCallFrameUrl(top_frame); InspectorTest.log(`Paused at ${url} with reason "${reason}".`); InspectorTest.log( - `Hit breakpoints: ${JSON.stringify(msg.params.hitBreakpoints)}`) + `Hit breakpoints: ${JSON.stringify(msg.params.hitBreakpoints)}`); return Protocol.Debugger.resume(); };