Fix "this" value in lazily-parsed module functions.

When preparsing top-level functions in a module, we didn't track
unresolved variables. Consequently, "this" ended up referencing
the global "this", which has the wrong value (in a module "this"
is supposed to be the undefined value).

This patch fixes that. This also lets us stop forcing context
allocation of all variables in module scopes, which the patch
takes care of as well.

Bug: chromium:791334
Change-Id: Ifac1f1adc033f3facfb3d29dd4bca32ee27bffcf
Reviewed-on: https://chromium-review.googlesource.com/808938Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50025}

src/ast/scopes.cc

@@ -147,8 +147,6 @@ Scope::Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type)
   DCHECK_NE(SCRIPT_SCOPE, scope_type);
   SetDefaults();
   set_language_mode(outer_scope->language_mode());
-  force_context_allocation_ =
-      !is_function_scope() && outer_scope->has_forced_context_allocation();
   outer_scope_->AddInnerScope(this);
 }
 
@@ -1370,12 +1368,8 @@ bool Scope::AllowsLazyParsingWithoutUnresolvedVariables(
     if (s->is_catch_scope()) continue;
     // With scopes do not introduce variables that need allocation.
     if (s->is_with_scope()) continue;
-    // Module scopes context-allocate all variables, and have no
-    // {this} or {arguments} variables whose existence depends on
-    // references to them.
-    if (s->is_module_scope()) continue;
-    // Only block scopes and function scopes should disallow preparsing.
-    DCHECK(s->is_block_scope() || s->is_function_scope());
+    DCHECK(s->is_module_scope() || s->is_block_scope() ||
+           s->is_function_scope());
     return false;
   }
   return true;
@@ -1734,9 +1728,6 @@ void Scope::Print(int n) {
     if (scope->was_lazily_parsed()) Indent(n1, "// lazily parsed\n");
     if (scope->ShouldEagerCompile()) Indent(n1, "// will be compiled\n");
   }
-  if (has_forced_context_allocation()) {
-    Indent(n1, "// forces context allocation\n");
-  }
   if (num_stack_slots_ > 0) {
     Indent(n1, "// ");
     PrintF("%d stack slots\n", num_stack_slots_);
@@ -2111,11 +2102,8 @@ bool Scope::MustAllocateInContext(Variable* var) {
   // an eval() call or a runtime with lookup), it must be allocated in the
   // context.
   //
-  // Exceptions: If the scope as a whole has forced context allocation, all
-  // variables will have context allocation, even temporaries.  Otherwise
-  // temporary variables are always stack-allocated.  Catch-bound variables are
+  // Temporary variables are always stack-allocated.  Catch-bound variables are
   // always context-allocated.
-  if (has_forced_context_allocation()) return true;
   if (var->mode() == TEMPORARY) return false;
   if (is_catch_scope()) return true;
   if ((is_script_scope() || is_eval_scope()) &&

src/ast/scopes.h

@@ -334,14 +334,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   bool is_hidden() const { return is_hidden_; }
   void set_is_hidden() { is_hidden_ = true; }
 
-  // In some cases we want to force context allocation for a whole scope.
-  void ForceContextAllocation() {
-    DCHECK(!already_resolved_);
-    force_context_allocation_ = true;
-  }
-  bool has_forced_context_allocation() const {
-    return force_context_allocation_;
-  }
   void ForceContextAllocationForParameters() {
     DCHECK(!already_resolved_);
     force_context_allocation_for_parameters_ = true;

src/parsing/parser.cc

@@ -690,7 +690,6 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
       var->AllocateTo(VariableLocation::PARAMETER, 0);
 
       PrepareGeneratorVariables();
-      scope->ForceContextAllocation();
       Expression* initial_yield =
           BuildInitialYield(kNoSourcePosition, kGeneratorFunction);
       body->Add(

test/cctest/test-parsing.cc

@@ -6224,9 +6224,9 @@ TEST(ModuleParsingInternals) {
 
   CHECK(declarations->AtForTest(8)->proxy()->raw_name()->IsOneByteEqualTo(
       "nonexport"));
-  CHECK(declarations->AtForTest(8)->proxy()->var()->binding_needs_init());
-  CHECK(declarations->AtForTest(8)->proxy()->var()->location() !=
-        i::VariableLocation::MODULE);
+  CHECK(!declarations->AtForTest(8)->proxy()->var()->binding_needs_init());
+  CHECK(declarations->AtForTest(8)->proxy()->var()->location() ==
+        i::VariableLocation::LOCAL);
 
   CHECK(
       declarations->AtForTest(9)->proxy()->raw_name()->IsOneByteEqualTo("mm"));

test/debugger/debug/debug-modules-set-variable-value.js

@@ -271,7 +271,7 @@ let salad = 12;
   function listener(event, exec_state) {
     if (event == Debug.DebugEvent.Break) {
       let scope_count = exec_state.frame().scopeCount();
-      let module_scope = exec_state.frame().scope(2);
+      let module_scope = exec_state.frame().scope(1);
       assertEquals(debug.ScopeType.Module, module_scope.scopeType());
       module_scope.setVariableValue('salad', 42);
     }
@@ -311,7 +311,7 @@ export let ham = 1;
   function listener(event, exec_state) {
     if (event == Debug.DebugEvent.Break) {
       let scope_count = exec_state.frame().scopeCount();
-      let module_scope = exec_state.frame().scope(2);
+      let module_scope = exec_state.frame().scope(1);
       assertEquals(debug.ScopeType.Module, module_scope.scopeType());
       module_scope.setVariableValue('ham', 2);
     }

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

@@ -139,10 +139,10 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_var: undefined, exported_var: undefined, imported_var: undefined},
+      {exported_var: undefined, imported_var: undefined},
       0, exec_state);
   CheckScopeDoesNotHave(
-      ["doesnotexist", "local_let", "exported_let", "imported_let"],
+      ["local_var", "doesntexist", "local_let", "exported_let", "imported_let"],
       0, exec_state);
 };
 debugger;
@@ -161,8 +161,9 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_let: 1, local_var: 2, exported_let: 3, exported_var: 4,
+      {exported_let: 3, exported_var: 4,
        imported_let: 3, imported_var: 4}, 0, exec_state);
+  CheckScopeDoesNotHave(["local_var", "local_let"], 0, exec_state);
 };
 debugger;
 EndTest();
@@ -178,8 +179,9 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_let: 11, local_var: 12, exported_let: 13, exported_var: 14,
+      {exported_let: 13, exported_var: 14,
        imported_let: 13, imported_var: 14}, 0, exec_state);
+  CheckScopeDoesNotHave(["local_var", "local_let"], 0, exec_state);
 };
 debugger;
 EndTest();

test/inspector/debugger/evaluate-on-call-frame-in-module-expected.txt

@@ -12,9 +12,8 @@ local:foo1
 module
 [
     [0] : a1 = 10
-    [1] : g1 = 1
-    [2] : b1 = 11
-    [3] : foo1 = function foo1() {   let c1 = 12;   let g1 = 2;   debugger;   return a1 + b1 + c1 + g1; }
+    [1] : b1 = 11
+    [2] : foo1 = function foo1() {   let c1 = 12;   let g1 = 2;   debugger;   return a1 + b1 + c1 + g1; }
 ]
 global
 [
@@ -180,9 +179,8 @@ foo2 =
 }
 module
 [
-    [0] : a3 = 30
-    [1] : foo2 = function foo2() {   let c2 = 22;   return foo1() + a2 + b2 + c2; }
-    [2] : b3 = 31
+    [0] : foo2 = function foo2() {   let c2 = 22;   return foo1() + a2 + b2 + c2; }
+    [1] : b3 = 31
 ]
 global
 [
@@ -200,20 +198,6 @@ Array =
     objectId : <objectId>
     type : function
 }
-a3 = 
-{
-    description : 30
-    type : number
-    value : 30
-}
-Evaluating: ++a3
-updated a3 = 
-{
-    description : 31
-    type : number
-    value : 31
-}
-Evaluating: --a3
 foo2 = 
 {
     className : Function
@@ -247,12 +231,6 @@ closure:bar
 [
     [0] : a = 0
 ]
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -283,37 +261,10 @@ updated a =
     value : 1
 }
 Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
 local:bar
 [
     [0] : a = 0
 ]
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -344,33 +295,6 @@ updated a =
     value : 1
 }
 Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -386,41 +310,6 @@ Array =
     objectId : <objectId>
     type : function
 }
