From f4fb8fc1f7e2760948d2d9b53f6e975a05030c04 Mon Sep 17 00:00:00 2001 From: Jaroslav Sevcik Date: Thu, 8 Dec 2022 16:55:38 +0100 Subject: [PATCH] [inspector] Introduce debugger session stop API We introduce V8InspectorSession::stop API to enable safe detach from the session. In particular, after calling 'stop', the session will leave any instrumentation pause it might be in and disarm all its instrumentation breakpoints. This is useful when the session disconnect request is registered on V8 interrupt (so it is unsafe to disconnect at that point), and the execution should first get to the message loop where the disconnect can be handled safely. Bug: chromium:1354043 Change-Id: I3caab12a21b123229835e8374efadc1f4c9954c2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4085143 Reviewed-by: Benedikt Meurer Reviewed-by: Kim-Anh Tran Commit-Queue: Jaroslav Sevcik Cr-Commit-Position: refs/heads/main@{#84753} --- include/v8-inspector.h | 3 + src/debug/debug-interface.h | 11 ++-- src/debug/debug.cc | 21 +++++-- src/debug/debug.h | 2 +- src/inspector/v8-debugger-agent-impl.cc | 15 +++-- src/inspector/v8-debugger-agent-impl.h | 12 +++- src/inspector/v8-debugger.cc | 29 +++++---- src/inspector/v8-debugger.h | 2 +- src/inspector/v8-inspector-session-impl.cc | 2 + src/inspector/v8-inspector-session-impl.h | 2 + .../debugger/session-stop-expected.txt | 15 +++++ test/inspector/debugger/session-stop.js | 60 +++++++++++++++++++ test/inspector/inspector-test.cc | 13 ++++ test/inspector/isolate-data.cc | 8 +++ test/inspector/isolate-data.h | 1 + test/inspector/protocol-test.js | 4 ++ 16 files changed, 170 insertions(+), 30 deletions(-) create mode 100644 test/inspector/debugger/session-stop-expected.txt create mode 100644 test/inspector/debugger/session-stop.js diff --git a/include/v8-inspector.h b/include/v8-inspector.h index d43c2ead0b..563ad196d6 100644 --- a/include/v8-inspector.h +++ b/include/v8-inspector.h @@ -212,6 +212,9 @@ class V8_EXPORT V8InspectorSession { std::unique_ptr* objectGroup) = 0; virtual void releaseObjectGroup(StringView) = 0; virtual void triggerPreciseCoverageDeltaUpdate(StringView occasion) = 0; + + // Prepare for shutdown (disables debugger pausing, etc.). + virtual void stop() = 0; }; class V8_EXPORT WebDriverValue { diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h index 30e736ae2c..e2721d8f44 100644 --- a/src/debug/debug-interface.h +++ b/src/debug/debug-interface.h @@ -287,14 +287,15 @@ class DebugDelegate { v8::Local paused_context, const std::vector& inspector_break_points_hit, base::EnumSet break_reasons = {}) {} - enum PauseAfterInstrumentation { - kPauseAfterInstrumentationRequested, - kNoPauseAfterInstrumentationRequested + enum class ActionAfterInstrumentation { + kPause, + kPauseIfBreakpointsHit, + kContinue }; - virtual PauseAfterInstrumentation BreakOnInstrumentation( + virtual ActionAfterInstrumentation BreakOnInstrumentation( v8::Local paused_context, const debug::BreakpointId instrumentationId) { - return kNoPauseAfterInstrumentationRequested; + return ActionAfterInstrumentation::kPauseIfBreakpointsHit; } virtual void ExceptionThrown(v8::Local paused_context, v8::Local exception, diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 9c6c5f4ca5..ce9b7feaee 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -480,11 +480,12 @@ void Debug::Unload() { debug_delegate_ = nullptr; } -debug::DebugDelegate::PauseAfterInstrumentation +debug::DebugDelegate::ActionAfterInstrumentation Debug::OnInstrumentationBreak() { RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger); if (!debug_delegate_) { - return debug::DebugDelegate::kNoPauseAfterInstrumentationRequested; + return debug::DebugDelegate::ActionAfterInstrumentation:: + kPauseIfBreakpointsHit; } DCHECK(in_debug_scope()); HandleScope scope(isolate_); @@ -517,11 +518,19 @@ void Debug::Break(JavaScriptFrame* frame, Handle break_target) { IsBreakOnInstrumentation(debug_info, location); bool shouldPauseAfterInstrumentation = false; if (hitInstrumentationBreak) { - debug::DebugDelegate::PauseAfterInstrumentation pauseDuringInstrumentation = + debug::DebugDelegate::ActionAfterInstrumentation action = OnInstrumentationBreak(); - shouldPauseAfterInstrumentation = - pauseDuringInstrumentation == - debug::DebugDelegate::kPauseAfterInstrumentationRequested; + switch (action) { + case debug::DebugDelegate::ActionAfterInstrumentation::kPause: + shouldPauseAfterInstrumentation = true; + break; + case debug::DebugDelegate::ActionAfterInstrumentation:: + kPauseIfBreakpointsHit: + shouldPauseAfterInstrumentation = false; + break; + case debug::DebugDelegate::ActionAfterInstrumentation::kContinue: + return; + } } // Find actual break points, if any, and trigger debug break event. diff --git a/src/debug/debug.h b/src/debug/debug.h index a47b693b5e..3be05bbd94 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 = {}); - debug::DebugDelegate::PauseAfterInstrumentation OnInstrumentationBreak(); + debug::DebugDelegate::ActionAfterInstrumentation 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 e3c1444fbc..f38a7503e2 100644 --- a/src/inspector/v8-debugger-agent-impl.cc +++ b/src/inspector/v8-debugger-agent-impl.cc @@ -371,7 +371,7 @@ V8DebuggerAgentImpl::V8DebuggerAgentImpl( : m_inspector(session->inspector()), m_debugger(m_inspector->debugger()), m_session(session), - m_enabled(false), + m_enableState(kDisabled), m_state(state), m_frontend(frontendChannel), m_isolate(m_inspector->isolate()) {} @@ -379,7 +379,7 @@ V8DebuggerAgentImpl::V8DebuggerAgentImpl( V8DebuggerAgentImpl::~V8DebuggerAgentImpl() = default; void V8DebuggerAgentImpl::enableImpl() { - m_enabled = true; + m_enableState = kEnabled; m_state->setBoolean(DebuggerAgentState::debuggerEnabled, true); m_debugger->enable(); @@ -401,6 +401,8 @@ void V8DebuggerAgentImpl::enableImpl() { Response V8DebuggerAgentImpl::enable(Maybe maxScriptsCacheSize, String16* outDebuggerId) { + if (m_enableState == kStopping) + return Response::ServerError("Debugger is stopping"); m_maxScriptCacheSize = v8::base::saturated_cast( maxScriptsCacheSize.fromMaybe(std::numeric_limits::max())); *outDebuggerId = @@ -449,14 +451,14 @@ Response V8DebuggerAgentImpl::disable() { m_skipAllPauses = false; m_state->setBoolean(DebuggerAgentState::skipAllPauses, false); m_state->remove(DebuggerAgentState::blackboxPattern); - m_enabled = false; + m_enableState = kDisabled; m_state->setBoolean(DebuggerAgentState::debuggerEnabled, false); m_debugger->disable(); return Response::Success(); } void V8DebuggerAgentImpl::restore() { - DCHECK(!m_enabled); + DCHECK(m_enableState == kDisabled); if (!m_state->booleanProperty(DebuggerAgentState::debuggerEnabled, false)) return; if (!m_inspector->client()->canExecuteScripts(m_session->contextGroupId())) @@ -2197,4 +2199,9 @@ Response V8DebuggerAgentImpl::processSkipList( m_skipList = std::move(skipListInit); return Response::Success(); } + +void V8DebuggerAgentImpl::stop() { + disable(); + m_enableState = kStopping; +} } // namespace v8_inspector diff --git a/src/inspector/v8-debugger-agent-impl.h b/src/inspector/v8-debugger-agent-impl.h index 09f9c3d4e4..836b738022 100644 --- a/src/inspector/v8-debugger-agent-impl.h +++ b/src/inspector/v8-debugger-agent-impl.h @@ -43,6 +43,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend { V8DebuggerAgentImpl(const V8DebuggerAgentImpl&) = delete; V8DebuggerAgentImpl& operator=(const V8DebuggerAgentImpl&) = delete; void restore(); + void stop(); // Part of the protocol. Response enable(Maybe maxScriptsCacheSize, @@ -144,7 +145,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend { std::unique_ptr> positions) override; - bool enabled() const { return m_enabled; } + bool enabled() const { return m_enableState == kEnabled; } void setBreakpointFor(v8::Local function, v8::Local condition, @@ -222,10 +223,17 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend { using DebuggerBreakpointIdToBreakpointIdMap = std::unordered_map; + enum EnableState { + kDisabled, + kEnabled, + kStopping, // This is the same as 'disabled', but it cannot become enabled + // again. + }; + V8InspectorImpl* m_inspector; V8Debugger* m_debugger; V8InspectorSessionImpl* m_session; - bool m_enabled; + EnableState m_enableState; protocol::DictionaryValue* m_state; protocol::Debugger::Frontend m_frontend; v8::Isolate* m_isolate; diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc index 70fc9c3ed7..6ecb753152 100644 --- a/src/inspector/v8-debugger.cc +++ b/src/inspector/v8-debugger.cc @@ -545,11 +545,11 @@ void V8Debugger::ScriptCompiled(v8::Local script, }); } -V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation( +V8Debugger::ActionAfterInstrumentation V8Debugger::BreakOnInstrumentation( v8::Local pausedContext, v8::debug::BreakpointId instrumentationId) { // Don't allow nested breaks. - if (isPaused()) return kNoPauseAfterInstrumentationRequested; + if (isPaused()) return ActionAfterInstrumentation::kPauseIfBreakpointsHit; int contextGroupId = m_inspector->contextGroupId(pausedContext); bool hasAgents = false; @@ -558,7 +558,7 @@ V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation( if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */)) hasAgents = true; }); - if (!hasAgents) return kNoPauseAfterInstrumentationRequested; + if (!hasAgents) return ActionAfterInstrumentation::kPauseIfBreakpointsHit; m_pausedContextGroupId = contextGroupId; m_instrumentationPause = true; @@ -580,14 +580,21 @@ V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation( m_pausedContextGroupId = 0; m_instrumentationPause = false; - m_inspector->forEachSession(contextGroupId, - [](V8InspectorSessionImpl* session) { - if (session->debuggerAgent()->enabled()) - session->debuggerAgent()->didContinue(); - }); - return requestedPauseAfterInstrumentation - ? kPauseAfterInstrumentationRequested - : kNoPauseAfterInstrumentationRequested; + hasAgents = false; + m_inspector->forEachSession( + contextGroupId, [&hasAgents](V8InspectorSessionImpl* session) { + if (session->debuggerAgent()->enabled()) + session->debuggerAgent()->didContinue(); + if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */)) + hasAgents = true; + }); + if (!hasAgents) { + return ActionAfterInstrumentation::kContinue; + } else if (requestedPauseAfterInstrumentation) { + return ActionAfterInstrumentation::kPause; + } else { + return ActionAfterInstrumentation::kPauseIfBreakpointsHit; + } } void V8Debugger::BreakProgramRequested( diff --git a/src/inspector/v8-debugger.h b/src/inspector/v8-debugger.h index d16250137a..6378a69947 100644 --- a/src/inspector/v8-debugger.h +++ b/src/inspector/v8-debugger.h @@ -194,7 +194,7 @@ class V8Debugger : public v8::debug::DebugDelegate, v8::Local paused_context, const std::vector& break_points_hit, v8::debug::BreakReasons break_reasons) override; - PauseAfterInstrumentation BreakOnInstrumentation( + ActionAfterInstrumentation BreakOnInstrumentation( v8::Local paused_context, v8::debug::BreakpointId) override; void ExceptionThrown(v8::Local paused_context, v8::Local exception, diff --git a/src/inspector/v8-inspector-session-impl.cc b/src/inspector/v8-inspector-session-impl.cc index 35dca5540d..7b04e5f916 100644 --- a/src/inspector/v8-inspector-session-impl.cc +++ b/src/inspector/v8-inspector-session-impl.cc @@ -511,4 +511,6 @@ void V8InspectorSessionImpl::triggerPreciseCoverageDeltaUpdate( m_profilerAgent->triggerPreciseCoverageDeltaUpdate(toString16(occasion)); } +void V8InspectorSessionImpl::stop() { m_debuggerAgent->stop(); } + } // namespace v8_inspector diff --git a/src/inspector/v8-inspector-session-impl.h b/src/inspector/v8-inspector-session-impl.h index b5ca62fd52..9e443161c4 100644 --- a/src/inspector/v8-inspector-session-impl.h +++ b/src/inspector/v8-inspector-session-impl.h @@ -100,6 +100,8 @@ class V8InspectorSessionImpl : public V8InspectorSession, static const unsigned kInspectedObjectBufferSize = 5; void triggerPreciseCoverageDeltaUpdate(StringView occasion) override; + void stop() override; + V8Inspector::ClientTrustLevel clientTrustLevel() { return m_clientTrustLevel; } diff --git a/test/inspector/debugger/session-stop-expected.txt b/test/inspector/debugger/session-stop-expected.txt new file mode 100644 index 0000000000..344f214707 --- /dev/null +++ b/test/inspector/debugger/session-stop-expected.txt @@ -0,0 +1,15 @@ +Checks V8InspectorSession::stop + +Running test: testSessionStopResumesPause +Evaluation returned: 42 + +Running test: testSessionStopResumesInstrumentationPause +Paused: instrumentation +Evaluation returned: 42 + +Running test: testSessionStopDisablesDebugger +Pause error(?): Debugger agent is not enabled + +Running test: testSessionStopDisallowsReenabling +Pause error(?) after stop: Debugger agent is not enabled +Pause error(?) after re-enable: Debugger agent is not enabled diff --git a/test/inspector/debugger/session-stop.js b/test/inspector/debugger/session-stop.js new file mode 100644 index 0000000000..540e85f026 --- /dev/null +++ b/test/inspector/debugger/session-stop.js @@ -0,0 +1,60 @@ +// 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. + +InspectorTest.log('Checks V8InspectorSession::stop'); + +InspectorTest.runAsyncTestSuite([ + async function testSessionStopResumesPause() { + let contextGroup = new InspectorTest.ContextGroup(); + let session = contextGroup.connect(); + let Protocol = session.Protocol; + + Protocol.Debugger.enable(); + await Protocol.Debugger.pause(); + const result = Protocol.Runtime.evaluate({expression: '42'}); + contextGroup.stop(); + InspectorTest.log( + `Evaluation returned: ${(await result).result.result.value}`); + }, + async function testSessionStopResumesInstrumentationPause() { + let contextGroup = new InspectorTest.ContextGroup(); + let session = contextGroup.connect(); + let Protocol = session.Protocol; + + Protocol.Debugger.enable(); + await Protocol.Debugger.setInstrumentationBreakpoint( + {instrumentation: 'beforeScriptExecution'}); + const paused = Protocol.Debugger.oncePaused(); + const result = Protocol.Runtime.evaluate({expression: '42'}); + InspectorTest.log(`Paused: ${(await paused).params.reason}`); + contextGroup.stop(); + InspectorTest.log( + `Evaluation returned: ${(await result).result.result.value}`); + }, + async function testSessionStopDisablesDebugger() { + let contextGroup = new InspectorTest.ContextGroup(); + let session = contextGroup.connect(); + let Protocol = session.Protocol; + + await Protocol.Debugger.enable(); + contextGroup.stop(); + const pauseResult = await Protocol.Debugger.pause(); + InspectorTest.log(`Pause error(?): ${pauseResult?.error?.message}`); + }, + async function testSessionStopDisallowsReenabling() { + let contextGroup = new InspectorTest.ContextGroup(); + let session = contextGroup.connect(); + let Protocol = session.Protocol; + + await Protocol.Debugger.enable(); + contextGroup.stop(); + const pauseResultAfterStop = await Protocol.Debugger.pause(); + InspectorTest.log( + `Pause error(?) after stop: ${pauseResultAfterStop?.error?.message}`); + await Protocol.Debugger.enable(); + const pauseResult = await Protocol.Debugger.pause(); + InspectorTest.log( + `Pause error(?) after re-enable: ${pauseResult?.error?.message}`); + } +]); diff --git a/test/inspector/inspector-test.cc b/test/inspector/inspector-test.cc index f3cbaffa16..37e1ca6846 100644 --- a/test/inspector/inspector-test.cc +++ b/test/inspector/inspector-test.cc @@ -73,6 +73,8 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask { utils->Set(isolate, "cancelPauseOnNextStatement", v8::FunctionTemplate::New( isolate, &UtilsExtension::CancelPauseOnNextStatement)); + utils->Set(isolate, "stop", + v8::FunctionTemplate::New(isolate, &UtilsExtension::Stop)); utils->Set(isolate, "setLogConsoleApiMessageCalls", v8::FunctionTemplate::New( isolate, &UtilsExtension::SetLogConsoleApiMessageCalls)); @@ -275,6 +277,17 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask { }); } + static void Stop(const v8::FunctionCallbackInfo& args) { + if (args.Length() != 1 || !args[0]->IsInt32()) { + FATAL("Internal error: stop(context_group_id)."); + } + int context_group_id = args[0].As()->Value(); + RunSyncTask(backend_runner_, + [&context_group_id](InspectorIsolateData* data) { + data->Stop(context_group_id); + }); + } + static void SetLogConsoleApiMessageCalls( const v8::FunctionCallbackInfo& args) { if (args.Length() != 1 || !args[0]->IsBoolean()) { diff --git a/test/inspector/isolate-data.cc b/test/inspector/isolate-data.cc index 98da172b68..9c09eccac8 100644 --- a/test/inspector/isolate-data.cc +++ b/test/inspector/isolate-data.cc @@ -201,6 +201,14 @@ void InspectorIsolateData::BreakProgram( } } +void InspectorIsolateData::Stop(int context_group_id) { + v8::SealHandleScope seal_handle_scope(isolate()); + for (int session_id : GetSessionIds(context_group_id)) { + auto it = sessions_.find(session_id); + if (it != sessions_.end()) it->second->stop(); + } +} + void InspectorIsolateData::SchedulePauseOnNextStatement( int context_group_id, const v8_inspector::StringView& reason, const v8_inspector::StringView& details) { diff --git a/test/inspector/isolate-data.h b/test/inspector/isolate-data.h index 2241a600ea..ff87581396 100644 --- a/test/inspector/isolate-data.h +++ b/test/inspector/isolate-data.h @@ -78,6 +78,7 @@ class InspectorIsolateData : public v8_inspector::V8InspectorClient { void BreakProgram(int context_group_id, const v8_inspector::StringView& reason, const v8_inspector::StringView& details); + void Stop(int context_group_id); void SchedulePauseOnNextStatement(int context_group_id, const v8_inspector::StringView& reason, const v8_inspector::StringView& details); diff --git a/test/inspector/protocol-test.js b/test/inspector/protocol-test.js index 80d1ea982f..2f6d52891d 100644 --- a/test/inspector/protocol-test.js +++ b/test/inspector/protocol-test.js @@ -160,6 +160,10 @@ InspectorTest.ContextGroup = class { utils.cancelPauseOnNextStatement(this.id); } + stop() { + utils.stop(this.id); + } + addScript(string, lineOffset, columnOffset, url) { utils.compileAndRunWithOrigin(this.id, string, url || '', lineOffset || 0, columnOffset || 0, false); }