[parser] Fix caching dynamic vars on wrong scope

When looking up a variable in a deserialized WITH scope, we were
unconditionally passing in the cache scope to the lookup, even if the
with was inside the cache scope. This would lead to and outer scope of
the with holding the generated dynamic variable. If the cache scope was
the SCRIPT scope, the dynamic variable would be interpreted as a global
object property.

Now, we only store the WITH scope dynamic variables in the cache scope
if it is an inner scope of the WITH scope, same as we do for 'normal'
scope lookups.

Fixed: chromium:1041210
Change-Id: I4e8eb25bbb8ea58311355d13a9c7c97bf2fa3ec7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1997135Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65732}

src/ast/scopes.cc

@@ -7,6 +7,7 @@
 #include <set>
 
 #include "src/ast/ast.h"
+#include "src/base/logging.h"
 #include "src/base/optional.h"
 #include "src/builtins/accessors.h"
 #include "src/common/message-template.h"
@@ -1899,8 +1900,9 @@ template <Scope::ScopeLookupMode mode>
 Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
                         Scope* outer_scope_end, Scope* cache_scope,
                         bool force_context_allocation) {
-  // If we have already passed it in earlier recursions though, so we should
-  // first quickly check if it is an outer scope before continuing.
+  // If we have already passed the cache scope in earlier recursions, we should
+  // first quickly check if the current scope uses the cache scope before
+  // continuing.
   if (mode == kDeserializedScope &&
       scope->deserialized_scope_uses_external_cache()) {
     Variable* var = cache_scope->variables_.Lookup(proxy->raw_name());
@@ -1918,13 +1920,17 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
     // the scopes in which it's evaluating.
     if (mode == kDeserializedScope &&
         V8_UNLIKELY(scope->is_debug_evaluate_scope_)) {
+      DCHECK(scope->deserialized_scope_uses_external_cache() ||
+             scope == cache_scope);
       return cache_scope->NonLocal(proxy->raw_name(), VariableMode::kDynamic);
     }
 
     // Try to find the variable in this scope.
     Variable* var;
+    Scope* current_cache_scope = nullptr;
     if (mode == kParsedScope) {
       var = scope->LookupLocal(proxy->raw_name());
+      DCHECK_NULL(cache_scope);
     } else {
       DCHECK_EQ(mode, kDeserializedScope);
       bool external_cache = scope->deserialized_scope_uses_external_cache();
@@ -1936,8 +1942,8 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
         Variable* var = scope->variables_.Lookup(proxy->raw_name());
         if (var != nullptr) return var;
       }
-      var = scope->LookupInScopeInfo(proxy->raw_name(),
-                                     external_cache ? cache_scope : scope);
+      current_cache_scope = external_cache ? cache_scope : scope;
+      var = scope->LookupInScopeInfo(proxy->raw_name(), current_cache_scope);
     }
 
     // We found a variable and we are done. (Even if there is an 'eval' in this
@@ -1966,14 +1972,14 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
 
     DCHECK(!scope->is_script_scope());
     if (V8_UNLIKELY(scope->is_with_scope())) {
-      return LookupWith(proxy, scope, outer_scope_end, cache_scope,
+      return LookupWith(proxy, scope, outer_scope_end, current_cache_scope,
                         force_context_allocation);
     }
     if (V8_UNLIKELY(
             scope->is_declaration_scope() &&
             scope->AsDeclarationScope()->sloppy_eval_can_extend_vars())) {
-      return LookupSloppyEval(proxy, scope, outer_scope_end, cache_scope,
-                              force_context_allocation);
+      return LookupSloppyEval(proxy, scope, outer_scope_end,
+                              current_cache_scope, force_context_allocation);
     }
 
     force_context_allocation |= scope->is_function_scope();

test/mjsunit/regress/regress-crbug-1041210.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.
+//
+// Flags: --stress-lazy-source-positions
+
+with ({}) {
+  (() => eval())();
+}