[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.
Logic of conversion step-next at return into step-in is moved to debug.cc.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org

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

src/debug/debug.cc

@@ -1764,6 +1764,7 @@ void Debug::OnException(Handle<Object> exception, Handle<Object> promise) {
     if (!break_on_exception_) return;
   }
 
+  bool empty_js_stack = false;
   {
     JavaScriptFrameIterator it(isolate_);
     // Check whether the top frame is blackboxed or the break location is muted.
@@ -1771,12 +1772,13 @@ void Debug::OnException(Handle<Object> exception, Handle<Object> promise) {
                        IsExceptionBlackboxed(uncaught))) {
       return;
     }
+    empty_js_stack = it.done();
   }
 
   DebugScope debug_scope(this);
   if (debug_scope.failed()) return;
 
-  if (debug_delegate_) {
+  if (debug_delegate_ && !empty_js_stack) {
     HandleScope scope(isolate_);
 
     // Create the execution state.
@@ -1788,8 +1790,8 @@ void Debug::OnException(Handle<Object> exception, Handle<Object> promise) {
         GetDebugEventContext(isolate_),
         v8::Utils::ToLocal(Handle<JSObject>::cast(exec_state)),
         v8::Utils::ToLocal(exception), promise->IsJSObject(), uncaught);
-    if (!non_inspector_listener_exists()) return;
   }
+  if (debug_delegate_ && !non_inspector_listener_exists()) return;
 
   // Create the event data object.
   Handle<Object> event_data;

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

@@ -127,7 +127,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   void reset();
 
   // Interface for V8InspectorImpl
-  bool didPause(v8::Local<v8::Context>, v8::Local<v8::Value> exception,
+  void didPause(int contextId, v8::Local<v8::Value> exception,
                 const std::vector<String16>& hitBreakpoints,
                 bool isPromiseRejection, bool isUncaught, bool isOOMBreak);
   void didContinue();
@@ -139,6 +139,8 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
                             const v8::debug::Location& start,
                             const v8::debug::Location& end);
 
+  bool skipAllPauses() const { return m_skipAllPauses; }
+
   v8::Isolate* isolate() { return m_isolate; }
 
  private:
@@ -165,6 +167,8 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   Response setBlackboxPattern(const String16& pattern);
   void resetBlackboxedStateCache();
 
+  bool isPaused() const;
+
   using ScriptsMap =
       protocol::HashMap<String16, std::unique_ptr<V8DebuggerScript>>;
   using BreakpointIdToDebuggerBreakpointIdsMap =
@@ -182,7 +186,6 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   protocol::DictionaryValue* m_state;
   protocol::Debugger::Frontend m_frontend;
   v8::Isolate* m_isolate;
-  v8::Global<v8::Context> m_pausedContext;
   JavaScriptCallFrames m_pausedCallFrames;
   ScriptsMap m_scripts;
   BreakpointIdToDebuggerBreakpointIdsMap m_breakpointIdToDebuggerBreakpointIds;
@@ -194,7 +197,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   bool m_javaScriptPauseScheduled;
 
   int m_recursionLevelForStepOut;
-  bool m_skipAllPauses;
+  bool m_skipAllPauses = false;
 
   std::unique_ptr<V8Regex> m_blackboxPattern;
   protocol::HashMap<String16, std::vector<std::pair<int, int>>>

src/inspector/v8-debugger.cc

@@ -253,7 +253,7 @@ void V8Debugger::setPauseOnExceptionsState(
 }
 
 void V8Debugger::setPauseOnNextStatement(bool pause) {
-  if (m_runningNestedMessageLoop) return;
+  if (isPaused()) return;
   if (pause)
     v8::debug::DebugBreak(m_isolate);
   else
@@ -474,11 +474,11 @@ void V8Debugger::handleProgramBreak(v8::Local<v8::Context> pausedContext,
                                     v8::Local<v8::Array> hitBreakpointNumbers,
                                     bool isPromiseRejection, bool isUncaught) {
   // Don't allow nested breaks.
-  if (m_runningNestedMessageLoop) return;
+  if (isPaused()) return;
 
   V8DebuggerAgentImpl* agent = m_inspector->enabledDebuggerAgentForGroup(
       m_inspector->contextGroupId(pausedContext));
-  if (!agent) return;
+  if (!agent || (agent->skipAllPauses() && !m_scheduledOOMBreak)) return;
 
   std::vector<String16> breakpointIds;
   if (!hitBreakpointNumbers.IsEmpty()) {
@@ -494,24 +494,23 @@ void V8Debugger::handleProgramBreak(v8::Local<v8::Context> pausedContext,
 
   m_pausedContext = pausedContext;
   m_executionState = executionState;
-  bool shouldPause =
-      agent->didPause(pausedContext, exception, breakpointIds,
-                      isPromiseRejection, isUncaught, m_scheduledOOMBreak);
-  if (shouldPause) {
   m_runningNestedMessageLoop = true;
+  agent->didPause(InspectedContext::contextId(pausedContext), exception,
+                  breakpointIds, isPromiseRejection, isUncaught,
+                  m_scheduledOOMBreak);
   int groupId = m_inspector->contextGroupId(pausedContext);
   DCHECK(groupId);
+  {
     v8::Context::Scope scope(pausedContext);
     v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
     CHECK(!context.IsEmpty() &&
           context != v8::debug::GetDebugContext(m_isolate));
     m_inspector->client()->runMessageLoopOnPause(groupId);
-    // The agent may have been removed in the nested loop.
-    agent = m_inspector->enabledDebuggerAgentForGroup(
-        m_inspector->contextGroupId(pausedContext));
-    if (agent) agent->didContinue();
     m_runningNestedMessageLoop = false;
   }
+  // The agent may have been removed in the nested loop.
+  agent = m_inspector->enabledDebuggerAgentForGroup(groupId);
+  if (agent) agent->didContinue();
   if (m_scheduledOOMBreak) m_isolate->RestoreOriginalHeapLimit();
   m_scheduledOOMBreak = false;
   m_pausedContext.Clear();
@@ -836,8 +835,6 @@ v8::Local<v8::Value> V8Debugger::functionLocation(
   return location;
 }
 
-bool V8Debugger::isPaused() { return !m_pausedContext.IsEmpty(); }
-
 std::unique_ptr<V8StackTraceImpl> V8Debugger::createStackTrace(
     v8::Local<v8::StackTrace> stackTrace) {
   int contextGroupId =

src/inspector/v8-debugger.h

@@ -65,7 +65,7 @@ class V8Debugger : public v8::debug::DebugDelegate {
   void enable();
   void disable();
 
-  bool isPaused();
+  bool isPaused() const { return m_runningNestedMessageLoop; }
   v8::Local<v8::Context> pausedContext() { return m_pausedContext; }
 
   int maxAsyncCallChainDepth() { return m_maxAsyncCallStackDepth; }

test/inspector/debugger/step-into-next-script-expected.txt

+Debugger breaks in next script after stepOut from previous one.
+
+Running test: testStepOut
+test (foo.js:12:2)
+(anonymous) (:0:0)
+
+(anonymous) (:0:5)
+
+(anonymous) (timeout1.js:0:0)
+
+foo (timeout2.js:1:12)
+
+(anonymous) (timeout3.js:0:0)
+
+
+Running test: testStepOver
+(anonymous) (:0:0)
+
+test (foo.js:12:2)
+(anonymous) (:0:0)
+
+test (foo.js:13:0)
+(anonymous) (:0:0)
+
+(anonymous) (:0:5)
+
+(anonymous) (timeout1.js:0:0)
+
+(anonymous) (timeout1.js:0:8)
+
+(anonymous) (timeout1.js:0:34)
+
+foo (timeout2.js:1:12)
+
+foo (timeout2.js:2:2)
+
+foo (timeout2.js:3:0)
+
+(anonymous) (timeout3.js:0:0)
+
+(anonymous) (timeout3.js:0:8)
+
+(anonymous) (timeout3.js:0:34)
+
+
+Running test: testStepInto
+(anonymous) (:0:0)
+
+test (foo.js:9:2)
+(anonymous) (:0:0)
+
+test (foo.js:10:2)
+(anonymous) (:0:0)
+
+test (foo.js:11:2)
+(anonymous) (:0:0)
+
+test (foo.js:12:2)
+(anonymous) (:0:0)
+
+test (foo.js:13:0)
+(anonymous) (:0:0)
+
+(anonymous) (:0:5)
+
+(anonymous) (timeout1.js:0:0)
+
+(anonymous) (timeout1.js:0:8)
+
+(anonymous) (timeout1.js:0:34)
+
+foo (timeout2.js:1:12)
+
+foo (timeout2.js:2:2)
+
+foo (timeout2.js:3:0)
+
+(anonymous) (timeout3.js:0:0)
+
+(anonymous) (timeout3.js:0:8)
+
+(anonymous) (timeout3.js:0:34)
+

test/inspector/debugger/step-into-next-script.js

+// Copyright 2017 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.
+
+print('Debugger breaks in next script after stepOut from previous one.');
+
+InspectorTest.addScript(`
+function test() {
+  setTimeout('var a = 1;//# sourceURL=timeout1.js', 0);
+  setTimeout(foo, 0);
+  setTimeout('var a = 3;//# sourceURL=timeout3.js', 0);
+  debugger;
+}
+//# sourceURL=foo.js`, 7, 26);
+
+InspectorTest.addScript(`
+function foo() {
+  return 42;
+}
+//# sourceURL=timeout2.js`)
+
+InspectorTest.setupScriptMap();
+var stepAction;
+Protocol.Debugger.onPaused(message => {
+  InspectorTest.logCallFrames(message.params.callFrames);
+  InspectorTest.log('');
+  Protocol.Debugger[stepAction]();
+});
+Protocol.Debugger.enable()
+InspectorTest.runTestSuite([
+  function testStepOut(next) {
+    stepAction = 'stepOut';
+    Protocol.Runtime.evaluate({ expression: 'test()' })
+      .then(() => InspectorTest.waitPendingTasks())
+      .then(next);
+  },
+
+  function testStepOver(next) {
+    stepAction = 'stepOver';
+    Protocol.Runtime.evaluate({ expression: 'test()' })
+      .then(() => InspectorTest.waitPendingTasks())
+      .then(next);
+  },
+
+  function testStepInto(next) {
+    stepAction = 'stepInto';
+    Protocol.Runtime.evaluate({ expression: 'test()' })
+      .then(() => InspectorTest.waitPendingTasks())
+      .then(next);
+  }
+]);

test/inspector/task-runner.cc

@@ -223,7 +223,11 @@ void ExecuteStringTask::AsyncRun(v8::Isolate* isolate,
              .ToLocal(&script))
       return;
     v8::MaybeLocal<v8::Value> result;
+    if (inspector_)
+      inspector_->willExecuteScript(local_context,
+                                    script->GetUnboundScript()->GetId());
     result = script->Run(local_context);
+    if (inspector_) inspector_->didExecuteScript(local_context);
   } else {
     v8::Local<v8::Module> module;
     if (!v8::ScriptCompiler::CompileModule(isolate, &scriptSource)

test/inspector/task-runner.h

@@ -99,7 +99,7 @@ class AsyncTask : public TaskRunner::Task {
   virtual void AsyncRun(v8::Isolate* isolate,
                         const v8::Global<v8::Context>& context) = 0;
 
- private:
+ protected:
   v8_inspector::V8Inspector* inspector_;
 };