[inspector] V8DebuggerAgent cleanup

src/debug/debug.cc

 // Copyright 2012 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.
 
 #include "src/debug/debug.h"
 
 #include <memory>
 
 #include "src/api.h"
 #include "src/arguments.h"
@@ skipping 1009 matching lines @@
   Handle<JSFunction> function(summary.function());
   Handle<SharedFunctionInfo> shared(function->shared());
   if (!EnsureDebugInfo(shared, function)) {
     // Return if ensuring debug info failed.
     return;
   }
 
   Handle<DebugInfo> debug_info(shared->GetDebugInfo());
   BreakLocation location = BreakLocation::FromFrame(debug_info, js_frame);
 
+  // It is used at least in two cases:
+  // - step-next at return position from first callback in microtask will break
+  // in next callback,
+  // - step-next at return position when caller frame is blackboxed will break
+  // in next not blackboxed function called by caller frame if any.
+  if (step_action == StepNext && location.IsReturn()) {

[kozy, 2017/02/01 07:29:43]

Yang, WDYT about this part? This code is just moved from V8DebuggerAgentImpl but
it doesn't sound right to me.

[Yang, 2017/02/01 07:46:12]

for the microtask case, we should make a check at the microtask runner, where we
would turn pending step-next into a step-in.

for the blackbox case, it's arguable whether step-next should take us into the
next call that the blackboxed caller makes. That could be accomplished by
clicking on step-in. The current behavior assumes that a step-next is actually a
step-next to the caller, and then a step-in into the next non-blackboxed call.

[kozy, 2017/02/02 06:58:56]

I think that case with blackboxed code could be considered in the same way as
microtask case. Why we'd like to support special step-next behavior in one case
and wouldn't in another?
Async step into will enable step-in target chained callback without iterating
through all other callbacks.

Maybe we could just remove this code and suggest users to use step-in when they
actually would like to go to next chained callback or not blackboxed function. I
prefer this.

[Yang, 2017/02/02 12:10:49]

You are right. Step-in at the return position to indicate that the user wants to
step into the next callback in queue or the next call from blackboxed caller
makes sense.

+    thread_local_.last_step_action_ = StepIn;
+    UpdateHookOnFunctionCall();
+  }
   // Any step at a return is a step-out.
   if (location.IsReturn()) step_action = StepOut;
   // A step-next at a tail call is a step-out.
   if (location.IsTailCall() && step_action == StepNext) step_action = StepOut;
   // A step-next in blackboxed function is a step-out.
   if (step_action == StepNext && IsBlackboxed(shared)) step_action = StepOut;
 
   thread_local_.last_statement_position_ =
       summary.abstract_code()->SourceStatementPosition(summary.code_offset());
   thread_local_.last_fp_ = frame->UnpaddedFP();
@@ skipping 773 matching lines @@
       return;
     }
   }
 
   DebugScope debug_scope(this);
   if (debug_scope.failed()) return;
 
   if (debug_delegate_) {
     HandleScope scope(isolate_);
 
+    {
+      // Exception from JavaScript should always contains at least one frame.
+      JavaScriptFrameIterator it(isolate_);
+      DCHECK(!it.done());
+      if (it.done()) return;

[kozy, 2017/02/01 07:29:43]

Yang, do you know when we can have empty stack here?

[Yang, 2017/02/01 07:46:12]

might be possible, e.g. if we call directly into a function, that is implemented
as C++ built-in, and that throws?

lets move this check to the previous JavaScriptFrameIterator.

[kozy, 2017/02/02 06:58:55]

Done.

+    }
+
     // Create the execution state.
     Handle<Object> exec_state;
     // Bail out and don't call debugger if exception.
     if (!MakeExecutionState().ToHandle(&exec_state)) return;
 
     debug_delegate_->ExceptionThrown(
         GetDebugEventContext(isolate_),
         v8::Utils::ToLocal(Handle<JSObject>::cast(exec_state)),
         v8::Utils::ToLocal(exception), promise->IsJSObject(), uncaught);
     if (!non_inspector_listener_exists()) return;
@@ skipping 595 matching lines @@
   return v8::Utils::ToLocal(callback_data_);
 }
 
 
 v8::Isolate* EventDetailsImpl::GetIsolate() const {
   return reinterpret_cast<v8::Isolate*>(exec_state_->GetIsolate());
 }
 
 }  // namespace internal
 }  // namespace v8