[debugger] materialize scope values in TDZ as undefined.

R=szuend@chromium.org

Fixes: chromium:718827
Change-Id: I261ce2cf692b5bcf88f4f7f67249ec49c837de4e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2241521
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68337}

src/debug/debug-scopes.cc

@@ -373,7 +373,8 @@ bool ScopeIterator::DeclaresLocals(Mode mode) const {
   if (type == ScopeTypeGlobal) return mode == Mode::ALL;
 
   bool declares_local = false;
-  auto visitor = [&](Handle<String> name, Handle<Object> value) {
+  auto visitor = [&](Handle<String> name, Handle<Object> value,
+                     ScopeType scope_type) {
     declares_local = true;
     return true;
   };
@@ -546,7 +547,18 @@ Handle<JSObject> ScopeIterator::ScopeObject(Mode mode) {
   }
 
   Handle<JSObject> scope = isolate_->factory()->NewJSObjectWithNullProto();
-  auto visitor = [=](Handle<String> name, Handle<Object> value) {
+  auto visitor = [=](Handle<String> name, Handle<Object> value,
+                     ScopeType scope_type) {
+    if (value->IsTheHole(isolate_)) {
+      // Reflect variables under TDZ as undefined in scope object.
+      if (scope_type == ScopeTypeScript &&
+          JSReceiver::HasOwnProperty(scope, name).FromMaybe(true)) {
+        // We also use the hole to represent overridden let-declarations via
+        // REPL mode in a script context. Catch this case.
+        return false;
+      }
+      value = isolate_->factory()->undefined_value();
+    }
     JSObject::AddProperty(isolate_, scope, name, value, NONE);
     return false;
   };
@@ -562,10 +574,10 @@ void ScopeIterator::VisitScope(const Visitor& visitor, Mode mode) const {
     case ScopeTypeCatch:
     case ScopeTypeBlock:
     case ScopeTypeEval:
-      return VisitLocalScope(visitor, mode);
+      return VisitLocalScope(visitor, mode, Type());
     case ScopeTypeModule:
       if (InInnerScope()) {
-        return VisitLocalScope(visitor, mode);
+        return VisitLocalScope(visitor, mode, Type());
       }
       DCHECK_EQ(Mode::ALL, mode);
       return VisitModuleScope(visitor);
@@ -714,7 +726,8 @@ void ScopeIterator::VisitScriptScope(const Visitor& visitor) const {
     Handle<Context> context = ScriptContextTable::GetContext(
         isolate_, script_contexts, context_index);
     Handle<ScopeInfo> scope_info(context->scope_info(), isolate_);
-    if (VisitContextLocals(visitor, scope_info, context)) return;
+    if (VisitContextLocals(visitor, scope_info, context, ScopeTypeScript))
+      return;
   }
 }
 
@@ -722,7 +735,8 @@ void ScopeIterator::VisitModuleScope(const Visitor& visitor) const {
   DCHECK(context_->IsModuleContext());
 
   Handle<ScopeInfo> scope_info(context_->scope_info(), isolate_);
-  if (VisitContextLocals(visitor, scope_info, context_)) return;
+  if (VisitContextLocals(visitor, scope_info, context_, ScopeTypeModule))
+    return;
 
   int count_index = scope_info->ModuleVariableCountIndex();
   int module_variable_count = Smi::cast(scope_info->get(count_index)).value();
@@ -741,29 +755,27 @@ void ScopeIterator::VisitModuleScope(const Visitor& visitor) const {
     Handle<Object> value =
         SourceTextModule::LoadVariable(isolate_, module, index);
 
-    // Reflect variables under TDZ as undeclared in scope object.
-    if (value->IsTheHole(isolate_)) continue;
-    if (visitor(name, value)) return;
+    if (visitor(name, value, ScopeTypeModule)) return;
   }
 }
 
 bool ScopeIterator::VisitContextLocals(const Visitor& visitor,
                                        Handle<ScopeInfo> scope_info,
-                                       Handle<Context> context) const {
+                                       Handle<Context> context,
+                                       ScopeType scope_type) const {
   // Fill all context locals to the context extension.
   for (int i = 0; i < scope_info->ContextLocalCount(); ++i) {
     Handle<String> name(scope_info->ContextLocalName(i), isolate_);
     if (ScopeInfo::VariableIsSynthetic(*name)) continue;
     int context_index = scope_info->ContextHeaderLength() + i;
     Handle<Object> value(context->get(context_index), isolate_);
-    // Reflect variables under TDZ as undefined in scope object.
-    if (value->IsTheHole(isolate_)) continue;
-    if (visitor(name, value)) return true;
+    if (visitor(name, value, scope_type)) return true;
   }
   return false;
 }
 
-bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
+bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode,
+                                ScopeType scope_type) const {
   if (mode == Mode::STACK && current_scope_->is_declaration_scope() &&
       current_scope_->AsDeclarationScope()->has_this_declaration()) {
     // TODO(bmeurer): We should refactor the general variable lookup
@@ -776,10 +788,11 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
             : frame_inspector_ == nullptr
                   ? handle(generator_->receiver(), isolate_)
                   : frame_inspector_->GetReceiver();
-    if (receiver->IsOptimizedOut(isolate_) || receiver->IsTheHole(isolate_)) {
+    if (receiver->IsOptimizedOut(isolate_)) {
       receiver = isolate_->factory()->undefined_value();
     }
-    if (visitor(isolate_->factory()->this_string(), receiver)) return true;
+    if (visitor(isolate_->factory()->this_string(), receiver, scope_type))
+      return true;
   }
 
   if (current_scope_->is_function_scope()) {
@@ -790,7 +803,7 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
                                         ? function_
                                         : frame_inspector_->GetFunction();
       Handle<String> name = function_var->name();
-      if (visitor(name, function)) return true;
+      if (visitor(name, function, scope_type)) return true;
     }
   }
 
@@ -839,9 +852,6 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
           index += parameter_count;
           DCHECK_LT(index, parameters_and_registers.length());
           value = handle(parameters_and_registers.get(index), isolate_);
-          if (value->IsTheHole(isolate_)) {
-            value = isolate_->factory()->undefined_value();
-          }
         } else {
           value = frame_inspector_->GetExpression(index);
           if (value->IsOptimizedOut(isolate_)) {
@@ -851,9 +861,6 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
               continue;
             }
             value = isolate_->factory()->undefined_value();
-          } else if (value->IsTheHole(isolate_)) {
-            // Reflect variables under TDZ as undeclared in scope object.
-            continue;
           }
         }
         break;
@@ -862,8 +869,6 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
         if (mode == Mode::STACK) continue;
         DCHECK(var->IsContextSlot());
         value = handle(context_->get(index), isolate_);
-        // Reflect variables under TDZ as undeclared in scope object.
-        if (value->IsTheHole(isolate_)) continue;
         break;
 
       case VariableLocation::MODULE: {
@@ -871,13 +876,11 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode) const {
         // if (var->IsExport()) continue;
         Handle<SourceTextModule> module(context_->module(), isolate_);
         value = SourceTextModule::LoadVariable(isolate_, module, var->index());
-        // Reflect variables under TDZ as undeclared in scope object.
-        if (value->IsTheHole(isolate_)) continue;
         break;
       }
     }
 
