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); }