[inspector] Make breakpoints active state per-agent

... as opposite to a global per-isolate one.
Also streamlined multiple checks into a single acceptsPause() method.

BUG=chromium:590878

Review-Url: https://codereview.chromium.org/2925903002
Cr-Commit-Position: refs/heads/master@{#45749}

src/inspector/v8-debugger-agent-impl.cc

@@ -211,13 +211,10 @@ void V8DebuggerAgentImpl::enableImpl() {
   for (size_t i = 0; i < compiledScripts.size(); i++)
     didParseSource(std::move(compiledScripts[i]), true);
 
-  // FIXME(WK44513): breakpoints activated flag should be synchronized between
-  // all front-ends
-  m_debugger->setBreakpointsActivated(true);
+  m_breakpointsActive = true;
+  m_debugger->setBreakpointsActive(true);
 }
 
-bool V8DebuggerAgentImpl::enabled() { return m_enabled; }
-
 Response V8DebuggerAgentImpl::enable() {
   if (enabled()) return Response::OK();
 
@@ -238,6 +235,10 @@ Response V8DebuggerAgentImpl::disable() {
   m_state->setInteger(DebuggerAgentState::asyncCallStackDepth, 0);
 
   if (isPaused()) m_debugger->continueProgram(m_session->contextGroupId());
+  if (m_breakpointsActive) {
+    m_debugger->setBreakpointsActive(false);
+    m_breakpointsActive = false;
+  }
   m_debugger->disable();
   JavaScriptCallFrames emptyCallFrames;
   m_pausedCallFrames.swap(emptyCallFrames);
@@ -286,8 +287,9 @@ void V8DebuggerAgentImpl::restore() {
 
 Response V8DebuggerAgentImpl::setBreakpointsActive(bool active) {
   if (!enabled()) return Response::Error(kDebuggerNotEnabled);
-  // TODO(dgozman): this state should be per-session, not global per debugger.
-  m_debugger->setBreakpointsActivated(active);
+  if (m_breakpointsActive == active) return Response::OK();
+  m_breakpointsActive = active;
+  m_debugger->setBreakpointsActive(active);
   if (!active && !m_breakReason.empty()) {
     clearBreakDetails();
     m_debugger->setPauseOnNextStatement(false, m_session->contextGroupId());
@@ -528,6 +530,10 @@ bool V8DebuggerAgentImpl::isFunctionBlackboxed(const String16& scriptId,
          std::distance(ranges.begin(), itStartRange) % 2;
 }
 
+bool V8DebuggerAgentImpl::acceptsPause(bool isOOMBreak) const {
+  return enabled() && (isOOMBreak || !m_skipAllPauses);
+}
+
 std::unique_ptr<protocol::Debugger::Location>
 V8DebuggerAgentImpl::resolveBreakpoint(const String16& breakpointId,
                                        const ScriptBreakpoint& breakpoint,
@@ -683,7 +689,7 @@ void V8DebuggerAgentImpl::clearBreakDetails() {
 void V8DebuggerAgentImpl::schedulePauseOnNextStatement(
     const String16& breakReason,
     std::unique_ptr<protocol::DictionaryValue> data) {
-  if (!enabled() || isPaused() || !m_debugger->breakpointsActivated()) return;
+  if (isPaused() || !acceptsPause(false) || !m_breakpointsActive) return;
   if (m_breakReason.empty()) {
     m_debugger->setPauseOnNextStatement(true, m_session->contextGroupId());
   }
@@ -691,7 +697,7 @@ void V8DebuggerAgentImpl::schedulePauseOnNextStatement(
 }
 
 void V8DebuggerAgentImpl::cancelPauseOnNextStatement() {
-  if (!enabled() || isPaused() || !m_debugger->breakpointsActivated()) return;
+  if (isPaused() || !acceptsPause(false) || !m_breakpointsActive) return;
   if (m_breakReason.size() == 1) {
     m_debugger->setPauseOnNextStatement(false, m_session->contextGroupId());
   }
@@ -1242,7 +1248,7 @@ void V8DebuggerAgentImpl::didContinue() {
 void V8DebuggerAgentImpl::breakProgram(
     const String16& breakReason,
     std::unique_ptr<protocol::DictionaryValue> data) {
-  if (!enabled() || !m_debugger->canBreakProgram() || m_skipAllPauses) return;
+  if (!enabled() || m_skipAllPauses || !m_debugger->canBreakProgram()) return;
   std::vector<BreakReason> currentScheduledReason;
   currentScheduledReason.swap(m_breakReason);
   pushBreakDetails(breakReason, std::move(data));

src/inspector/v8-debugger-agent-impl.h

@@ -109,7 +109,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
       std::unique_ptr<protocol::Array<protocol::Debugger::ScriptPosition>>
           positions) override;
 
-  bool enabled();
+  bool enabled() const { return m_enabled; }
 
   void setBreakpointAt(const String16& scriptId, int lineNumber,
                        int columnNumber, BreakpointSource,
@@ -137,7 +137,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
                             const v8::debug::Location& start,
                             const v8::debug::Location& end);
 
-  bool skipAllPauses() const { return m_skipAllPauses; }
+  bool acceptsPause(bool isOOMBreak) const;
 
   v8::Isolate* isolate() { return m_isolate; }
 
@@ -194,6 +194,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   void popBreakDetails();
 
   bool m_skipAllPauses = false;
+  bool m_breakpointsActive = false;
 
   std::unique_ptr<V8Regex> m_blackboxPattern;
   protocol::HashMap<String16, std::vector<std::pair<int, int>>>

src/inspector/v8-debugger.cc

@@ -161,7 +161,6 @@ V8Debugger::V8Debugger(v8::Isolate* isolate, V8InspectorImpl* inspector)
     : m_isolate(isolate),
       m_inspector(inspector),
       m_enableCount(0),
-      m_breakpointsActivated(true),
       m_ignoreScriptParsedEventsCounter(0),
       m_maxAsyncCallStacks(kMaxAsyncTaskStacks),
       m_maxAsyncCallStackDepth(0),
@@ -309,13 +308,13 @@ void V8Debugger::clearBreakpoints() {
   v8::debug::Call(debuggerContext(), clearBreakpoints).ToLocalChecked();
 }
 
-void V8Debugger::setBreakpointsActivated(bool activated) {
+void V8Debugger::setBreakpointsActive(bool active) {
   if (!enabled()) {
     UNREACHABLE();
     return;
   }
-  v8::debug::SetBreakPointsActive(m_isolate, activated);
-  m_breakpointsActivated = activated;
+  m_breakpointsActiveCount += active ? 1 : -1;
+  v8::debug::SetBreakPointsActive(m_isolate, m_breakpointsActiveCount);
 }
 
 v8::debug::ExceptionBreakState V8Debugger::getPauseOnExceptionsState() {
@@ -347,14 +346,13 @@ void V8Debugger::setPauseOnNextStatement(bool pause, int targetContextGroupId) {
 }
 
 bool V8Debugger::canBreakProgram() {
-  if (!m_breakpointsActivated) return false;
   return !v8::debug::AllFramesOnStackAreBlackboxed(m_isolate);
 }
 
 void V8Debugger::breakProgram(int targetContextGroupId) {
+  DCHECK(canBreakProgram());
   // Don't allow nested breaks.
   if (isPaused()) return;
-  if (!canBreakProgram()) return;
   DCHECK(targetContextGroupId);
   m_targetContextGroupId = targetContextGroupId;
   v8::debug::BreakRightNow(m_isolate);
@@ -616,15 +614,12 @@ void V8Debugger::handleProgramBreak(v8::Local<v8::Context> pausedContext,
 
   bool scheduledOOMBreak = m_scheduledOOMBreak;
   bool scheduledAssertBreak = m_scheduledAssertBreak;
-  auto agentCheck = [&scheduledOOMBreak](V8DebuggerAgentImpl* agent) {
-    return agent->enabled() && (scheduledOOMBreak || !agent->skipAllPauses());
-  };
-
   bool hasAgents = false;
   m_inspector->forEachSession(
       contextGroupId,
-      [&agentCheck, &hasAgents](V8InspectorSessionImpl* session) {
-        if (agentCheck(session->debuggerAgent())) hasAgents = true;
+      [&scheduledOOMBreak, &hasAgents](V8InspectorSessionImpl* session) {
+        if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak))
+          hasAgents = true;
       });
   if (!hasAgents) return;
 
@@ -652,10 +647,10 @@ void V8Debugger::handleProgramBreak(v8::Local<v8::Context> pausedContext,
   m_pausedContextGroupId = contextGroupId;
 
   m_inspector->forEachSession(
-      contextGroupId, [&agentCheck, &pausedContext, &exception, &breakpointIds,
+      contextGroupId, [&pausedContext, &exception, &breakpointIds,
                        &isPromiseRejection, &isUncaught, &scheduledOOMBreak,
                        &scheduledAssertBreak](V8InspectorSessionImpl* session) {
-        if (agentCheck(session->debuggerAgent())) {
+        if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
           session->debuggerAgent()->didPause(
               InspectedContext::contextId(pausedContext), exception,
               breakpointIds, isPromiseRejection, isUncaught, scheduledOOMBreak,

src/inspector/v8-debugger.h

@@ -44,8 +44,7 @@ class V8Debugger : public v8::debug::DebugDelegate {
   String16 setBreakpoint(const ScriptBreakpoint&, int* actualLineNumber,
                          int* actualColumnNumber);
   void removeBreakpoint(const String16& breakpointId);
-  void setBreakpointsActivated(bool);
-  bool breakpointsActivated() const { return m_breakpointsActivated; }
+  void setBreakpointsActive(bool);
 
   v8::debug::ExceptionBreakState getPauseOnExceptionsState();
   void setPauseOnExceptionsState(v8::debug::ExceptionBreakState);
@@ -183,7 +182,7 @@ class V8Debugger : public v8::debug::DebugDelegate {
   v8::Isolate* m_isolate;
   V8InspectorImpl* m_inspector;
   int m_enableCount;
-  bool m_breakpointsActivated;
+  int m_breakpointsActiveCount = 0;
   v8::Global<v8::Object> m_debuggerScript;
   v8::Global<v8::Context> m_debuggerContext;
   v8::Local<v8::Object> m_executionState;

test/inspector/sessions/debugger-stepping-and-breakpoints-expected.txt

@@ -188,3 +188,16 @@ Resuming in 2
 Resumed in 2
 Skipping pauses in 2
 Evaluating common breakpoint in 1
+Unskipping pauses in 1
+Unskipping pauses in 2
+Deactivating breakpoints in 1
+Evaluating common breakpoint in 1
+Paused in 2:
+  reason: other
+  hit breakpoints: test.js:11:0
+  location: foo@11
+  data: null
+Resuming in 2
+Resumed in 2
+Deactivating breakpoints in 2
+Evaluating common breakpoint in 1

test/inspector/sessions/debugger-stepping-and-breakpoints.js

@@ -143,6 +143,25 @@ function bar() {
   InspectorTest.log('Evaluating common breakpoint in 1');
   await session1.Protocol.Runtime.evaluate({expression: 'foo();'});
 
+  InspectorTest.log('Unskipping pauses in 1');
+  await session1.Protocol.Debugger.setSkipAllPauses({skip: false});
+  InspectorTest.log('Unskipping pauses in 2');
+  await session2.Protocol.Debugger.setSkipAllPauses({skip: false});
+
+  InspectorTest.log('Deactivating breakpoints in 1');
+  await session1.Protocol.Debugger.setBreakpointsActive({active: false});
+  InspectorTest.log('Evaluating common breakpoint in 1');
+  session1.Protocol.Runtime.evaluate({expression: 'foo();'});
+  await waitForPaused(session2, 2);
+  InspectorTest.log('Resuming in 2');
+  session2.Protocol.Debugger.resume();
+  await waitForResumed(session2, 2);
+
+  InspectorTest.log('Deactivating breakpoints in 2');
+  await session2.Protocol.Debugger.setBreakpointsActive({active: false});
+  InspectorTest.log('Evaluating common breakpoint in 1');
+  await session1.Protocol.Runtime.evaluate({expression: 'foo();'});
+
   InspectorTest.completeTest();
 
   function waitForBothPaused() {