[ic] Correctly Handle global loads when global object has proxies

When global object has proxies we should first call hasProperty and
then call GetProperty according to spec. This cl fixes both
LoadGlobal and LoadLookupGlobal to correctly handle these cases.

Also fixes tests that didn't expect hasProperty to be called.

Change-Id: I3a45df7ae24be74dd46cf04cafbf8c2d7018b3af
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1876059
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64580}

src/ic/accessor-assembler.cc

@@ -543,6 +543,14 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase(
 
   BIND(&proxy);
   {
+    // TODO(mythria): LoadGlobals don't use this path. LoadGlobals need special
+    // handling with proxies which is currently not supported by builtins. So
+    // for such cases, we should install a slow path and never reach here. Fix
+    // it to not generate this for LoadGlobals.
+    CSA_ASSERT(this,
+               WordNotEqual(IntPtrConstant(static_cast<int>(on_nonexistent)),
+                            IntPtrConstant(static_cast<int>(
+                                OnNonExistent::kThrowReferenceError))));
     TVARIABLE(IntPtrT, var_index);
     TVARIABLE(Name, var_unique);
 

src/ic/ic.cc

@@ -370,8 +370,9 @@ void IC::ConfigureVectorState(Handle<Name> name, MapHandles const& maps,
   OnFeedbackChanged("Polymorphic");
 }
 
-MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
-  bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
+MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name,
+                                 bool update_feedback) {
+  bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic && update_feedback;
 
   // If the object is undefined or null it's illegal to try to get any
   // of its properties; throw a TypeError in that case.
@@ -439,8 +440,8 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
     // Get the property.
     Handle<Object> result;
 
-    ASSIGN_RETURN_ON_EXCEPTION(isolate(), result, Object::GetProperty(&it),
-                               Object);
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate(), result, Object::GetProperty(&it, IsLoadGlobalIC()), Object);
     if (it.IsFound()) {
       return result;
     } else if (!ShouldThrowReferenceError()) {
@@ -451,7 +452,8 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
   return ReferenceError(name);
 }
 
-MaybeHandle<Object> LoadGlobalIC::Load(Handle<Name> name) {
+MaybeHandle<Object> LoadGlobalIC::Load(Handle<Name> name,
+                                       bool update_feedback) {
   Handle<JSGlobalObject> global = isolate()->global_object();
 
   if (name->IsString()) {
@@ -475,7 +477,7 @@ MaybeHandle<Object> LoadGlobalIC::Load(Handle<Name> name) {
         return ReferenceError(name);
       }
 
-      bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
+      bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic && update_feedback;
       if (use_ic) {
         if (nexus()->ConfigureLexicalVarMode(
                 lookup_result.context_index, lookup_result.slot_index,
@@ -491,7 +493,7 @@ MaybeHandle<Object> LoadGlobalIC::Load(Handle<Name> name) {
       return result;
     }
   }
-  return LoadIC::Load(global, name);
+  return LoadIC::Load(global, name, update_feedback);
 }
 
 static bool AddOneReceiverMapIfMissing(MapHandles* receiver_maps,
@@ -659,6 +661,14 @@ void LoadIC::UpdateCaches(LookupIterator* lookup) {
     code = LoadHandler::LoadFullChain(
         isolate(), receiver_map(),
         MaybeObjectHandle(isolate()->factory()->null_value()), smi_handler);
+  } else if (IsLoadGlobalIC() && lookup->state() == LookupIterator::JSPROXY) {
+    // If there is proxy just install the slow stub since we need to call the
+    // HasProperty trap for global loads. The ProxyGetProperty builtin doesn't
+    // handle this case.
+    Handle<Smi> slow_handler = LoadHandler::LoadSlow(isolate());
+    Handle<JSProxy> holder = lookup->GetHolder<JSProxy>();
+    code = LoadHandler::LoadFromPrototype(isolate(), receiver_map(), holder,
+                                          slow_handler);
   } else {
     if (IsLoadGlobalIC()) {
       if (lookup->TryLookupCachedProperty()) {
@@ -2226,42 +2236,14 @@ RUNTIME_FUNCTION(Runtime_LoadGlobalIC_Slow) {
   DCHECK_EQ(3, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
 
-  Handle<Context> native_context = isolate->native_context();
-  Handle<ScriptContextTable> script_contexts(
-      native_context->script_context_table(), isolate);
-
-  ScriptContextTable::LookupResult lookup_result;
-  if (ScriptContextTable::Lookup(isolate, *script_contexts, *name,
-                                 &lookup_result)) {
-    Handle<Context> script_context = ScriptContextTable::GetContext(
-        isolate, script_contexts, lookup_result.context_index);
-    Handle<Object> result(script_context->get(lookup_result.slot_index),
-                          isolate);
-    if (*result == ReadOnlyRoots(isolate).the_hole_value()) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewReferenceError(MessageTemplate::kNotDefined, name));
-    }
-    return *result;
-  }
+  Handle<Smi> slot = args.at<Smi>(1);
+  Handle<FeedbackVector> vector = args.at<FeedbackVector>(2);
+  FeedbackSlot vector_slot = FeedbackVector::ToSlot(slot->value());
+  FeedbackSlotKind kind = vector->GetKind(vector_slot);
 
-  Handle<JSGlobalObject> global(native_context->global_object(), isolate);
+  LoadGlobalIC ic(isolate, vector, vector_slot, kind);
   Handle<Object> result;
-  bool is_found = false;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, result,
-      Runtime::GetObjectProperty(isolate, global, name, &is_found));
-  if (!is_found) {
-    Handle<Smi> slot = args.at<Smi>(1);
-    Handle<FeedbackVector> vector = args.at<FeedbackVector>(2);
-    FeedbackSlot vector_slot = FeedbackVector::ToSlot(slot->value());
-    FeedbackSlotKind kind = vector->GetKind(vector_slot);
-    // It is actually a LoadGlobalICs here but the predicate handles this case
-    // properly.
-    if (LoadIC::ShouldThrowReferenceError(kind)) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewReferenceError(MessageTemplate::kNotDefined, name));
-    }
-  }
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result, ic.Load(name, false));
   return *result;
 }
 

src/ic/ic.h

@@ -183,7 +183,8 @@ class LoadIC : public IC {
   }
 
   V8_WARN_UNUSED_RESULT MaybeHandle<Object> Load(Handle<Object> object,
-                                                 Handle<Name> name);
+                                                 Handle<Name> name,
+                                                 bool update_feedback = true);
 
  protected:
   // Update the inline cache and the global stub cache based on the
@@ -203,7 +204,8 @@ class LoadGlobalIC : public LoadIC {
                FeedbackSlot slot, FeedbackSlotKind kind)
       : LoadIC(isolate, vector, slot, kind) {}
 
-  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Load(Handle<Name> name);
+  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Load(Handle<Name> name,
+                                                 bool update_feedback = true);
 };
 
 class KeyedLoadIC : public LoadIC {

src/objects/contexts.cc

@@ -135,11 +135,11 @@ JSGlobalProxy Context::global_proxy() {
  * Lookups a property in an object environment, taking the unscopables into
  * account. This is used For HasBinding spec algorithms for ObjectEnvironment.
  */
-static Maybe<bool> UnscopableLookup(LookupIterator* it) {
+static Maybe<bool> UnscopableLookup(LookupIterator* it, bool is_with_context) {
   Isolate* isolate = it->isolate();
 
   Maybe<bool> found = JSReceiver::HasProperty(it);
-  if (found.IsNothing() || !found.FromJust()) return found;
+  if (!is_with_context || found.IsNothing() || !found.FromJust()) return found;
 
   Handle<Object> unscopables;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -234,7 +234,7 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
       if ((flags & FOLLOW_PROTOTYPE_CHAIN) == 0 ||
           object->IsJSContextExtensionObject()) {
         maybe = JSReceiver::GetOwnPropertyAttributes(object, name);
-      } else if (context->IsWithContext()) {
+      } else {
         // A with context will never bind "this", but debug-eval may look into
         // a with context when resolving "this". Other synthetic variables such
         // as new.target may be resolved as VariableMode::kDynamicLocal due to
@@ -243,10 +243,11 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
         // TODO(v8:5405): Replace this check with a DCHECK when resolution of
         // of synthetic variables does not go through this code path.
         if (ScopeInfo::VariableIsSynthetic(*name)) {
+          DCHECK(context->IsWithContext());
           maybe = Just(ABSENT);
         } else {
           LookupIterator it(object, name, object);
-          Maybe<bool> found = UnscopableLookup(&it);
+          Maybe<bool> found = UnscopableLookup(&it, context->IsWithContext());
           if (found.IsNothing()) {
             maybe = Nothing<PropertyAttributes>();
           } else {
@@ -256,8 +257,6 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
             maybe = Just(found.FromJust() ? NONE : ABSENT);
           }
         }
-      } else {
-        maybe = JSReceiver::GetPropertyAttributes(object, name);
       }
 
       if (maybe.IsNothing()) return Handle<Object>();

src/objects/objects.cc

@@ -1083,7 +1083,7 @@ MaybeHandle<Object> Object::GetLengthFromArrayLike(Isolate* isolate,
 
 // static
 MaybeHandle<Object> Object::GetProperty(LookupIterator* it,
-                                        OnNonExistent on_non_existent) {
+                                        bool is_global_reference) {
   for (; it->IsFound(); it->Next()) {
     switch (it->state()) {
       case LookupIterator::NOT_FOUND:
@@ -1098,10 +1098,18 @@ MaybeHandle<Object> Object::GetProperty(LookupIterator* it,
           receiver = handle(JSGlobalObject::cast(*receiver).global_proxy(),
                             it->isolate());
         }
+        if (is_global_reference) {
+          Maybe<bool> maybe = JSProxy::HasProperty(
+              it->isolate(), it->GetHolder<JSProxy>(), it->GetName());
+          if (maybe.IsNothing() || !maybe.FromJust()) {
+            it->NotFound();
+            return it->isolate()->factory()->undefined_value();
+          }
+        }
         MaybeHandle<Object> result =
             JSProxy::GetProperty(it->isolate(), it->GetHolder<JSProxy>(),
                                  it->GetName(), receiver, &was_found);
-        if (!was_found) it->NotFound();
+        if (!was_found && !is_global_reference) it->NotFound();
         return result;
       }
       case LookupIterator::INTERCEPTOR: {
@@ -1125,11 +1133,6 @@ MaybeHandle<Object> Object::GetProperty(LookupIterator* it,
     }
   }
 
-  if (on_non_existent == OnNonExistent::kThrowReferenceError) {
-    THROW_NEW_ERROR(it->isolate(),
-                    NewReferenceError(MessageTemplate::kNotDefined, it->name()),
-                    Object);
-  }
   return it->isolate()->factory()->undefined_value();
 }
 

src/objects/objects.h

@@ -468,8 +468,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
       Isolate* isolate, Handle<Object> object, Handle<Object> callable);
 
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
-  GetProperty(LookupIterator* it,
-              OnNonExistent on_non_existent = OnNonExistent::kReturnUndefined);
+  GetProperty(LookupIterator* it, bool is_global_reference = false);
 
   // ES6 [[Set]] (when passed kDontThrow)
   // Invariants for this and related functions (unless stated otherwise):

src/runtime/runtime-proxy.cc

@@ -23,7 +23,14 @@ RUNTIME_FUNCTION(Runtime_GetPropertyWithReceiver) {
   CONVERT_ARG_HANDLE_CHECKED(JSReceiver, holder, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
   CONVERT_ARG_HANDLE_CHECKED(Object, receiver, 2);
+  // TODO(mythria): Remove the on_non_existent parameter to this function. This
+  // should only be called when getting named properties on receiver. This
+  // doesn't handle the global variable loads.
+#ifdef DEBUG
   CONVERT_ARG_HANDLE_CHECKED(Smi, on_non_existent, 3);
+  DCHECK_NE(static_cast<OnNonExistent>(on_non_existent->value()),
+            OnNonExistent::kThrowReferenceError);
+#endif
 
   bool success = false;
   LookupIterator it = LookupIterator::PropertyOrElement(isolate, receiver, key,
@@ -33,9 +40,7 @@ RUNTIME_FUNCTION(Runtime_GetPropertyWithReceiver) {
     return ReadOnlyRoots(isolate).exception();
   }
 
-  RETURN_RESULT_OR_FAILURE(
-      isolate, Object::GetProperty(
-                   &it, static_cast<OnNonExistent>(on_non_existent->value())));
+  RETURN_RESULT_OR_FAILURE(isolate, Object::GetProperty(&it));
 }
 
 RUNTIME_FUNCTION(Runtime_SetPropertyWithReceiver) {

test/mjsunit/es6/global-proto-proxy.js

@@ -8,7 +8,7 @@ var global = this;
   var calledGet = false;
   var calledHas = false;
   var calledSet = false;
-  var target = {};
+  var target = {getGlobal: 1};
   var assertEquals = global.assertEquals;
   var proxy = new Proxy(target, {
     has(target, property) {
@@ -35,7 +35,8 @@ var global = this;
   assertTrue(calledSet);
   "findGlobal" in global;
   assertTrue(calledHas);
+  assertEquals("number", typeof(makeGlobal));
   var deletedOwn = delete makeGlobal;
   assertTrue(deletedOwn);
-  assertEquals(void 0, makeGlobal);
+  assertEquals("undefined", typeof(makeGlobal));
 })();

test/mjsunit/global-load-with-proxy.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.
+
+// Flags: --allow-natives-syntax
+
+var get_count = 0;
+var has_count = 0;
+var property_descriptor_count = 0;
+globalThis.__proto__ = new Proxy({},
+                                 {get() {get_count++},
+                                  has() {has_count++;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+function checkCounts(count) {
+  assertEquals(has_count, count);
+  assertEquals(get_count, 0);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function load_lookup_global_error() {
+  eval("var b = 10");
+  x;
+}
+assertThrows(load_lookup_global_error, ReferenceError);
+checkCounts(1);
+
+%EnsureFeedbackVectorForFunction(load_lookup_global_error);
+assertThrows(load_lookup_global_error, ReferenceError);
+checkCounts(2);
+assertThrows(load_lookup_global_error, ReferenceError);
+checkCounts(3);
+
+function load_global_error() {
+  x;
+}
+assertThrows(load_global_error, ReferenceError);
+checkCounts(4);
+
+%EnsureFeedbackVectorForFunction(load_global_error);
+assertThrows(load_global_error, ReferenceError);
+checkCounts(5);
+assertThrows(load_global_error, ReferenceError);
+checkCounts(6);
+
+
+// Check when the object is present on the proxy.
+get_count = 0;
+has_count = 0;
+property_descriptor_count = 0;
+
+globalThis.__proto__ = new Proxy({},
+                                 {get() {get_count++; return 10;},
+                                  has() {has_count++; return true;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+function checkCountsWithGet(count) {
+  assertEquals(has_count, count);
+  assertEquals(get_count, count);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function load_lookup_global() {
+  eval("var b = 10");
+  return x;
+}
+assertEquals(load_lookup_global(), 10);
+checkCountsWithGet(1);
+
+%EnsureFeedbackVectorForFunction(load_lookup_global);
+assertEquals(load_lookup_global(), 10);
+checkCountsWithGet(2);
+assertEquals(load_lookup_global(), 10);
+checkCountsWithGet(3);
+
+function load_global() {
+  return x;
+}
+assertEquals(load_global(), 10);
+checkCountsWithGet(4);
+
+%EnsureFeedbackVectorForFunction(load_global);
+assertEquals(load_global(), 10);
+checkCountsWithGet(5);
+assertEquals(load_global(), 10);
+checkCountsWithGet(6);
+
+// Check unbound variable access inside typeof
+get_count = 0;
+has_count = 0;
+property_descriptor_count = 0;
+
+globalThis.__proto__ = new Proxy({},
+                                 {get() {get_count++},
+                                  has() {has_count++;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+function checkCountsInsideTypeof(count) {
+  assertEquals(has_count, count);
+  assertEquals(get_count, 0);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function load_lookup_inside_typeof() {
+  eval("var b = 10");
+  return typeof(x);
+}
+assertEquals(load_lookup_inside_typeof(), "undefined");
+checkCountsInsideTypeof(1);
+
+%EnsureFeedbackVectorForFunction(load_lookup_inside_typeof);
+assertEquals(load_lookup_inside_typeof(), "undefined");
+checkCountsInsideTypeof(2);
+assertEquals(load_lookup_inside_typeof(), "undefined");
+checkCountsInsideTypeof(3);
+
+function load_inside_typeof() {
+  return typeof(x);
+}
+assertEquals(load_inside_typeof(), "undefined");
+checkCountsInsideTypeof(4);
+
+%EnsureFeedbackVectorForFunction(load_inside_typeof);
+assertEquals(load_inside_typeof(), "undefined");
+checkCountsInsideTypeof(5);
+assertEquals(load_inside_typeof(), "undefined");
+checkCountsInsideTypeof(6);
+
+// Check bound variable access inside typeof
+get_count = 0;
+has_count = 0;
+property_descriptor_count = 0;
+
+globalThis.__proto__ = new Proxy({},
+                                 {get() {get_count++; return 10;},
+                                  has() {has_count++; return true;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+
+function checkCountsBoundVarInsideTypeof(count) {
+  assertEquals(has_count, count);
+  assertEquals(get_count, count);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function load_lookup_number_inside_typeof() {
+  eval("var b = 10");
+  return typeof(x);
+}
+assertEquals(load_lookup_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(1);
+
+%EnsureFeedbackVectorForFunction(load_lookup_number_inside_typeof);
+assertEquals(load_lookup_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(2);
+assertEquals(load_lookup_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(3);
+
+function load_number_inside_typeof() {
+  return typeof(x);
+}
+assertEquals(load_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(4);
+
+%EnsureFeedbackVectorForFunction(load_inside_typeof);
+assertEquals(load_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(5);
+assertEquals(load_number_inside_typeof(), "number");
+checkCountsBoundVarInsideTypeof(6);
+
+// Check that if has property returns true we don't throw even when get property
+// says otherwise.
+
+globalThis.__proto__ = new Proxy({},
+                                 {has() {has_count++; return true;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+
+function load_lookup_global_has_property() {
+  eval("var b = 10");
+  return x;
+}
+assertEquals(load_lookup_global_has_property(), undefined);
+
+function load_global_has_property() {
+  return x;
+}
+assertEquals(load_global_has_property(), undefined);

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

@@ -4,6 +4,9 @@
 
 const globalThis = this;
 Object.setPrototypeOf(this, new Proxy({}, {
+  has() { return true; },
+  getOwnPropertyDescriptor() {
+    assertUnreachable("getOwnPropertyDescriptor shouldn't be called."); },
   get(target, prop, receiver) {
     assertTrue(receiver === globalThis);
   }