[runtime] Correctly handle global stores when global object has proxies

When global object has proxies we should first call hasProperty and
then call SetProperty if has property returns true. This cl fixes both
StoreGlobal and StoreLookupGlobal to correctly handle these cases.

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

src/ic/ic.cc

@@ -1316,6 +1316,9 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle<Object> value,
         case LookupIterator::TRANSITION:
           UNREACHABLE();
         case LookupIterator::JSPROXY:
+          // StoreGlobals should check for HasProperty before setting the value.
+          // Builtins currently don't handle this.
+          if (IsStoreGlobalIC()) return false;
           return true;
         case LookupIterator::INTERCEPTOR: {
           Handle<JSObject> holder = it->GetHolder<JSObject>();
@@ -1371,7 +1374,8 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle<Object> value,
 }
 
 MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
-                                         Handle<Object> value) {
+                                         Handle<Object> value,
+                                         bool should_update_feedback) {
   DCHECK(name->IsString());
 
   // Look up in script context table.
@@ -1398,7 +1402,8 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
       return ReferenceError(name);
     }
 
-    bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
+    bool use_ic =
+        (state() != NO_FEEDBACK) && FLAG_use_ic && should_update_feedback;
     if (use_ic) {
       if (nexus()->ConfigureLexicalVarMode(
               lookup_result.context_index, lookup_result.slot_index,
@@ -1416,12 +1421,14 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
     return value;
   }
 
-  return StoreIC::Store(global, name, value);
+  return StoreIC::Store(global, name, value, StoreOrigin::kNamed,
+                        should_update_feedback);
 }
 
 MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
                                    Handle<Object> value,
-                                   StoreOrigin store_origin) {
+                                   StoreOrigin store_origin,
+                                   bool should_update_feedback) {
   // TODO(verwaest): Let SetProperty do the migration, since storing a property
   // might deprecate the current map again, if value does not fit.
   if (MigrateDeprecated(isolate(), object)) {
@@ -1432,7 +1439,8 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
     return result;
   }
 
-  bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
+  bool use_ic =
+      (state() != NO_FEEDBACK) && FLAG_use_ic && should_update_feedback;
   // If the object is undefined or null it's illegal to try to set any
   // properties on it; throw a TypeError in that case.
   if (object->IsNullOrUndefined(isolate())) {
@@ -1465,7 +1473,8 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
   }
   if (use_ic) UpdateCaches(&it, value, store_origin);
 
-  MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
+  MAYBE_RETURN_NULL(Object::SetProperty(
+      &it, value, store_origin, Nothing<ShouldThrow>(), IsStoreGlobalIC()));
   return value;
 }
 
@@ -2355,36 +2364,9 @@ RUNTIME_FUNCTION(Runtime_StoreGlobalIC_Slow) {
   }
 #endif
 
-  Handle<JSGlobalObject> global = isolate->global_object();
-  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);
-    if (lookup_result.mode == VariableMode::kConst) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewTypeError(MessageTemplate::kConstAssign, global, name));
-    }
-
-    Handle<Object> previous_value(script_context->get(lookup_result.slot_index),
-                                  isolate);
-
-    if (previous_value->IsTheHole(isolate)) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewReferenceError(MessageTemplate::kNotDefined, name));
-    }
-
-    script_context->set(lookup_result.slot_index, *value);
-    return *value;
-  }
-
-  RETURN_RESULT_OR_FAILURE(
-      isolate, Runtime::SetObjectProperty(isolate, global, name, value,
-                                          StoreOrigin::kMaybeKeyed));
+  StoreGlobalIC ic(isolate, Handle<FeedbackVector>(), FeedbackSlot(),
+                   FeedbackSlotKind::kStoreGlobalStrict);
+  RETURN_RESULT_OR_FAILURE(isolate, ic.Store(name, value, false));
 }
 
 RUNTIME_FUNCTION(Runtime_KeyedStoreIC_Miss) {

src/ic/ic.h

@@ -251,7 +251,8 @@ class StoreIC : public IC {
 
   V8_WARN_UNUSED_RESULT MaybeHandle<Object> Store(
       Handle<Object> object, Handle<Name> name, Handle<Object> value,
-      StoreOrigin store_origin = StoreOrigin::kNamed);
+      StoreOrigin store_origin = StoreOrigin::kNamed,
+      bool should_update_feedback = true);
 
   bool LookupForWrite(LookupIterator* it, Handle<Object> value,
                       StoreOrigin store_origin);
@@ -275,8 +276,9 @@ class StoreGlobalIC : public StoreIC {
                 FeedbackSlot slot, FeedbackSlotKind kind)
       : StoreIC(isolate, vector, slot, kind) {}
 
-  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Store(Handle<Name> name,
-                                                  Handle<Object> value);
+  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Store(
+      Handle<Name> name, Handle<Object> value,
+      bool should_update_feedback = true);
 };
 
 enum KeyedStoreCheckMap { kDontCheckMap, kCheckMap };

