[parser] Fix variable caching for conflict lookup

During conflict lookup (for lexical variables and sloppy block function
hoisting), we cache the looked-up variable on the current scope if the
lookup goes through a ScopeInfo. However, for variable lookup during
scope analysis, we use the "entry point" as the cache.

Since both lookups can create Variables, this can cause us to create
duplicate variables, e.g. a duplicate function name variable in the
attached test.

Instead, for ScopeInfo conflict lookups we can cache the result on the
function's outer scope, which shoud be equivalent to the entry point.

As a (necessary) drive-by, we can terminate the lookup early if we find
a VAR with the same name, as we can safely assume that its existence
means that it doesn't conflict, which means that our variable can't
conflict either.

Bug: chromium:1026603
Change-Id: I19f80f65597ba6573ebe0b48aa5698f55e5c3ea1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1928861
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65138}

src/ast/scopes.cc

@@ -504,9 +504,9 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
     // example, that does not prevent hoisting of the function in
     // `{ let e; try {} catch (e) { function e(){} } }`
     do {
-      var = query_scope->LookupInScopeOrScopeInfo(name);
-      if (var != nullptr && IsLexicalVariableMode(var->mode())) {
-        should_hoist = false;
+      var = query_scope->LookupInScopeOrScopeInfo(name, outer_scope_);
+      if (var != nullptr) {
+        should_hoist = !IsLexicalVariableMode(var->mode());
         break;
       }
       query_scope = query_scope->outer_scope();
@@ -1158,9 +1158,12 @@ Declaration* DeclarationScope::CheckConflictingVarDeclarations() {
     do {
       // There is a conflict if there exists a non-VAR binding up to the
       // declaration scope in which this sloppy-eval runs.
-      Variable* other_var =
-          current->LookupInScopeOrScopeInfo(decl->var()->raw_name());
-      if (other_var != nullptr && IsLexicalVariableMode(other_var->mode())) {
+      Variable* other_var = current->LookupInScopeOrScopeInfo(
+          decl->var()->raw_name(), outer_scope_);
+      if (other_var != nullptr) {
+        // If this is a VAR, then we know that it doesn't conflict with
+        // anything, so we can't conflict with anything either.
+        if (!IsLexicalVariableMode(other_var->mode())) return nullptr;
         DCHECK(!current->is_catch_scope());
         return decl;
       }

src/ast/scopes.h

@@ -555,15 +555,16 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
     return false;
   }
 
-  Variable* LookupInScopeOrScopeInfo(const AstRawString* name) {
+  Variable* LookupInScopeOrScopeInfo(const AstRawString* name, Scope* cache) {
     Variable* var = variables_.Lookup(name);
     if (var != nullptr || scope_info_.is_null()) return var;
-    return LookupInScopeInfo(name, this);
+    DCHECK_NOT_NULL(cache);
+    return LookupInScopeInfo(name, cache);
   }
 
   Variable* LookupForTesting(const AstRawString* name) {
     for (Scope* scope = this; scope != nullptr; scope = scope->outer_scope()) {
-      Variable* var = scope->LookupInScopeOrScopeInfo(name);
+      Variable* var = scope->LookupInScopeOrScopeInfo(name, this);
       if (var != nullptr) return var;
     }
     return nullptr;

test/mjsunit/regress/regress-crbug-1026603.js

+// Copyright 2019 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.
+
+(function f() {
+  with ({}) {
+    // Make sure that variable conflict resulution and variable lookup through
+    // deserialized scopes use the same cache scope. Declare a variable which
+    // checks for (and fails to find) a conflict, allocating the f variable as
+    // it goes, then access f, which also looks up f.
+    eval("var f; f;");
+  }
+})();