[debug] Fix crash in debug scope search

This CL fixes a crash when we build the scope chain after re-parsing
for Debugger.evaluateOnCallFrame.

The following script causes the crash:

class A {
  test(){
    debugger;
  }
  f = (x) => {}
}
let a = new A()
a.test()

The current scope search tries to be smart and descends deeper
into the scope tree based on source position. That is not a sound
approach as V8 doesn't guarantee that sibling scopes don't overlap.

In the above case V8 creates an instance initializer scope where
f is assigned (and the initializer scope is the parent scope for
the arrow function). The problem is that the initializer scope
uses the same source range as the class `A` itself, so when we
look for the scope for `test`, we descend wrongly into the
initializer scope and can't recover.

The solution is to not try and be too smart:
  - First, find the closure scope with a straight-up DFS.
  - Once we have that, descend from there and try to find the
    closest fitting scope around the break position.

R=bmeurer@chromium.org, jarin@chromium.org

Bug: chromium:1348186
Change-Id: Ic5e20c4d12b3d768f76a17367dc0f87bcc73763b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3807594Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82216}

src/debug/debug-scopes.cc

@@ -154,34 +154,49 @@ class ScopeChainRetriever {
   }
 
   void RetrieveScopeChain() {
-    Scope* parent = nullptr;
-    Scope* current = scope_;
-    SetClosureScopeIfFound(current);
-
-    while (parent != current) {
-      parent = current;
-      for (Scope* inner_scope = current->inner_scope(); inner_scope != nullptr;
-           inner_scope = inner_scope->sibling()) {
-        if (SetClosureScopeIfFound(inner_scope) ||
-            ContainsPosition(inner_scope)) {
-          current = inner_scope;
-          break;
-        }
-      }
-    }
-    start_scope_ = current;
+    // 1. Find the closure scope with a DFS.
+    RetrieveClosureScope(scope_);
+    DCHECK_NOT_NULL(closure_scope_);
+
+    // 2. Starting from the closure scope search inwards. Given that V8's scope
+    //    tree doesn't guarantee that siblings don't overlap, we look at all
+    //    scopes and pick the one with the tightest bounds around `position_`.
+    start_scope_ = closure_scope_;
+    RetrieveStartScope(closure_scope_);
   }
 
-  bool SetClosureScopeIfFound(Scope* scope) {
-    const int start = scope->start_position();
-    const int end = scope->end_position();
-    if (start == break_scope_start_ && end == break_scope_end_) {
+  bool RetrieveClosureScope(Scope* scope) {
+    if (break_scope_start_ == scope->start_position() &&
+        break_scope_end_ == scope->end_position()) {
       closure_scope_ = scope->AsDeclarationScope();
       return true;
     }
+
+    for (Scope* inner_scope = scope->inner_scope(); inner_scope != nullptr;
+         inner_scope = inner_scope->sibling()) {
+      if (RetrieveClosureScope(inner_scope)) return true;
+    }
     return false;
   }
 
+  void RetrieveStartScope(Scope* scope) {
+    const int start = scope->start_position();
+    const int end = scope->end_position();
+
+    // Update start_scope_ if scope contains `position_` and scope is a tighter
+    // fit than the currently set start_scope_.
+    // Generators have the same source position so we also check for equality.
+    if (ContainsPosition(scope) && start >= start_scope_->start_position() &&
+        end <= start_scope_->end_position()) {
+      start_scope_ = scope;
+    }
+
+    for (Scope* inner_scope = scope->inner_scope(); inner_scope != nullptr;
+         inner_scope = inner_scope->sibling()) {
+      RetrieveStartScope(inner_scope);
+    }
+  }
+
   bool ContainsPosition(Scope* scope) {
     const int start = scope->start_position();
     const int end = scope->end_position();

test/inspector/debugger/evaluate-on-callframe-this-expected.txt

+Don't crash while paused in a class method and evaluating 'this'
+{
+    className : A
+    description : A
+    objectId : <objectId>
+    type : object
+}

test/inspector/debugger/evaluate-on-callframe-this.js

+// Copyright 2022 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(`Don't crash while paused in a class method and evaluating 'this'`);
+
+(async () => {
+  await Protocol.Debugger.enable();
+  contextGroup.addScript(`
+  class A {
+    test() {
+      debugger;
+    }
+    f = (x) => {}
+  }
+  const a = new A();
+  a.test();
+  `, 0, 0, 'test.js');
+
+  Protocol.Debugger.enable();
+  Protocol.Runtime.evaluate({expression: 'run()'});
+
+  const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+  const frame = callFrames[0];
+
+  const { result: { result } } =
+    await Protocol.Debugger.evaluateOnCallFrame({ callFrameId: frame.callFrameId, expression: 'this' });
+  InspectorTest.logMessage(result);
+
+  InspectorTest.completeTest();
+})();