[debug] Immediately step-in for 'stack check triggered' debug breaks

This CL changes debug breaks that are triggered via interrupts (i.e.
via stack check). One client of this behavior is the `Debugger.pause`
CDP method.

The problem is that when we pause so early, the JSFunction didn't have
time yet to create and push it's context. This requires special
handling in the ScopeIterator and makes an upcoming change unnecessary
complex.

Another (minor) problem is that local debug-evaluate can't change
context-allocated local variables (see changed regression bug). Since
the context is not yet created and pushed, variables are written to
the DebugEvaluateContext that goes away after the evaluation.

The solution is to mirror what `BreakOnNextFunction` does. Instead
of staying paused in the middle of the function entry, we trigger
a "step in" and pause at the first valid breakable position instead.
This ensures that the function context is already created and pushed.

Note that we do this only in case for JSFunctions. In all other cases
we keep the existing behavior and stay paused in the entry.

R=jgruber@chromium.org

Fixed: chromium:1246907
Change-Id: I0cd8ae6e049a3b55bdd44858e769682a1ca47064
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3854501Reviewed-by: Jakob Linke <jgruber@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82817}

src/debug/debug.cc

@@ -387,6 +387,7 @@ void Debug::ThreadInit() {
   base::Relaxed_Store(&thread_local_.current_debug_scope_,
                       static_cast<base::AtomicWord>(0));
   thread_local_.break_on_next_function_call_ = false;
+  thread_local_.scheduled_break_on_next_function_call_ = false;
   UpdateHookOnFunctionCall();
   thread_local_.promise_stack_ = Smi::zero();
 }
@@ -513,15 +514,22 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
   bool has_break_points;
   MaybeHandle<FixedArray> break_points_hit =
       CheckBreakPoints(debug_info, &location, &has_break_points);
-  if (!break_points_hit.is_null() || break_on_next_function_call()) {
+  if (!break_points_hit.is_null() || break_on_next_function_call() ||
+      scheduled_break_on_function_call()) {
     StepAction lastStepAction = last_step_action();
+    DCHECK_IMPLIES(scheduled_break_on_function_call(),
+                   lastStepAction == StepNone);
+    debug::BreakReasons break_reasons;
+    if (scheduled_break_on_function_call()) {
+      break_reasons.Add(debug::BreakReason::kScheduled);
+    }
     // Clear all current stepping setup.
     ClearStepping();
     // Notify the debug event listeners.
     OnDebugBreak(!break_points_hit.is_null()
                      ? break_points_hit.ToHandleChecked()
                      : isolate_->factory()->empty_fixed_array(),
-                 lastStepAction);
+                 lastStepAction, break_reasons);
     return;
   }
 
