[debugger] Report hit breakpoints when stopping at a debugger statement

Previously when hitting a debugger statement we would ignore reporting
the hit breakpoints.

Bug: chromium:1229541, chromium:1133307
Change-Id: I47427a541391a27fc7783930e5e7eb41fbf2bb6a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3306373Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78145}

src/debug/debug.cc

@@ -568,30 +568,58 @@ MaybeHandle<FixedArray> Debug::CheckBreakPoints(Handle<DebugInfo> debug_info,
 }
 
 bool Debug::IsMutedAtCurrentLocation(JavaScriptFrame* frame) {
-  RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
-  HandleScope scope(isolate_);
   // A break location is considered muted if break locations on the current
   // statement have at least one break point, and all of these break points
   // evaluate to false. Aside from not triggering a debug break event at the
   // break location, we also do not trigger one for debugger statements, nor
   // an exception event on exception at this location.
+  RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
+  HandleScope scope(isolate_);
+  bool has_break_points;
+  MaybeHandle<FixedArray> checked =
+      GetHitBreakpointsAtCurrentStatement(frame, &has_break_points);
+  return has_break_points && checked.is_null();
+}
+
+MaybeHandle<FixedArray> Debug::GetHitBreakpointsAtCurrentStatement(
+    JavaScriptFrame* frame, bool* has_break_points) {
+  RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
   FrameSummary summary = FrameSummary::GetTop(frame);
   Handle<JSFunction> function = summary.AsJavaScript().function();
-  if (!function->shared().HasBreakInfo()) return false;
+  if (!function->shared().HasBreakInfo()) {
+    *has_break_points = false;
+    return {};
+  }
   Handle<DebugInfo> debug_info(function->shared().GetDebugInfo(), isolate_);
   // Enter the debugger.
   DebugScope debug_scope(this);
   std::vector<BreakLocation> break_locations;
   BreakLocation::AllAtCurrentStatement(debug_info, frame, &break_locations);
+
+  Handle<FixedArray> break_points_hit = isolate_->factory()->NewFixedArray(
+      debug_info->GetBreakPointCount(isolate_));
+  int break_points_hit_count = 0;
   bool has_break_points_at_all = false;
   for (size_t i = 0; i < break_locations.size(); i++) {
-    bool has_break_points;
-    MaybeHandle<FixedArray> check_result =
-        CheckBreakPoints(debug_info, &break_locations[i], &has_break_points);
-    has_break_points_at_all |= has_break_points;
-    if (has_break_points && !check_result.is_null()) return false;
+    bool location_has_break_points;
+    MaybeHandle<FixedArray> check_result = CheckBreakPoints(
+        debug_info, &break_locations[i], &location_has_break_points);
+    has_break_points_at_all |= location_has_break_points;
+    if (!check_result.is_null()) {
+      Handle<FixedArray> break_points_current_hit =
+          check_result.ToHandleChecked();
+      int num_objects = break_points_current_hit->length();
+      for (int j = 0; j < num_objects; ++j) {
+        break_points_hit->set(break_points_hit_count++,
+                              break_points_current_hit->get(j));
       }
-  return has_break_points_at_all;
+    }
+  }
+  *has_break_points = has_break_points_at_all;
+  if (break_points_hit_count == 0) return {};
+
+  break_points_hit->Shrink(isolate_, break_points_hit_count);
+  return break_points_hit;
 }
 
 // Check whether a single break point object is triggered.
