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

This reverts commit b8ac4eb4dc.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1020533

Original change's description:
> [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}

TBR=mythria@chromium.org,verwaest@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

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

src/ic/ic.cc

@@ -1324,9 +1324,6 @@ 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>();
@@ -1382,8 +1379,7 @@ 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.
@@ -1410,8 +1406,7 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
       return ReferenceError(name);
     }
 
-    bool use_ic =
+    bool use_ic = (state() != NO_FEEDBACK) && FLAG_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,
@@ -1430,14 +1425,12 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
     return value;
   }
 
-  return StoreIC::Store(global, name, value, StoreOrigin::kNamed,
+  return StoreIC::Store(global, name, value);
-                        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)) {
@@ -1448,8 +1441,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
     return result;
   }
 
-  bool use_ic =
+  bool use_ic = (state() != NO_FEEDBACK) && FLAG_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())) {
@@ -1488,8 +1480,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
                       : TraceIC("StoreIC", name);
   }
 
-  MAYBE_RETURN_NULL(Object::SetProperty(
+  MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
-      &it, value, store_origin, Nothing<ShouldThrow>(), IsStoreGlobalIC()));
   return value;
 }
 
@@ -2379,9 +2370,36 @@ RUNTIME_FUNCTION(Runtime_StoreGlobalIC_Slow) {
   }
 #endif
 
-  StoreGlobalIC ic(isolate, Handle<FeedbackVector>(), FeedbackSlot(),
+  Handle<JSGlobalObject> global = isolate->global_object();
-                   FeedbackSlotKind::kStoreGlobalStrict);
+  Handle<Context> native_context = isolate->native_context();
-  RETURN_RESULT_OR_FAILURE(isolate, ic.Store(name, value, false));
+  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));
 }
 
 RUNTIME_FUNCTION(Runtime_KeyedStoreIC_Miss) {

src/ic/ic.h

@@ -251,8 +251,7 @@ 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);
@@ -276,9 +275,8 @@ class StoreGlobalIC : public StoreIC {
                 FeedbackSlot slot, FeedbackSlotKind kind)
       : StoreIC(isolate, vector, slot, kind) {}
 
-  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Store(
+  V8_WARN_UNUSED_RESULT MaybeHandle<Object> Store(Handle<Name> name,
-      Handle<Name> name, Handle<Object> value,
+                                                  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,8 +2439,7 @@ 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,
+                                        StoreOrigin store_origin, bool* found) {
-                                        bool is_global_reference, bool* found) {
   it->UpdateProtector();
   DCHECK(it->IsFound());
 
@@ -2468,15 +2467,6 @@ 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);
       }
@@ -2562,12 +2552,11 @@ 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(
+    Maybe<bool> result =
-        it, value, should_throw, store_origin, is_global_reference, &found);
+        SetPropertyInternal(it, value, should_throw, store_origin, &found);
     if (found) return result;
   }
 
@@ -2592,8 +2581,8 @@ Maybe<bool> Object::SetSuperProperty(LookupIterator* it, Handle<Object> value,
 
   if (it->IsFound()) {
     bool found = true;
-    Maybe<bool> result = SetPropertyInternal(it, value, should_throw,
+    Maybe<bool> result =
-                                             store_origin, false, &found);
+        SetPropertyInternal(it, value, should_throw, store_origin, &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 and is_global_reference is false)
+  // ES6 [[Set]] (when passed kDontThrow)
   // 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,8 +479,7 @@ 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,
@@ -693,7 +692,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 is_global_reference, bool* found);
+      StoreOrigin store_origin, bool* found);
 
   V8_WARN_UNUSED_RESULT static MaybeHandle<Name> ConvertToName(
       Isolate* isolate, Handle<Object> input);

src/runtime/runtime-scopes.cc

@@ -931,38 +931,22 @@ 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.
-    Handle<JSReceiver> object = Handle<JSReceiver>::cast(holder);
+    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.
-    Handle<JSObject> object(context->global_object(), isolate);
+    object = handle(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,7 +13,6 @@ 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

@@ -1,125 +0,0 @@
-// 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,24 +12,3 @@ 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);