diff --git a/src/ic/ic.cc b/src/ic/ic.cc index 098af67998..5528f34fbe 100644 --- a/src/ic/ic.cc +++ b/src/ic/ic.cc @@ -1324,9 +1324,6 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle 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 holder = it->GetHolder(); @@ -1382,8 +1379,7 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle value, } MaybeHandle StoreGlobalIC::Store(Handle name, - Handle value, - bool should_update_feedback) { + Handle value) { DCHECK(name->IsString()); // Look up in script context table. @@ -1410,8 +1406,7 @@ MaybeHandle StoreGlobalIC::Store(Handle name, return ReferenceError(name); } - bool use_ic = - (state() != NO_FEEDBACK) && FLAG_use_ic && should_update_feedback; + bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic; if (use_ic) { if (nexus()->ConfigureLexicalVarMode( lookup_result.context_index, lookup_result.slot_index, @@ -1430,14 +1425,12 @@ MaybeHandle StoreGlobalIC::Store(Handle name, return value; } - return StoreIC::Store(global, name, value, StoreOrigin::kNamed, - should_update_feedback); + return StoreIC::Store(global, name, value); } MaybeHandle StoreIC::Store(Handle object, Handle name, Handle value, - StoreOrigin store_origin, - bool should_update_feedback) { + StoreOrigin store_origin) { // 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 StoreIC::Store(Handle object, Handle name, return result; } - bool use_ic = - (state() != NO_FEEDBACK) && FLAG_use_ic && should_update_feedback; + bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic; // 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 StoreIC::Store(Handle object, Handle name, : TraceIC("StoreIC", name); } - MAYBE_RETURN_NULL(Object::SetProperty( - &it, value, store_origin, Nothing(), IsStoreGlobalIC())); + MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin)); return value; } @@ -2379,9 +2370,36 @@ RUNTIME_FUNCTION(Runtime_StoreGlobalIC_Slow) { } #endif - StoreGlobalIC ic(isolate, Handle(), FeedbackSlot(), - FeedbackSlotKind::kStoreGlobalStrict); - RETURN_RESULT_OR_FAILURE(isolate, ic.Store(name, value, false)); + Handle global = isolate->global_object(); + Handle native_context = isolate->native_context(); + Handle script_contexts( + native_context->script_context_table(), isolate); + + ScriptContextTable::LookupResult lookup_result; + if (ScriptContextTable::Lookup(isolate, *script_contexts, *name, + &lookup_result)) { + Handle 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 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) { diff --git a/src/ic/ic.h b/src/ic/ic.h index 5acfbb967d..6e17517dcc 100644 --- a/src/ic/ic.h +++ b/src/ic/ic.h @@ -251,8 +251,7 @@ class StoreIC : public IC { V8_WARN_UNUSED_RESULT MaybeHandle Store( Handle object, Handle name, Handle value, - StoreOrigin store_origin = StoreOrigin::kNamed, - bool should_update_feedback = true); + StoreOrigin store_origin = StoreOrigin::kNamed); bool LookupForWrite(LookupIterator* it, Handle 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 Store( - Handle name, Handle value, - bool should_update_feedback = true); + V8_WARN_UNUSED_RESULT MaybeHandle Store(Handle name, + Handle value); }; enum KeyedStoreCheckMap { kDontCheckMap, kCheckMap }; diff --git a/src/interpreter/interpreter-generator.cc b/src/interpreter/interpreter-generator.cc index 0f39765cb8..5f686f86b8 100644 --- a/src/interpreter/interpreter-generator.cc +++ b/src/interpreter/interpreter-generator.cc @@ -214,7 +214,7 @@ IGNITION_HANDLER(LdaGlobalInsideTypeof, InterpreterLoadGlobalAssembler) { // StaGlobal // // Store the value in the accumulator into the global with name in constant pool -// entry using FeedbackVector slot . +// entry using FeedBackVector slot . IGNITION_HANDLER(StaGlobal, InterpreterAssembler) { TNode context = GetContext(); diff --git a/src/objects/objects.cc b/src/objects/objects.cc index 02dced5609..505e67338f 100644 --- a/src/objects/objects.cc +++ b/src/objects/objects.cc @@ -2439,8 +2439,7 @@ MaybeHandle Object::SetProperty(Isolate* isolate, Handle object, Maybe Object::SetPropertyInternal(LookupIterator* it, Handle value, Maybe should_throw, - StoreOrigin store_origin, - bool is_global_reference, bool* found) { + StoreOrigin store_origin, bool* found) { it->UpdateProtector(); DCHECK(it->IsFound()); @@ -2468,15 +2467,6 @@ Maybe Object::SetPropertyInternal(LookupIterator* it, receiver = handle(JSGlobalObject::cast(*receiver).global_proxy(), it->isolate()); } - if (is_global_reference) { - Maybe maybe = JSProxy::HasProperty( - it->isolate(), it->GetHolder(), it->GetName()); - if (maybe.IsNothing()) return Nothing(); - if (!maybe.FromJust()) { - *found = false; - return Nothing(); - } - } return JSProxy::SetProperty(it->GetHolder(), it->GetName(), value, receiver, should_throw); } @@ -2562,12 +2552,11 @@ Maybe Object::SetPropertyInternal(LookupIterator* it, Maybe Object::SetProperty(LookupIterator* it, Handle value, StoreOrigin store_origin, - Maybe should_throw, - bool is_global_reference) { + Maybe should_throw) { if (it->IsFound()) { bool found = true; - Maybe result = SetPropertyInternal( - it, value, should_throw, store_origin, is_global_reference, &found); + Maybe result = + SetPropertyInternal(it, value, should_throw, store_origin, &found); if (found) return result; } @@ -2592,8 +2581,8 @@ Maybe Object::SetSuperProperty(LookupIterator* it, Handle value, if (it->IsFound()) { bool found = true; - Maybe result = SetPropertyInternal(it, value, should_throw, - store_origin, false, &found); + Maybe result = + SetPropertyInternal(it, value, should_throw, store_origin, &found); if (found) return result; } diff --git a/src/objects/objects.h b/src/objects/objects.h index 223a844333..1383e5708e 100644 --- a/src/objects/objects.h +++ b/src/objects/objects.h @@ -470,7 +470,7 @@ class Object : public TaggedImpl { V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle 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 { // covered by it (eg., concerning API callbacks). V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe SetProperty( LookupIterator* it, Handle value, StoreOrigin store_origin, - Maybe should_throw = Nothing(), - bool is_global_reference = false); + Maybe should_throw = Nothing()); V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle SetProperty(Isolate* isolate, Handle object, Handle name, Handle value, @@ -693,7 +692,7 @@ class Object : public TaggedImpl { // Return value is only meaningful if [found] is set to true on return. V8_WARN_UNUSED_RESULT static Maybe SetPropertyInternal( LookupIterator* it, Handle value, Maybe should_throw, - StoreOrigin store_origin, bool is_global_reference, bool* found); + StoreOrigin store_origin, bool* found); V8_WARN_UNUSED_RESULT static MaybeHandle ConvertToName( Isolate* isolate, Handle input); diff --git a/src/runtime/runtime-scopes.cc b/src/runtime/runtime-scopes.cc index 4a6c42dab1..36326c931c 100644 --- a/src/runtime/runtime-scopes.cc +++ b/src/runtime/runtime-scopes.cc @@ -931,38 +931,22 @@ MaybeHandle 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 object; if (attributes != ABSENT) { // The property exists on the holder. - Handle object = Handle::cast(holder); - - ASSIGN_RETURN_ON_EXCEPTION( - isolate, value, Object::SetProperty(isolate, object, name, value), - Object); + object = Handle::cast(holder); } 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 object(context->global_object(), isolate); - - if (context_lookup_flags == - static_cast(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)); - } + object = handle(context->global_object(), isolate); } + ASSIGN_RETURN_ON_EXCEPTION(isolate, value, + Object::SetProperty(isolate, object, name, value), + Object); return value; } diff --git a/test/mjsunit/es6/global-proto-proxy.js b/test/mjsunit/es6/global-proto-proxy.js index 0605e5d370..1fcb9fb434 100644 --- a/test/mjsunit/es6/global-proto-proxy.js +++ b/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) { diff --git a/test/mjsunit/global-store-with-proxy.js b/test/mjsunit/global-store-with-proxy.js deleted file mode 100644 index 2d829b670e..0000000000 --- a/test/mjsunit/global-store-with-proxy.js +++ /dev/null @@ -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)); diff --git a/test/mjsunit/regress/regress-1018871.js b/test/mjsunit/regress/regress-1018871.js index 4dbecdca55..302429de2a 100644 --- a/test/mjsunit/regress/regress-1018871.js +++ b/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);