src/interpreter/interpreter-generator.cc

@@ -214,7 +214,7 @@ IGNITION_HANDLER(LdaGlobalInsideTypeof, InterpreterLoadGlobalAssembler) {
 // StaGlobal <name_index> <slot>
 //
 // Store the value in the accumulator into the global with name in constant pool
-// entry <name_index> using FeedBackVector slot <slot>.
+// entry <name_index> using FeedbackVector slot <slot>.
 IGNITION_HANDLER(StaGlobal, InterpreterAssembler) {
   TNode<Context> context = GetContext();
 

src/objects/objects.cc

@@ -2439,7 +2439,8 @@ MaybeHandle<Object> Object::SetProperty(Isolate* isolate, Handle<Object> object,
 Maybe<bool> Object::SetPropertyInternal(LookupIterator* it,
                                         Handle<Object> value,
                                         Maybe<ShouldThrow> should_throw,
-                                        StoreOrigin store_origin, bool* found) {
+                                        StoreOrigin store_origin,
+                                        bool is_global_reference, bool* found) {
   it->UpdateProtector();
   DCHECK(it->IsFound());
 
@@ -2467,6 +2468,15 @@ Maybe<bool> Object::SetPropertyInternal(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()) return Nothing<bool>();
+          if (!maybe.FromJust()) {
+            *found = false;
+            return Nothing<bool>();
+          }
+        }
         return JSProxy::SetProperty(it->GetHolder<JSProxy>(), it->GetName(),
                                     value, receiver, should_throw);
       }
@@ -2552,11 +2562,12 @@ Maybe<bool> Object::SetPropertyInternal(LookupIterator* it,
 
 Maybe<bool> Object::SetProperty(LookupIterator* it, Handle<Object> value,
                                 StoreOrigin store_origin,
-                                Maybe<ShouldThrow> should_throw) {
+                                Maybe<ShouldThrow> should_throw,
+                                bool is_global_reference) {
   if (it->IsFound()) {
     bool found = true;
-    Maybe<bool> result =
-        SetPropertyInternal(it, value, should_throw, store_origin, &found);
+    Maybe<bool> result = SetPropertyInternal(
+        it, value, should_throw, store_origin, is_global_reference, &found);
     if (found) return result;
   }
 
@@ -2581,8 +2592,8 @@ Maybe<bool> Object::SetSuperProperty(LookupIterator* it, Handle<Object> value,
 
   if (it->IsFound()) {
     bool found = true;
-    Maybe<bool> result =
-        SetPropertyInternal(it, value, should_throw, store_origin, &found);
+    Maybe<bool> result = SetPropertyInternal(it, value, should_throw,
+                                             store_origin, false, &found);
     if (found) return result;
   }
 

src/objects/objects.h

@@ -470,7 +470,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
   GetProperty(LookupIterator* it, bool is_global_reference = false);
 
-  // ES6 [[Set]] (when passed kDontThrow)
+  // ES6 [[Set]] (when passed kDontThrow and is_global_reference is false)
   // Invariants for this and related functions (unless stated otherwise):
   // 1) When the result is Nothing, an exception is pending.
   // 2) When passed kThrowOnError, the result is never Just(false).
@@ -479,7 +479,8 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   // covered by it (eg., concerning API callbacks).
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe<bool> SetProperty(
       LookupIterator* it, Handle<Object> value, StoreOrigin store_origin,
-      Maybe<ShouldThrow> should_throw = Nothing<ShouldThrow>());
+      Maybe<ShouldThrow> should_throw = Nothing<ShouldThrow>(),
+      bool is_global_reference = false);
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
   SetProperty(Isolate* isolate, Handle<Object> object, Handle<Name> name,
               Handle<Object> value,
@@ -692,7 +693,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   // Return value is only meaningful if [found] is set to true on return.
   V8_WARN_UNUSED_RESULT static Maybe<bool> SetPropertyInternal(
       LookupIterator* it, Handle<Object> value, Maybe<ShouldThrow> should_throw,
-      StoreOrigin store_origin, bool* found);
+      StoreOrigin store_origin, bool is_global_reference, bool* found);
 
   V8_WARN_UNUSED_RESULT static MaybeHandle<Name> ConvertToName(
       Isolate* isolate, Handle<Object> input);

src/runtime/runtime-scopes.cc

@@ -931,22 +931,38 @@ MaybeHandle<Object> StoreLookupSlot(
   // Slow case: The property is not in a context slot.  It is either in a
   // context extension object, a property of the subject of a with, or a
   // property of the global object.
-  Handle<JSReceiver> object;
   if (attributes != ABSENT) {
     // The property exists on the holder.
-    object = Handle<JSReceiver>::cast(holder);
+    Handle<JSReceiver> object = Handle<JSReceiver>::cast(holder);
+
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate, value, Object::SetProperty(isolate, object, name, value),
+        Object);
   } else if (is_strict(language_mode)) {
     // If absent in strict mode: throw.
     THROW_NEW_ERROR(
         isolate, NewReferenceError(MessageTemplate::kNotDefined, name), Object);
   } else {
     // If absent in sloppy mode: add the property to the global object.
-    object = handle(context->global_object(), isolate);
+    Handle<JSObject> object(context->global_object(), isolate);
+
+    if (context_lookup_flags ==
+        static_cast<ContextLookupFlags>(DONT_FOLLOW_CHAINS)) {
+      ASSIGN_RETURN_ON_EXCEPTION(
+          isolate, value, Object::SetProperty(isolate, object, name, value),
+          Object);
+    } else {
+      // When we follow the context chain, we would have already done a lookup
+      // on the global object, so we should not call SetProperty which does a
+      // lookup again. This would be user visible when there are proxies.
+      LookupIterator it(isolate, object, name, object,
+                        LookupIterator::OWN_SKIP_INTERCEPTOR);
+      MAYBE_RETURN_NULL(JSObject::AddDataProperty(&it, value, NONE,
+                                                  Just(ShouldThrow::kDontThrow),
+                                                  StoreOrigin::kMaybeKeyed));
+    }
   }
 
-  ASSIGN_RETURN_ON_EXCEPTION(isolate, value,
-                             Object::SetProperty(isolate, object, name, value),
-                             Object);
   return value;
 }
 

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

@@ -13,6 +13,7 @@ var global = this;
   var proxy = new Proxy(target, {
     has(target, property) {
       calledHas = true;
+      if (property == 'makeGlobal') return true;
       return Reflect.has(target, property);
     },
     get(target, property, receiver) {

test/mjsunit/global-store-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 set_count = 0;
+var get_count = 0;
+var has_count = 0;
+var property_descriptor_count = 0;
+globalThis.__proto__ = new Proxy({},
+                                 {get() {get_count++},
+                                  has() {has_count++;},
+                                  set() {set_count++;},
+                                  getOwnPropertyDescriptor() {property_desciptor_count++}});
+function checkCounts(count) {
+  assertEquals(has_count, count);
+  assertEquals(set_count, 0);
+  assertEquals(get_count, 0);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function store_lookup_global_has_returns_false() {
+  eval("var b = 10");
+  return x = 10;
+}
+assertEquals(store_lookup_global_has_returns_false(), 10);
+checkCounts(1);
+
+%EnsureFeedbackVectorForFunction(store_lookup_global_has_returns_false);
+delete x;
+assertEquals(store_lookup_global_has_returns_false(), 10);
+checkCounts(2);
+delete x;
+assertEquals(store_lookup_global_has_returns_false(), 10);
+checkCounts(3);
+
+function store_global_has_returns_false(n) {
+  return x0 = 10;
+}
+assertEquals(store_global_has_returns_false(), 10);
+checkCounts(4);
+assertEquals("number", typeof(x));
+
+%EnsureFeedbackVectorForFunction(store_global_has_returns_false);
+delete x0;
+assertEquals(store_global_has_returns_false(), 10);
+checkCounts(5);
+delete x0;
+assertEquals(store_global_has_returns_false(), 10);
+checkCounts(6);
+
+
+// Check when the object is present on the proxy.
+get_count = 0;
+has_count = 0;
+set_count = 0;
+property_descriptor_count = 0;
+
+var proxy = new Proxy({}, {get() {get_count++;},
+                           has() {has_count++; return true;},
+                           set() {set_count++; return true; },
+                           getOwnPropertyDescriptor() {property_desciptor_count++}});
+Object.setPrototypeOf(globalThis, proxy);
+function checkCountsWithSet(count) {
+  assertEquals(has_count, count);
+  assertEquals(set_count, count);
+  assertEquals(get_count, 0);
+  assertEquals(property_descriptor_count, 0);
+}
+
+function store_lookup_global() {
+  eval("var b = 10");
+  return x1 = 10;
+}
+assertEquals(store_lookup_global(), 10);
+checkCountsWithSet(1);
+
+%EnsureFeedbackVectorForFunction(store_lookup_global);
+assertEquals(store_lookup_global(), 10);
+checkCountsWithSet(2);
+assertEquals(store_lookup_global(), 10);
+checkCountsWithSet(3);
+
+function store_global() {
+  return x1 = 10;
+}
+
+assertEquals(store_global(), 10);
+checkCountsWithSet(4);
+
+%EnsureFeedbackVectorForFunction(store_global);
+assertEquals(store_global(), 10);
+checkCountsWithSet(5);
+assertEquals(store_global(), 10);
+checkCountsWithSet(6);
+assertEquals("undefined", typeof(x1));
+
+// Check unbound variable access inside typeof
+get_count = 0;
+has_count = 0;
+set_count = 0;
+
+// Check that if has property returns true we don't have set trap.
+proxy = new Proxy({}, {has() {has_count++; return true;},
+                       getOwnPropertyDescriptor() {property_desciptor_count++}});
+Object.setPrototypeOf(globalThis, proxy);
+
+function store_global_no_set() {
+  return x2 = 10;
+}
+
+has_count = 3;
+assertEquals(store_global_no_set(), 10);
+checkCounts(4);
+assertEquals("number", typeof(x2));
+
+%EnsureFeedbackVectorForFunction(store_global_no_set);
+delete x2;
+assertEquals(store_global_no_set(), 10);
+checkCounts(5);
+delete x2;
+assertEquals(store_global_no_set(), 10);
+checkCounts(6);
+assertEquals("undefined", typeof(x1));

test/mjsunit/regress/regress-1018871.js

@@ -12,3 +12,24 @@ function f() {
   x;
 }
 assertThrows(f, RangeError);
+
+function f_store() {
+  var obj = {z: 0};
+  var proxy = new Proxy(obj, {
+    has() { z = 10; },
+  });
+  Object.setPrototypeOf(v_this, proxy);
+  z = 10;
+}
+assertThrows(f_store, RangeError);
+
+function f_set() {
+  var obj = {z: 0};
+  var proxy = new Proxy(obj, {
+    has() {return true; },
+    set() { z = x; }
+  });
+  Object.setPrototypeOf(v_this, proxy);
+  z = 10;
+}
+assertThrows(f_set, RangeError);