[debug] Don't crash when breaking on entry to functions with heap vars

Any function with heap-allocated variables starts by creating and
pushing a new context for its execution. When entering the debugger due
to the stack check in the beginning of InterpreterEntryTrampoline, the
function has not yet had a chance to push that new context. The code in
ScopeIterator currently assumes that any function which needs a context
already has one by the time the debugger attempts to iterate scopes, but
in this case that assumption is invalid, which can cause a null deref.

This change introduces a new function ScopeIterator::NeedsAndHasContext
to replace previous calls to current_scope_->NeedsContext(). This new
function checks for the case where the current scope matches the closure
scope but the context matches the containing context for the function,
which implies that the function has not yet pushed its own context.

Bug: v8:10319, chromium:1038747
Change-Id: I29636f269c44d35b68d8446769d17170eed50e89
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2168021
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67519}

src/debug/debug-scopes.cc

@@ -291,6 +291,9 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
     if (ignore_nested_scopes) {
       current_scope_ = closure_scope_;
       start_scope_ = current_scope_;
+      // ignore_nested_scopes is only used for the return-position breakpoint,
+      // so we can safely assume that the closure context for the current
+      // function exists if it needs one.
       if (closure_scope_->NeedsContext()) {
         context_ = handle(context_->closure_context(), isolate_);
       }
@@ -312,7 +315,7 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
 }
 
 void ScopeIterator::UnwrapEvaluationContext() {
-  if (context_->is_null() || !context_->IsDebugEvaluateContext()) return;
+  if (!context_->IsDebugEvaluateContext()) return;
   Context current = *context_;
   do {
     Object wrapped = current.get(Context::WRAPPED_CONTEXT_INDEX);
@@ -381,11 +384,25 @@ bool ScopeIterator::DeclaresLocals(Mode mode) const {
 }
 
 bool ScopeIterator::HasContext() const {
-  return !InInnerScope() || current_scope_->NeedsContext();
+  return !InInnerScope() || NeedsAndHasContext();
+}
+
+bool ScopeIterator::NeedsAndHasContext() const {
+  if (!current_scope_->NeedsContext()) return false;
+  // Generally, if a scope needs a context, then we can assume that it has a
+  // context. However, the stack check during function entry happens before the
+  // function has a chance to create and push its own context, so we must check
+  // for the case where the function is executing in its parent context. This
+  // case is only possible in function scopes; top-level code (modules and
+  // non-module scripts) begin execution in the context they need and don't have
+  // a separate step to push the correct context.
+  return !(current_scope_ == closure_scope_ &&
+           current_scope_->is_function_scope() && !function_.is_null() &&
+           function_->context() == *context_);
 }
 
 void ScopeIterator::AdvanceOneScope() {
-  if (current_scope_->NeedsContext()) {
+  if (NeedsAndHasContext()) {
     DCHECK(!context_->previous().is_null());
     context_ = handle(context_->previous(), isolate_);
   }
@@ -413,7 +430,7 @@ void ScopeIterator::AdvanceContext() {
 
     current_scope_ = current_scope_->outer_scope();
     CollectLocalsFromCurrentScope();
-  } while (!current_scope_->NeedsContext());
+  } while (!NeedsAndHasContext());
 }
 
 void ScopeIterator::Next() {
@@ -429,22 +446,23 @@ void ScopeIterator::Next() {
     return;
   }
 
-  bool inner = InInnerScope();
-  if (current_scope_ == closure_scope_) function_ = Handle<JSFunction>();
+  bool leaving_closure = current_scope_ == closure_scope_;
 
   if (scope_type == ScopeTypeScript) {
-    DCHECK_IMPLIES(InInnerScope(), current_scope_->is_script_scope());
+    DCHECK_IMPLIES(InInnerScope() && !leaving_closure,
+                   current_scope_->is_script_scope());
     seen_script_scope_ = true;
     if (context_->IsScriptContext()) {
       context_ = handle(context_->previous(), isolate_);
     }
-  } else if (!inner) {
+  } else if (!InInnerScope()) {
     AdvanceContext();
   } else {
     DCHECK_NOT_NULL(current_scope_);
     AdvanceToNonHiddenScope();
 
-    if (!InInnerScope() && current_scope_ != closure_scope_) {
+    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 blacklist.
       locals_ = StringSet::New(isolate_);
@@ -452,6 +470,8 @@ void ScopeIterator::Next() {
     }
   }
 
+  if (leaving_closure) function_ = Handle<JSFunction>();
+
   UnwrapEvaluationContext();
 }
 
@@ -461,34 +481,29 @@ ScopeIterator::ScopeType ScopeIterator::Type() const {
   if (InInnerScope()) {
     switch (current_scope_->scope_type()) {
       case FUNCTION_SCOPE:
-        DCHECK_IMPLIES(current_scope_->NeedsContext(),
+        DCHECK_IMPLIES(NeedsAndHasContext(),
                        context_->IsFunctionContext() ||
                            context_->IsDebugEvaluateContext());
         return ScopeTypeLocal;
       case MODULE_SCOPE:
-        DCHECK_IMPLIES(current_scope_->NeedsContext(),
-                       context_->IsModuleContext());
+        DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsModuleContext());
         return ScopeTypeModule;
       case SCRIPT_SCOPE:
-        DCHECK_IMPLIES(
-            current_scope_->NeedsContext(),
-            context_->IsScriptContext() || context_->IsNativeContext());
+        DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsScriptContext() ||
+                                                 context_->IsNativeContext());
         return ScopeTypeScript;
       case WITH_SCOPE:
-        DCHECK_IMPLIES(current_scope_->NeedsContext(),
-                       context_->IsWithContext());
+        DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsWithContext());
         return ScopeTypeWith;
       case CATCH_SCOPE:
         DCHECK(context_->IsCatchContext());
         return ScopeTypeCatch;
       case BLOCK_SCOPE:
       case CLASS_SCOPE:
-        DCHECK_IMPLIES(current_scope_->NeedsContext(),
-                       context_->IsBlockContext());
+        DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsBlockContext());
         return ScopeTypeBlock;
       case EVAL_SCOPE:
-        DCHECK_IMPLIES(current_scope_->NeedsContext(),
-                       context_->IsEvalContext());
+        DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsEvalContext());
         return ScopeTypeEval;
     }
     UNREACHABLE();
@@ -590,7 +605,7 @@ bool ScopeIterator::SetVariableValue(Handle<String> name,
         DCHECK_EQ(ScopeTypeLocal, Type());
         if (SetLocalVariableValue(name, value)) return true;
         // There may not be an associated context since we're InInnerScope().
-        if (!current_scope_->NeedsContext()) return false;
+        if (!NeedsAndHasContext()) return false;
       } else {
         DCHECK_EQ(ScopeTypeClosure, Type());
         if (SetContextVariableValue(name, value)) return true;
@@ -634,7 +649,7 @@ void ScopeIterator::DebugPrint() {
 
     case ScopeIterator::ScopeTypeLocal: {
       os << "Local:\n";
-      if (current_scope_->NeedsContext()) {
+      if (NeedsAndHasContext()) {
         context_->Print(os);
         if (context_->has_extension()) {
           Handle<HeapObject> extension(context_->extension(), isolate_);

src/debug/debug-scopes.h

@@ -102,6 +102,7 @@ class ScopeIterator {
 
   bool InInnerScope() const { return !function_.is_null(); }
   bool HasContext() const;
+  bool NeedsAndHasContext() const;
   Handle<Context> CurrentContext() const {
     DCHECK(HasContext());
     return context_;
@@ -112,7 +113,11 @@ class ScopeIterator {
   std::unique_ptr<ParseInfo> info_;
   FrameInspector* const frame_inspector_ = nullptr;
   Handle<JSGeneratorObject> generator_;
+
+  // The currently-executing function from the inspected frame, or null if this
+  // ScopeIterator has already iterated to any Scope outside that function.
   Handle<JSFunction> function_;
+
   Handle<Context> context_;
   Handle<Script> script_;
   Handle<StringSet> locals_;

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

+// Copyright 2020 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.
+
+var Debug = debug.Debug;
+
+var frame;
+
+Debug.setListener(function (event, exec_state, event_data, data) {
+  if (event == Debug.DebugEvent.Break) {
+    frame = exec_state.frame(0);
+
+    // Try changing the value, which hasn't yet been initialized.
+    assertEquals(3, frame.evaluate("result = 3").value());
+    assertEquals(3, frame.evaluate("result").value());
+  }
+});
+
+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);
+
+  var result = 0;
+
+  // Regardless of how `result` is allocated, it should now be initialized.
+  assertEquals(0, result);
+
+  // Close over `result` to cause it to be heap-allocated.
+  return () => ++result;
+}
+
+// Break on entry to a function which includes heap-allocated variables.
+%ScheduleBreak();
+makeCounter();
+
+// Check the frame state which was collected during the breakpoint.
+assertEquals(1, frame.localCount());
+assertEquals('result', frame.localName(0));
+assertEquals(undefined, frame.localValue(0).value());
+assertEquals(3, frame.scopeCount());
+assertEquals(debug.ScopeType.Local, frame.scope(0).scopeType());
+assertEquals(debug.ScopeType.Script, frame.scope(1).scopeType());
+assertEquals(debug.ScopeType.Global, frame.scope(2).scopeType());