-    if (visitor(var->name(), value)) return true;
+    if (visitor(var->name(), value, scope_type)) return true;
   }
   return false;
 }
@@ -894,9 +897,10 @@ Handle<JSObject> ScopeIterator::WithContextExtension() {
 
 // Create a plain JSObject which materializes the block scope for the specified
 // block context.
-void ScopeIterator::VisitLocalScope(const Visitor& visitor, Mode mode) const {
+void ScopeIterator::VisitLocalScope(const Visitor& visitor, Mode mode,
+                                    ScopeType scope_type) const {
   if (InInnerScope()) {
-    if (VisitLocals(visitor, mode)) return;
+    if (VisitLocals(visitor, mode, scope_type)) return;
     if (mode == Mode::STACK && Type() == ScopeTypeLocal) {
       // Hide |this| in arrow functions that may be embedded in other functions
       // but don't force |this| to be context-allocated. Otherwise we'd find the
@@ -904,7 +908,7 @@ void ScopeIterator::VisitLocalScope(const Visitor& visitor, Mode mode) const {
       if (!closure_scope_->has_this_declaration() &&
           !closure_scope_->HasThisReference()) {
         if (visitor(isolate_->factory()->this_string(),
-                    isolate_->factory()->undefined_value()))
+                    isolate_->factory()->undefined_value(), scope_type))
           return;
       }
       // Add |arguments| to the function scope even if it wasn't used.
@@ -919,13 +923,15 @@ void ScopeIterator::VisitLocalScope(const Visitor& visitor, Mode mode) const {
         JavaScriptFrame* frame = GetFrame();
         Handle<JSObject> arguments = Accessors::FunctionGetArguments(
             frame, frame_inspector_->inlined_frame_index());
-        if (visitor(isolate_->factory()->arguments_string(), arguments)) return;
+        if (visitor(isolate_->factory()->arguments_string(), arguments,
+                    scope_type))
+          return;
       }
     }
   } else {
     DCHECK_EQ(Mode::ALL, mode);
     Handle<ScopeInfo> scope_info(context_->scope_info(), isolate_);
-    if (VisitContextLocals(visitor, scope_info, context_)) return;
+    if (VisitContextLocals(visitor, scope_info, context_, scope_type)) return;
   }
 
   if (mode == Mode::ALL && HasContext()) {
@@ -945,7 +951,7 @@ void ScopeIterator::VisitLocalScope(const Visitor& visitor, Mode mode) const {
       DCHECK(keys->get(i).IsString());
       Handle<String> key(String::cast(keys->get(i)), isolate_);
       Handle<Object> value = JSReceiver::GetDataProperty(extension, key);
-      if (visitor(key, value)) return;
+      if (visitor(key, value, scope_type)) return;
     }
   }
 }

src/debug/debug-scopes.h

@@ -141,8 +141,8 @@ class ScopeIterator {
 
   void UnwrapEvaluationContext();
 
-  using Visitor =
-      std::function<bool(Handle<String> name, Handle<Object> value)>;
+  using Visitor = std::function<bool(Handle<String> name, Handle<Object> value,
+                                     ScopeType scope_type)>;
 
   Handle<JSObject> WithContextExtension();
 
@@ -159,12 +159,14 @@ class ScopeIterator {
 
   // Helper functions.
   void VisitScope(const Visitor& visitor, Mode mode) const;
-  void VisitLocalScope(const Visitor& visitor, Mode mode) const;
+  void VisitLocalScope(const Visitor& visitor, Mode mode,
+                       ScopeType scope_type) const;
   void VisitScriptScope(const Visitor& visitor) const;
   void VisitModuleScope(const Visitor& visitor) const;
-  bool VisitLocals(const Visitor& visitor, Mode mode) const;
+  bool VisitLocals(const Visitor& visitor, Mode mode,
+                   ScopeType scope_type) const;
   bool VisitContextLocals(const Visitor& visitor, Handle<ScopeInfo> scope_info,
-                          Handle<Context> context) const;
+                          Handle<Context> context, ScopeType scope_type) const;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(ScopeIterator);
 };

test/debugger/debug/harmony/modules-debug-scopes2.mjs

@@ -77,16 +77,6 @@ function CheckScopeChain(scopes, exec_state) {
 }
 
 
-function CheckScopeDoesNotHave(properties, number, exec_state) {
-  var scope = exec_state.frame().scope(number);
-  for (var p of properties) {
-    var property_mirror = scope.scopeObject().property(p);
-    assertTrue(property_mirror.isUndefined(),
-               'property ' + p + ' found in scope');
-  }
-}
-
-
 // Check that the scope contains at least minimum_content. For functions just
 // check that there is a function.
 function CheckScopeContent(minimum_content, number, exec_state) {
@@ -138,9 +128,6 @@ listener_delegate = function(exec_state) {
   CheckScopeContent(
       {exported_var: undefined, imported_var: undefined},
       0, exec_state);
-  CheckScopeDoesNotHave(
-      ["doesntexist", "exported_let", "imported_let"],
-      0, exec_state);
 };
 debugger;
 EndTest();
@@ -160,7 +147,6 @@ listener_delegate = function(exec_state) {
   CheckScopeContent(
       {exported_let: 3, exported_var: 4,
        imported_let: 3, imported_var: 4}, 0, exec_state);
-  CheckScopeDoesNotHave([], 0, exec_state);
 };
 debugger;
 EndTest();
@@ -178,7 +164,6 @@ listener_delegate = function(exec_state) {
   CheckScopeContent(
       {exported_let: 13, exported_var: 14,
        imported_let: 13, imported_var: 14}, 0, exec_state);
-  CheckScopeDoesNotHave([], 0, exec_state);
 };
 debugger;
 EndTest();

test/inspector/debugger/tdz-modules-scopes-expected.txt

@@ -0,0 +1,45 @@
+Test module scope with variables in TDZ.
+Debug break
+  Scope type: module
+    moduleLet : undefined
+    moduleConst : undefined
+    exportedModuleLet : undefined
+    exportedModuleConst : undefined
+Debug break
+  Scope type: local
+  Scope type: module
+    exportedModuleLet : undefined
+    exportedModuleConst : undefined
+Debug break
+  Scope type: module
+    moduleLet : 3
+    moduleConst : 4
+    exportedModuleLet : 1
+    exportedModuleConst : 2
+Debug break
+  Scope type: local
+  Scope type: module
+    exportedModuleLet : 1
+    exportedModuleConst : 2
+Debug break
+  Scope type: module
+    exportedModuleLet : 1
+    exportedModuleConst : 2
+    moduleLet : undefined
+    moduleConst : undefined
+Debug break
+  Scope type: local
+  Scope type: module
+    exportedModuleLet : 1
+    exportedModuleConst : 2
+Debug break
+  Scope type: module
+    exportedModuleLet : 1
+    exportedModuleConst : 2
+    moduleLet : 5
+    moduleConst : 6
+Debug break
+  Scope type: local
+  Scope type: module
+    exportedModuleLet : 1
+    exportedModuleConst : 2

test/inspector/debugger/tdz-modules-scopes.js

@@ -0,0 +1,52 @@
+// 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.
+
+// Flags: --no-stress-opt --no-always-opt
+
+let {session, contextGroup, Protocol} =
+    InspectorTest.start('Test module scope with variables in TDZ.');
+
+
+Protocol.Debugger.enable();
+
+contextGroup.addModule(`
+debugger;
+(function() { debugger })();
+let moduleLet = 3;
+const moduleConst = 4;
+export let exportedModuleLet = 1;
+export const exportedModuleConst = 2;
+export default 42
+debugger;
+(function() { debugger })();
+`, "module1");
+
+contextGroup.addModule(`
+debugger;
+(function() { debugger })();
+import { exportedModuleLet, exportedModuleConst } from 'module1';
+let moduleLet = 5;
+const moduleConst = 6;
+debugger;
+(function() { debugger })();
+`, "module2");
+
+(async function() {
+for (let i =0; i < 8; i++) {
+  let message = await Protocol.Debugger.oncePaused();
+  let scopeChain = message.params.callFrames[0].scopeChain;
+  let evalScopeObjectIds = [];
+  InspectorTest.log("Debug break");
+  for (let scope of scopeChain) {
+    if (scope.type == "global") continue;
+    InspectorTest.log(`  Scope type: ${scope.type}`);
+    let { result: { result: locals }} = await Protocol.Runtime.getProperties({ "objectId" : scope.object.objectId });
+    for (let local of locals) {
+      InspectorTest.log(`    ${local.name} : ${local.value.value}`);
+    }
+  }
+  await Protocol.Debugger.resume();
+}
+InspectorTest.completeTest();
+})();

test/inspector/debugger/tdz-scopes-expected.txt

@@ -0,0 +1,53 @@
+Test scopes with variables in TDZ.
+Debug break
+  Scope type: block
+    blockLet : undefined
+    blockConst : undefined
+    contextBlockLet : undefined
+    contextBlockConst : undefined
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: local
+  Scope type: block
+    contextBlockLet : undefined
+    contextBlockConst : undefined
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: block
+    blockLet : 1
+    blockConst : 2
+    contextBlockLet : 3
+    contextBlockConst : 4
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: local
+  Scope type: block
+    contextBlockLet : 3
+    contextBlockConst : 4
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: local
+  Scope type: script
+    scriptLet : undefined
+    scriptConst : undefined
+Debug break
+  Scope type: script
+    scriptLet : 1
+    scriptConst : 2
+Debug break
+  Scope type: local
+  Scope type: script
+    scriptLet : 1
+    scriptConst : 2

test/inspector/debugger/tdz-scopes.js

@@ -0,0 +1,50 @@
+// 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.
+
+// Flags: --no-stress-opt --no-always-opt
+
+let {session, contextGroup, Protocol} =
+    InspectorTest.start('Test scopes with variables in TDZ.');
+
+
+Protocol.Debugger.enable();
+
+contextGroup.addScript(`
+{
+  debugger;
+  (function() { debugger })();
+  let blockLet = 1;
+  const blockConst = 2;
+  let contextBlockLet = 3;
+  let contextBlockConst = 4;
+  _ => contextBlockConst + contextBlockLet;
+  debugger;
+  (function() { debugger })();
+}
+debugger;
+(function() { debugger })();
+let scriptLet = 1;
+const scriptConst = 2;
+debugger;
+(function() { debugger })();
+`);
+
+(async function() {
+for (let i =0; i < 8; i++) {
+  let message = await Protocol.Debugger.oncePaused();
+  let scopeChain = message.params.callFrames[0].scopeChain;
+  let evalScopeObjectIds = [];
+  InspectorTest.log("Debug break");
+  for (let scope of scopeChain) {
+    if (scope.type == "global") continue;
+    InspectorTest.log(`  Scope type: ${scope.type}`);
+    let { result: { result: locals }} = await Protocol.Runtime.getProperties({ "objectId" : scope.object.objectId });
+    for (let local of locals) {
+      InspectorTest.log(`    ${local.name} : ${local.value.value}`);
+    }
+  }
+  await Protocol.Debugger.resume();
+}
+InspectorTest.completeTest();
+})();