[debug] Fix bug in locals blocklist and refactor scope iterator

This CL shuffles around some code in `ScopeIterator` to better reflect
the two (internal) iteration modes:

  - While "inside" the paused function we iterate based on lexical
    scopes.
  - Once we move past the paused function we iterate based on runtime
    contexts.

This CL renames the advancing functions to `AdvanceScope` and
`AdvanceContext` respectively which operate in the following way:

  - `AdvanceScope` first checks if the current lexical scope requires
    a context. If so, we move one context up the chain, since the next
    lexical scope belongs to that next context. Then we move up one
    lexical scope.

  - `AdvanceContext` moves one context up the context chain. Then we
    we move up through all the lexical scopes until we find the next
    lexical scope that requires a context.

The tricky bit is the transition from scope iteration mode to context
iteration mode. This is where the bug fix comes in. After doing one
standard `AdvanceScope` from the `closure_scope_` to the next
lexical scope, we need to keep moving up through the lexical scope
until we find the next lexical scope that requires a context.

The CL also changes how we collect the locals blocklist. The
locals blocklist is always put on the current context. So every
time we move up one context we reset the locals blocklist and
every time we move up the lexical scope we collect the scope
locals into the blocklist.


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

Fixed: chromium:1354464
Change-Id: I7b37687a8827c20d0660a25413d2c9117b5fe5ba
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3842158
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82615}

src/debug/debug-scopes.cc

@@ -26,7 +26,8 @@ ScopeIterator::ScopeIterator(Isolate* isolate, FrameInspector* frame_inspector,
     : isolate_(isolate),
       frame_inspector_(frame_inspector),
       function_(frame_inspector_->GetFunction()),
-      script_(frame_inspector_->GetScript()) {
+      script_(frame_inspector_->GetScript()),
+      locals_(StringSet::New(isolate)) {
   if (!frame_inspector->GetContext()->IsContext()) {
     // Optimized frame, context or function cannot be materialized. Give up.
     return;
@@ -56,7 +57,9 @@ Handle<Object> ScopeIterator::GetFunctionDebugName() const {
 }
 
 ScopeIterator::ScopeIterator(Isolate* isolate, Handle<JSFunction> function)
-    : isolate_(isolate), context_(function->context(), isolate) {
+    : isolate_(isolate),
+      context_(function->context(), isolate),
+      locals_(StringSet::New(isolate)) {
   if (!function->shared().IsSubjectToDebugging()) {
     context_ = Handle<Context>();
     return;
@@ -71,7 +74,8 @@ ScopeIterator::ScopeIterator(Isolate* isolate,
       generator_(generator),
       function_(generator->function(), isolate),
       context_(generator->context(), isolate),
-      script_(Script::cast(function_->shared().script()), isolate) {
+      script_(Script::cast(function_->shared().script()), isolate),
+      locals_(StringSet::New(isolate)) {
   CHECK(function_->shared().IsSubjectToDebugging());
   TryParseAndRetrieveScopes(ReparseStrategy::kFunctionLiteral);
 }
@@ -389,36 +393,47 @@ bool ScopeIterator::NeedsAndHasContext() const {
            function_->context() == *context_);
 }
 
-void ScopeIterator::AdvanceOneScope() {
-  if (NeedsAndHasContext()) {
-    DCHECK(!context_->previous().is_null());
-    context_ = handle(context_->previous(), isolate_);
-  }
-  DCHECK(current_scope_->outer_scope() != nullptr);
+bool ScopeIterator::AdvanceOneScope() {
+  if (!current_scope_ || !current_scope_->outer_scope()) return false;
+
   current_scope_ = current_scope_->outer_scope();
+  CollectLocalsFromCurrentScope();
+  return true;
+}
+
+void ScopeIterator::AdvanceOneContext() {
+  DCHECK(!context_->IsNativeContext());
+  DCHECK(!context_->previous().is_null());
+  context_ = handle(context_->previous(), isolate_);
+
+  // The locals blocklist is always associated with a context. So when we
+  // move one context up, we also reset the locals_ blocklist.
+  locals_ = StringSet::New(isolate_);
 }
 
-void ScopeIterator::AdvanceToNonHiddenScope() {
+void ScopeIterator::AdvanceScope() {
+  DCHECK(InInnerScope());
+
   do {
-    AdvanceOneScope();
+    if (NeedsAndHasContext()) {
+      // current_scope_ needs a context so moving one scope up requires us to
+      // also move up one context.
+      AdvanceOneContext();
+    }
+
+    CHECK(AdvanceOneScope());
   } while (current_scope_->is_hidden());
 }
 
 void ScopeIterator::AdvanceContext() {
-  DCHECK(!context_->IsNativeContext());
-  context_ = handle(context_->previous(), isolate_);
+  AdvanceOneContext();
 
   // While advancing one context, we need to advance at least one
   // scope, but until we hit the next scope that actually requires
   // a context. All the locals collected along the way build the
   // blocklist for debug-evaluate for this context.
-  locals_ = StringSet::New(isolate_);
-  do {
-    if (!current_scope_ || !current_scope_->outer_scope()) break;
-
-    current_scope_ = current_scope_->outer_scope();
-    CollectLocalsFromCurrentScope();
-  } while (!NeedsAndHasContext());
+  while (AdvanceOneScope() && !NeedsAndHasContext()) {
+  }
 }
 
 void ScopeIterator::Next() {
@@ -447,14 +462,16 @@ void ScopeIterator::Next() {
     AdvanceContext();
   } else {
     DCHECK_NOT_NULL(current_scope_);
-    AdvanceToNonHiddenScope();
+    AdvanceScope();
 
     if (leaving_closure) {
       DCHECK(current_scope_ != closure_scope_);
-      // Edge case when we just go past {closure_scope_}. This case
-      // already needs to start collecting locals for the blocklist.
-      locals_ = StringSet::New(isolate_);
-      CollectLocalsFromCurrentScope();
+      // If the current_scope_ doesn't need a context, we advance the scopes
+      // and collect the blocklist along the way until we find the scope
+      // that should match `context_`.
+      // But only do this if we have complete scope information.
+      while (!NeedsAndHasContext() && AdvanceOneScope()) {
+      }
     }
   }
 

src/debug/debug-scopes.h

@@ -130,8 +130,9 @@ class ScopeIterator {
     return frame_inspector_->javascript_frame();
   }
 
-  void AdvanceOneScope();
-  void AdvanceToNonHiddenScope();
+  bool AdvanceOneScope();
+  void AdvanceOneContext();
+  void AdvanceScope();
   void AdvanceContext();
   void CollectLocalsFromCurrentScope();
 

test/debugger/debug/debug-evaluate-shadowed-context-4.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.
+
+// Test that debug-evaluate properly shadows stack-allocated variables.
+
+Debug = debug.Debug
+
+let exception = null;
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    assertThrows(() => exec_state.frame(0).evaluate("a").value());
+  } catch (e) {
+    exception = e;
+    print(e + e.stack);
+  }
+}
+
+Debug.setListener(listener);
+
+(function f() {
+  let a = 1;
+  () => a;  // a is context-allocated
+  return function g() {
+    let a = 2;  // a is stack-allocated
+    {
+      let b = 3;
+      return function h() {
+        debugger;
+      }
+    }
+  }
+})()()();
+
+Debug.setListener(null);
+assertNull(exception);