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

This is a reland of commit 3297ccca

This is a straight-up reland of the original CL. The failing test
was flaky and removed with https://crrev.com/c/3868727. We replaced
the test with a proper DevTools e2e test: https://crrev.com/c/3867522

Original change's description:
> [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/+/3854501
> Reviewed-by: Jakob Linke <jgruber@chromium.org>
> Commit-Queue: Simon Zünd <szuend@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#82817}

Change-Id: I1938ccb5979fd80dff530b2ffe3f18714b7eff3f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3867727
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82901}

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;