-a = 
-{
-    description : 1
-    type : number
-    value : 1
-}
-Evaluating: ++a
-updated a = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
 
 Running test: testDifferentModuleVariables
 (anonymous) (module5:5:0)
@@ -503,3 +392,112 @@ updated c =
     value : 1
 }
 Evaluating: --c
+
+Running test: testCapturedLocalVariable
+(anonymous) (module6:2:25)
+(anonymous) (module6:2:37)
+local
+[
+    [0] : y = 5
+]
+module
+[
+    [0] : x = 5
+]
+global
+[
+  ...
+]
+Check variables in frame#0
+let x = 5;
+(function() { let y = x; #debugger; })()
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}
+y = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++y
+updated y = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --y
+x = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++x
+updated x = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --x
+module
+[
+    [0] : x = 5
+]
+global
+[
+  ...
+]
+Check variables in frame#1
+let x = 5;
+(function() { let y = x; debugger; })#()
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}
+x = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++x
+updated x = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --x
+
+Running test: testLocalVariableToplevel
+(anonymous) (module7:2:0)
+global
+[
+  ...
+]
+Check variables in frame#0
+let x = 5;
+#debugger;
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}

test/inspector/debugger/evaluate-on-call-frame-in-module.js

@@ -49,6 +49,16 @@ export var c = 0;
 debugger;
 `;
 
+var module6 = `
+let x = 5;
+(function() { let y = x; debugger; })()
+`;
+
+var module7 = `
+let x = 5;
+debugger;
+`;
+
 InspectorTest.runAsyncTestSuite([
   async function testTotal() {
     session.setupScriptMap();
@@ -82,6 +92,26 @@ InspectorTest.runAsyncTestSuite([
       await checkFrame(callFrames[i], i);
     }
     await Protocol.Debugger.resume();
+  },
+
+  async function testCapturedLocalVariable() {
+    contextGroup.addModule(module6, 'module6');
+    let {params:{callFrames}} = (await Protocol.Debugger.oncePaused());
+    session.logCallFrames(callFrames);
+    for (let i = 0; i < callFrames.length; ++i) {
+      await checkFrame(callFrames[i], i);
+    }
+    await Protocol.Debugger.resume();
+  },
+
+  async function testLocalVariableToplevel() {
+    contextGroup.addModule(module7, 'module7');
+    let {params:{callFrames}} = (await Protocol.Debugger.oncePaused());
+    session.logCallFrames(callFrames);
+    for (let i = 0; i < callFrames.length; ++i) {
+      await checkFrame(callFrames[i], i);
+    }
+    await Protocol.Debugger.resume();
   }
 ]);
 

test/mjsunit/regress/regress-791334.js

+// Copyright 2017 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.
+
+// MODULE
+
+let foo = () => { return this };
+assertEquals(undefined, foo());