@@ -1068,7 +1076,8 @@ void Debug::ClearBreakOnNextFunctionCall() {
 
 void Debug::PrepareStepIn(Handle<JSFunction> function) {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
-  CHECK(last_step_action() >= StepInto || break_on_next_function_call());
+  CHECK(last_step_action() >= StepInto || break_on_next_function_call() ||
+        scheduled_break_on_function_call());
   if (ignore_events()) return;
   if (in_debug_scope()) return;
   if (break_disabled()) return;
@@ -1378,6 +1387,7 @@ void Debug::ClearStepping() {
   thread_local_.last_frame_count_ = -1;
   thread_local_.target_frame_count_ = -1;
   thread_local_.break_on_next_function_call_ = false;
+  thread_local_.scheduled_break_on_next_function_call_ = false;
   clear_restart_frame();
   UpdateHookOnFunctionCall();
 }
@@ -2494,13 +2504,28 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode,
   HandleScope scope(isolate_);
   MaybeHandle<FixedArray> break_points;
   {
-    JavaScriptFrameIterator it(isolate_);
+    StackTraceFrameIterator it(isolate_);
     DCHECK(!it.done());
-    Object fun = it.frame()->function();
-    if (fun.IsJSFunction()) {
-      Handle<JSFunction> function(JSFunction::cast(fun), isolate_);
-      // Don't stop in builtin and blackboxed functions.
+    JavaScriptFrame* frame = it.frame()->is_java_script()
+                                 ? JavaScriptFrame::cast(it.frame())
+                                 : nullptr;
+    if (frame && frame->function().IsJSFunction()) {
+      Handle<JSFunction> function(frame->function(), isolate_);
       Handle<SharedFunctionInfo> shared(function->shared(), isolate_);
+
+      // kScheduled breaks are triggered by the stack check. While we could
+      // pause here, the JSFunction didn't have time yet to create and push
+      // it's context. Instead, we step into the function and pause at the
+      // first official breakable position.
+      // This behavior mirrors "BreakOnNextFunctionCall".
+      if (break_reasons.contains(v8::debug::BreakReason::kScheduled)) {
+        CHECK_EQ(last_step_action(), StepAction::StepNone);
+        thread_local_.scheduled_break_on_next_function_call_ = true;
+        PrepareStepIn(function);
+        return;
+      }
+
+      // Don't stop in builtin and blackboxed functions.
       bool ignore_break = ignore_break_mode == kIgnoreIfTopFrameBlackboxed
                               ? IsBlackboxed(shared)
                               : AllFramesOnStackAreBlackboxed();
@@ -2512,7 +2537,7 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode,
         DebugScope debug_scope(this);
 
         std::vector<BreakLocation> break_locations;
-        BreakLocation::AllAtCurrentStatement(debug_info, it.frame(),
+        BreakLocation::AllAtCurrentStatement(debug_info, frame,
                                              &break_locations);
 
         for (size_t i = 0; i < break_locations.size(); i++) {

src/debug/debug.h

@@ -395,6 +395,10 @@ class V8_EXPORT_PRIVATE Debug {
     return thread_local_.break_on_next_function_call_;
   }
 
+  bool scheduled_break_on_function_call() const {
+    return thread_local_.scheduled_break_on_next_function_call_;
+  }
+
   bool IsRestartFrameScheduled() const {
     return thread_local_.restart_frame_id_ != StackFrameId::NO_ID;
   }
@@ -584,6 +588,11 @@ class V8_EXPORT_PRIVATE Debug {
     // debugger to break on next function call.
     bool break_on_next_function_call_;
 
+    // This flag is true when we break via stack check (BreakReason::kScheduled)
+    // We don't stay paused there but instead "step in" to the function similar
+    // to what "BreakOnNextFunctionCall" does.
+    bool scheduled_break_on_next_function_call_;
+
     // Throwing an exception may cause a Promise rejection.  For this purpose
     // we keep track of a stack of nested promises.
     Object promise_stack_;

test/debugger/debug/debug-stack-check-position.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Check that even though we schedule and break on function entry, we still
+// pause on the first breakable position.
 
 var Debug = debug.Debug;
 var exception = null;
@@ -16,8 +18,8 @@ function listener(event, exec_state, event_data, data) {
   }
 }
 
-function f() {     // BREAK
-  return 1;
+function f() {
+  return 1;        // BREAK
 }
 
 Debug.setListener(listener);

test/debugger/debug/regress/regress-10319.js

@@ -17,11 +17,10 @@ Debug.setListener(function (event, exec_state, event_data, data) {
 });
 
 function makeCounter() {
-  // If the variable `result` were stack-allocated, it would be 3 at this point
-  // due to the debugging activity during function entry. However, for a
-  // heap-allocated variable, the debugger evaluated `result = 3` in a temporary
-  // scope instead and had no effect on this variable.
-  assertEquals(undefined, result);
+  // The debug-evaluate should be able to set this to 3. This was fixed with
+  // https://crbug.com/1246907 which delays debug breaks triggered by stack
+  // checks until the function context is properly set up.
+  assertEquals(3, result);
 
   var result = 0;