@@ -825,17 +853,17 @@ void Debug::RemoveBreakpointForWasmScript(Handle<Script> script, int id) {
 
 void Debug::RecordWasmScriptWithBreakpoints(Handle<Script> script) {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
-  if (wasm_scripts_with_breakpoints_.is_null()) {
+  if (wasm_scripts_with_break_points_.is_null()) {
     Handle<WeakArrayList> new_list = isolate_->factory()->NewWeakArrayList(4);
-    wasm_scripts_with_breakpoints_ =
+    wasm_scripts_with_break_points_ =
         isolate_->global_handles()->Create(*new_list);
   }
   {
     DisallowGarbageCollection no_gc;
-    for (int idx = wasm_scripts_with_breakpoints_->length() - 1; idx >= 0;
+    for (int idx = wasm_scripts_with_break_points_->length() - 1; idx >= 0;
          --idx) {
       HeapObject wasm_script;
-      if (wasm_scripts_with_breakpoints_->Get(idx).GetHeapObject(
+      if (wasm_scripts_with_break_points_->Get(idx).GetHeapObject(
               &wasm_script) &&
           wasm_script == *script) {
         return;
@@ -843,11 +871,11 @@ void Debug::RecordWasmScriptWithBreakpoints(Handle<Script> script) {
     }
   }
   Handle<WeakArrayList> new_list = WeakArrayList::Append(
-      isolate_, wasm_scripts_with_breakpoints_, MaybeObjectHandle{script});
-  if (*new_list != *wasm_scripts_with_breakpoints_) {
+      isolate_, wasm_scripts_with_break_points_, MaybeObjectHandle{script});
+  if (*new_list != *wasm_scripts_with_break_points_) {
     isolate_->global_handles()->Destroy(
-        wasm_scripts_with_breakpoints_.location());
-    wasm_scripts_with_breakpoints_ =
+        wasm_scripts_with_break_points_.location());
+    wasm_scripts_with_break_points_ =
         isolate_->global_handles()->Create(*new_list);
   }
 }
@@ -862,12 +890,12 @@ void Debug::ClearAllBreakPoints() {
   });
 #if V8_ENABLE_WEBASSEMBLY
   // Clear all wasm breakpoints.
-  if (!wasm_scripts_with_breakpoints_.is_null()) {
+  if (!wasm_scripts_with_break_points_.is_null()) {
     DisallowGarbageCollection no_gc;
-    for (int idx = wasm_scripts_with_breakpoints_->length() - 1; idx >= 0;
+    for (int idx = wasm_scripts_with_break_points_->length() - 1; idx >= 0;
          --idx) {
       HeapObject raw_wasm_script;
-      if (wasm_scripts_with_breakpoints_->Get(idx).GetHeapObject(
+      if (wasm_scripts_with_break_points_->Get(idx).GetHeapObject(
               &raw_wasm_script)) {
         Script wasm_script = Script::cast(raw_wasm_script);
         WasmScript::ClearAllBreakpoints(wasm_script);
@@ -875,7 +903,7 @@ void Debug::ClearAllBreakPoints() {
             isolate_);
       }
     }
-    wasm_scripts_with_breakpoints_ = Handle<WeakArrayList>{};
+    wasm_scripts_with_break_points_ = Handle<WeakArrayList>{};
   }
 #endif  // V8_ENABLE_WEBASSEMBLY
 }
@@ -2142,12 +2170,10 @@ void Debug::OnDebugBreak(Handle<FixedArray> break_points_hit,
   }
 
   std::vector<int> inspector_break_points_hit;
-  int inspector_break_points_count = 0;
   // This array contains breakpoints installed using JS debug API.
   for (int i = 0; i < break_points_hit->length(); ++i) {
     BreakPoint break_point = BreakPoint::cast(break_points_hit->get(i));
     inspector_break_points_hit.push_back(break_point.id());
-    ++inspector_break_points_count;
   }
   {
     RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebuggerCallback);
@@ -2356,12 +2382,13 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode) {
   StackLimitCheck check(isolate_);
   if (check.HasOverflowed()) return;
 
+  HandleScope scope(isolate_);
+  MaybeHandle<FixedArray> break_points;
   {
     JavaScriptFrameIterator it(isolate_);
     DCHECK(!it.done());
     Object fun = it.frame()->function();
     if (fun.IsJSFunction()) {
-      HandleScope scope(isolate_);
       Handle<JSFunction> function(JSFunction::cast(fun), isolate_);
       // Don't stop in builtin and blackboxed functions.
       Handle<SharedFunctionInfo> shared(function->shared(), isolate_);
@@ -2370,7 +2397,11 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode) {
                               : AllFramesOnStackAreBlackboxed();
       if (ignore_break) return;
       // Don't stop if the break location is muted.
-      if (IsMutedAtCurrentLocation(it.frame())) return;
+      bool has_break_points;
+      break_points =
+          GetHitBreakpointsAtCurrentStatement(it.frame(), &has_break_points);
+      bool is_muted = has_break_points && break_points.is_null();
+      if (is_muted) return;
     }
   }
 
@@ -2379,10 +2410,10 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode) {
   // Clear stepping to avoid duplicate breaks.
   ClearStepping();
 
-  HandleScope scope(isolate_);
   DebugScope debug_scope(this);
-
-  OnDebugBreak(isolate_->factory()->empty_fixed_array(), lastStepAction);
+  OnDebugBreak(break_points.is_null() ? isolate_->factory()->empty_fixed_array()
+                                      : break_points.ToHandleChecked(),
+               lastStepAction);
 }
 
 #ifdef DEBUG

src/debug/debug.h

@@ -448,6 +448,9 @@ class V8_EXPORT_PRIVATE Debug {
   MaybeHandle<FixedArray> CheckBreakPoints(Handle<DebugInfo> debug_info,
                                            BreakLocation* location,
                                            bool* has_break_points = nullptr);
+  MaybeHandle<FixedArray> GetHitBreakpointsAtCurrentStatement(
+      JavaScriptFrame* frame, bool* hasBreakpoints);
+
   bool IsMutedAtCurrentLocation(JavaScriptFrame* frame);
   // Check whether a BreakPoint object is hit. Evaluate condition depending
   // on whether this is a regular break location or a break at function entry.
@@ -555,7 +558,7 @@ class V8_EXPORT_PRIVATE Debug {
 
 #if V8_ENABLE_WEBASSEMBLY
   // This is a global handle, lazily initialized.
-  Handle<WeakArrayList> wasm_scripts_with_breakpoints_;
+  Handle<WeakArrayList> wasm_scripts_with_break_points_;
 #endif  // V8_ENABLE_WEBASSEMBLY
 
   Isolate* isolate_;

test/inspector/debugger/debugger-statement-expected.txt

+Tests that pauses due to debugger statements and breakpoints report the hit breakpoint
+
+Running test: testDebuggerStatement
+Paused with reason other and hit breakpoints: 1:0:0:foo.js.

test/inspector/debugger/debugger-statement.js

+// Copyright 2021 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.
+
+const {session, contextGroup, Protocol} = InspectorTest.start(
+    `Tests that pauses due to debugger statements and breakpoints report the hit breakpoint`);
+
+InspectorTest.runAsyncTestSuite([async function testDebuggerStatement() {
+  await Protocol.Debugger.enable();
+  await Protocol.Runtime.enable();
+  const {result: {scriptId}} = await Protocol.Runtime.compileScript(
+      {expression: `debugger;`, sourceURL: 'foo.js', persistScript: true});
+  await Protocol.Debugger.setBreakpointByUrl({
+    lineNumber: 0,
+    url: 'foo.js',
+  });
+  const runPromise = Protocol.Runtime.runScript({scriptId});
+  let {params: {reason, hitBreakpoints}} = await Protocol.Debugger.oncePaused();
+  InspectorTest.log(
+      `Paused with reason ${reason} and hit breakpoints: ${hitBreakpoints}.`);
+  await Protocol.Debugger.resume();
+  await runPromise;
+
+  await Protocol.Debugger.disable();
+  await Protocol.Runtime.disable();
+}]);