diff --git a/src/builtins/builtins-internal-gen.cc b/src/builtins/builtins-internal-gen.cc index c2e9767f30..b4adea4124 100644 --- a/src/builtins/builtins-internal-gen.cc +++ b/src/builtins/builtins-internal-gen.cc @@ -544,8 +544,8 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) { Label dictionary(this), dont_delete(this); GotoIf(IsDictionaryMap(receiver_map), &dictionary); - // Fast properties need to clear recorded slots, which can only be done - // in C++. + // Fast properties need to clear recorded slots and mark the deleted + // property as mutable, which can only be done in C++. Goto(&slow); BIND(&dictionary); diff --git a/src/objects/map-inl.h b/src/objects/map-inl.h index be106a61ba..d715dfac6c 100644 --- a/src/objects/map-inl.h +++ b/src/objects/map-inl.h @@ -190,6 +190,10 @@ bool Map::TooManyFastProperties(StoreOrigin store_origin) const { } } +Name Map::GetLastDescriptorName(Isolate* isolate) const { + return instance_descriptors(isolate).GetKey(LastAdded()); +} + PropertyDetails Map::GetLastDescriptorDetails(Isolate* isolate) const { return instance_descriptors(isolate).GetDetails(LastAdded()); } diff --git a/src/objects/map-updater.cc b/src/objects/map-updater.cc index 6a26dd2acf..820edd1e83 100644 --- a/src/objects/map-updater.cc +++ b/src/objects/map-updater.cc @@ -192,49 +192,6 @@ Handle MapUpdater::ReconfigureToDataField(InternalIndex descriptor, PropertyDetails old_details = old_descriptors_->GetDetails(modified_descriptor_); - // If the {descriptor} was "const" data field so far, we need to update the - // {old_map_} here, otherwise we could get the constants wrong, i.e. - // - // o.x = 1; - // change o.x's attributes to something else - // delete o.x; - // o.x = 2; - // - // could trick V8 into thinking that `o.x` is still 1 even after the second - // assignment. - // This situation is similar to what might happen with property deletion. - if (old_details.constness() == PropertyConstness::kConst && - old_details.location() == kField && - old_details.attributes() != new_attributes_) { - // Ensure we'll be updating constness of the up-to-date version of old_map_. - Handle old_map = UpdateMapNoLock(isolate_, old_map_); - PropertyDetails details = - old_map->instance_descriptors(isolate_).GetDetails(descriptor); - Handle field_type( - old_map->instance_descriptors(isolate_).GetFieldType(descriptor), - isolate_); - GeneralizeField(isolate_, old_map, descriptor, PropertyConstness::kMutable, - details.representation(), field_type); - DCHECK_EQ(PropertyConstness::kMutable, - old_map->instance_descriptors(isolate_) - .GetDetails(descriptor) - .constness()); - // The old_map_'s property must become mutable. - // Note, that the {old_map_} and {old_descriptors_} are not expected to be - // updated by the generalization if the map is already deprecated. - DCHECK_IMPLIES( - !old_map_->is_deprecated(), - PropertyConstness::kMutable == - old_descriptors_->GetDetails(modified_descriptor_).constness()); - // Although the property in the old map is marked as mutable we still - // treat it as constant when merging with the new path in transition tree. - // This is fine because up until this reconfiguration the field was - // known to be constant, so it's fair to proceed treating it as such - // during this reconfiguration session. The issue is that after the - // reconfiguration the original field might become mutable (see the delete - // example above). - } - // If property kind is not reconfigured merge the result with // representation/field type from the old descriptor. if (old_details.kind() == new_kind_) { @@ -1051,6 +1008,8 @@ void MapUpdater::GeneralizeField(Isolate* isolate, Handle map, PropertyConstness new_constness, Representation new_representation, Handle new_field_type) { + DCHECK(!map->is_deprecated()); + // Check if we actually need to generalize the field type at all. Handle old_descriptors(map->instance_descriptors(isolate), isolate); diff --git a/src/objects/map.h b/src/objects/map.h index cb2ac4478b..33d9394b43 100644 --- a/src/objects/map.h +++ b/src/objects/map.h @@ -611,6 +611,7 @@ class Map : public HeapObject { // chain state. inline bool IsPrototypeValidityCellValid() const; + inline Name GetLastDescriptorName(Isolate* isolate) const; inline PropertyDetails GetLastDescriptorDetails(Isolate* isolate) const; inline InternalIndex LastAdded() const; diff --git a/src/objects/property-details.h b/src/objects/property-details.h index bab6e297e4..1b856a1257 100644 --- a/src/objects/property-details.h +++ b/src/objects/property-details.h @@ -32,6 +32,12 @@ enum PropertyAttributes { // a non-existent property. }; +// Number of distinct bits in PropertyAttributes. +static const int kPropertyAttributesBitsCount = 3; + +static const int kPropertyAttributesCombinationsCount = + 1 << kPropertyAttributesBitsCount; + enum PropertyFilter { ALL_PROPERTIES = 0, ONLY_WRITABLE = 1, @@ -63,6 +69,11 @@ STATIC_ASSERT(SKIP_STRINGS == STATIC_ASSERT(SKIP_SYMBOLS == static_cast(v8::PropertyFilter::SKIP_SYMBOLS)); +// Assert that kPropertyAttributesBitsCount value matches the definition of +// ALL_ATTRIBUTES_MASK. +STATIC_ASSERT((ALL_ATTRIBUTES_MASK == (READ_ONLY | DONT_ENUM | DONT_DELETE)) == + (kPropertyAttributesBitsCount == 3)); + class Smi; class TypeInfo; diff --git a/src/objects/transitions.cc b/src/objects/transitions.cc index edfba3adfb..ac908030a2 100644 --- a/src/objects/transitions.cc +++ b/src/objects/transitions.cc @@ -270,6 +270,34 @@ MaybeHandle TransitionsAccessor::FindTransitionToDataProperty( return Handle(target, isolate_); } +void TransitionsAccessor::ForEachTransitionTo( + Name name, const ForEachTransitionCallback& callback, + DisallowGarbageCollection* no_gc) { + DCHECK(name.IsUniqueName()); + switch (encoding()) { + case kPrototypeInfo: + case kUninitialized: + case kMigrationTarget: + return; + case kWeakRef: { + Map target = Map::cast(raw_transitions_->GetHeapObjectAssumeWeak()); + InternalIndex descriptor = target.LastAdded(); + DescriptorArray descriptors = target.instance_descriptors(kRelaxedLoad); + Name key = descriptors.GetKey(descriptor); + if (key == name) { + callback(target); + } + return; + } + case kFullTransitionArray: { + base::SharedMutexGuardIf scope( + isolate_->full_transition_array_access(), concurrent_access_); + return transitions().ForEachTransitionTo(name, callback); + } + } + UNREACHABLE(); +} + bool TransitionsAccessor::CanHaveMoreTransitions() { if (map_.is_dictionary_map()) return false; if (encoding() == kFullTransitionArray) { @@ -613,6 +641,21 @@ Map TransitionArray::SearchAndGetTarget(PropertyKind kind, Name name, return SearchDetailsAndGetTarget(transition, kind, attributes); } +void TransitionArray::ForEachTransitionTo( + Name name, const ForEachTransitionCallback& callback) { + int transition = SearchName(name, nullptr); + if (transition == kNotFound) return; + + int nof_transitions = number_of_transitions(); + DCHECK(transition < nof_transitions); + Name key = GetKey(transition); + for (; transition < nof_transitions && GetKey(transition) == key; + transition++) { + Map target = GetTarget(transition); + callback(target); + } +} + void TransitionArray::Sort() { DisallowGarbageCollection no_gc; // In-place insertion sort. diff --git a/src/objects/transitions.h b/src/objects/transitions.h index 4f992bc6cf..237cfcd0ef 100644 --- a/src/objects/transitions.h +++ b/src/objects/transitions.h @@ -19,6 +19,9 @@ namespace v8 { namespace internal { +// Find all transitions with given name and calls the callback. +using ForEachTransitionCallback = std::function; + // TransitionsAccessor is a helper class to encapsulate access to the various // ways a Map can store transitions to other maps in its respective field at // Map::kTransitionsOrPrototypeInfo. @@ -68,6 +71,14 @@ class V8_EXPORT_PRIVATE TransitionsAccessor { return FindTransitionToDataProperty(name, kFieldOnly); } + // Find all transitions with given name and calls the callback. + // Neither GCs nor operations requiring Isolate::full_transition_array_access + // lock are allowed inside the callback. + // If any of the GC- or lock-requiring processing is necessary, it has to be + // done outside of the callback. + void ForEachTransitionTo(Name name, const ForEachTransitionCallback& callback, + DisallowGarbageCollection* no_gc); + inline Handle ExpectedTransitionKey(); inline Handle ExpectedTransitionTarget(); @@ -320,6 +331,10 @@ class TransitionArray : public WeakFixedArray { Map SearchDetailsAndGetTarget(int transition, PropertyKind kind, PropertyAttributes attributes); + // Find all transitions with given name and calls the callback. + void ForEachTransitionTo(Name name, + const ForEachTransitionCallback& callback); + inline int number_of_transitions() const; static bool CompactPrototypeTransitionArray(Isolate* isolate, diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc index 4c80ad8b77..56f3af33c6 100644 --- a/src/runtime/runtime-object.cc +++ b/src/runtime/runtime-object.cc @@ -19,6 +19,7 @@ #include "src/objects/map-updater.h" #include "src/objects/property-descriptor-object.h" #include "src/objects/property-descriptor.h" +#include "src/objects/property-details.h" #include "src/objects/swiss-name-dictionary-inl.h" #include "src/runtime/runtime-utils.h" #include "src/runtime/runtime.h" @@ -118,6 +119,51 @@ inline void ClearField(Isolate* isolate, JSObject object, FieldIndex index) { } } +void GeneralizeAllTransitionsToFieldAsMutable(Isolate* isolate, Handle map, + Handle name) { + InternalIndex descriptor(map->NumberOfOwnDescriptors()); + + Handle target_maps[kPropertyAttributesCombinationsCount]; + int target_maps_count = 0; + + // Collect all outgoing field transitions. + { + DisallowGarbageCollection no_gc; + TransitionsAccessor transitions(isolate, *map, &no_gc); + transitions.ForEachTransitionTo( + *name, + [&](Map target) { + DCHECK_EQ(descriptor, target.LastAdded()); + DCHECK_EQ(*name, target.GetLastDescriptorName(isolate)); + PropertyDetails details = target.GetLastDescriptorDetails(isolate); + // Currently, we track constness only for fields. + if (details.kind() == kData && + details.constness() == PropertyConstness::kConst) { + target_maps[target_maps_count++] = handle(target, isolate); + } + DCHECK_IMPLIES(details.kind() == kAccessor, + details.constness() == PropertyConstness::kConst); + }, + &no_gc); + CHECK_LE(target_maps_count, kPropertyAttributesCombinationsCount); + } + + for (int i = 0; i < target_maps_count; i++) { + Handle target = target_maps[i]; + PropertyDetails details = + target->instance_descriptors(isolate).GetDetails(descriptor); + Handle field_type( + target->instance_descriptors(isolate).GetFieldType(descriptor), + isolate); + MapUpdater::GeneralizeField(isolate, target, descriptor, + PropertyConstness::kMutable, + details.representation(), field_type); + DCHECK_EQ(PropertyConstness::kMutable, target->instance_descriptors(isolate) + .GetDetails(descriptor) + .constness()); + } +} + bool DeleteObjectPropertyFast(Isolate* isolate, Handle receiver, Handle raw_key) { // This implements a special case for fast property deletion: when the @@ -127,6 +173,8 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle receiver, // (1) The receiver must be a regular object and the key a unique name. Handle receiver_map(receiver->map(), isolate); if (receiver_map->IsSpecialReceiverMap()) return false; + DCHECK(receiver_map->IsJSObjectMap()); + if (!raw_key->IsUniqueName()) return false; Handle key = Handle::cast(raw_key); // (2) The property to be deleted must be the last property. @@ -149,26 +197,6 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle receiver, // Preconditions successful. No more bailouts after this point. - // If the {descriptor} was "const" so far, we need to update the - // {receiver_map} here, otherwise we could get the constants wrong, i.e. - // - // o.x = 1; - // delete o.x; - // o.x = 2; - // - // could trick V8 into thinking that `o.x` is still 1 even after the second - // assignment. - if (details.constness() == PropertyConstness::kConst && - details.location() == kField) { - Handle field_type(descriptors->GetFieldType(descriptor), - isolate); - MapUpdater::GeneralizeField(isolate, receiver_map, descriptor, - PropertyConstness::kMutable, - details.representation(), field_type); - DCHECK_EQ(PropertyConstness::kMutable, - descriptors->GetDetails(descriptor).constness()); - } - // Zap the property to avoid keeping objects alive. Zapping is not necessary // for properties stored in the descriptor array. if (details.location() == kField) { @@ -216,6 +244,30 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle receiver, receiver->HeapObjectVerify(isolate); receiver->property_array().PropertyArrayVerify(isolate); #endif + + // If the {descriptor} was "const" so far, we need to update the + // {receiver_map} here, otherwise we could get the constants wrong, i.e. + // + // o.x = 1; + // [change o.x's attributes or reconfigure property kind] + // delete o.x; + // o.x = 2; + // + // could trick V8 into thinking that `o.x` is still 1 even after the second + // assignment. + + // Step 1: Migrate object to an up-to-date shape. + if (parent_map->is_deprecated()) { + JSObject::MigrateInstance(isolate, Handle::cast(receiver)); + parent_map = handle(receiver->map(), isolate); + } + + // Step 2: Mark outgoing transitions from the up-to-date version of the + // parent_map to same property name of any kind or attributes as mutable. + // Also migrate object to the up-to-date map to make the object shapes + // converge sooner. + GeneralizeAllTransitionsToFieldAsMutable(isolate, parent_map, key); + return true; } diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 12bdcf3ab7..835b6e2031 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -777,9 +777,9 @@ class Runtime : public AllStatic { // Get the runtime intrinsic function table. static const Function* RuntimeFunctionTable(Isolate* isolate); - V8_WARN_UNUSED_RESULT static Maybe DeleteObjectProperty( - Isolate* isolate, Handle receiver, Handle key, - LanguageMode language_mode); + V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe + DeleteObjectProperty(Isolate* isolate, Handle receiver, + Handle key, LanguageMode language_mode); V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle SetObjectProperty(Isolate* isolate, Handle object, Handle key, diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc index 100f3126d0..8ae7be6233 100644 --- a/test/cctest/test-field-type-tracking.cc +++ b/test/cctest/test-field-type-tracking.cc @@ -14,8 +14,10 @@ #include "src/init/v8.h" #include "src/objects/field-type.h" #include "src/objects/heap-number-inl.h" +#include "src/objects/internal-index.h" #include "src/objects/map-updater.h" #include "src/objects/objects-inl.h" +#include "src/objects/property-details.h" #include "src/objects/property.h" #include "src/objects/struct-inl.h" #include "src/objects/transitions.h" @@ -1122,16 +1124,7 @@ void TestReconfigureDataFieldAttribute_GeneralizeField( CHECK(!map2->is_stable()); CHECK(!map2->is_deprecated()); CHECK_NE(*map2, *new_map); - // If the "source" property was const then update constness expectations for - // "source" map and ensure the deoptimization dependency was triggered. - if (to.constness == PropertyConstness::kConst) { - expectations2.SetDataField(kSplitProp, READ_ONLY, - PropertyConstness::kMutable, to.representation, - to.type); - CHECK(code_src_field_const->marked_for_deoptimization()); - } else { - CHECK(!code_src_field_const->marked_for_deoptimization()); - } + CHECK(!code_src_field_const->marked_for_deoptimization()); CHECK(expectations2.Check(*map2)); for (int i = kSplitProp; i < kPropCount; i++) { @@ -3045,6 +3038,122 @@ TEST(RepresentationPredicatesAreInSync) { } } +TEST(DeletePropertyGeneralizesConstness) { + CcTest::InitializeVM(); + v8::HandleScope scope(CcTest::isolate()); + Isolate* isolate = CcTest::i_isolate(); + Handle any_type = FieldType::Any(isolate); + + // Create a map with some properties. + Handle initial_map = Map::Create(isolate, kPropCount + 3); + Handle map = initial_map; + for (int i = 0; i < kPropCount; i++) { + Handle name = CcTest::MakeName("prop", i); + map = Map::CopyWithField(isolate, map, name, any_type, NONE, + PropertyConstness::kConst, Representation::Smi(), + INSERT_TRANSITION) + .ToHandleChecked(); + } + Handle parent_map = map; + CHECK(!map->is_deprecated()); + + Handle name_x = CcTest::MakeString("x"); + Handle name_y = CcTest::MakeString("y"); + + map = Map::CopyWithField(isolate, parent_map, name_x, any_type, NONE, + PropertyConstness::kConst, Representation::Smi(), + INSERT_TRANSITION) + .ToHandleChecked(); + + // Create an object, initialize its properties and add a couple of clones. + Handle object1 = isolate->factory()->NewJSObjectFromMap(map); + for (int i = 0; i < kPropCount; i++) { + FieldIndex index = FieldIndex::ForDescriptor(*map, InternalIndex(i)); + object1->FastPropertyAtPut(index, Smi::FromInt(i)); + } + Handle object2 = isolate->factory()->CopyJSObject(object1); + + CHECK(!map->is_deprecated()); + CHECK(!parent_map->is_deprecated()); + + // Transition to Double must deprecate m1. + CHECK(!Representation::Smi().CanBeInPlaceChangedTo(Representation::Double())); + + // Reconfigure one of the first properties to make the whole transition tree + // deprecated (including |parent_map| and |map|). + Handle new_map = + ReconfigureProperty(isolate, map, InternalIndex(0), PropertyKind::kData, + NONE, Representation::Double(), any_type); + CHECK(map->is_deprecated()); + CHECK(parent_map->is_deprecated()); + CHECK(!new_map->is_deprecated()); + // The "x" property is still kConst. + CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(), + PropertyConstness::kConst); + + Handle new_parent_map = Map::Update(isolate, parent_map); + CHECK(!new_parent_map->is_deprecated()); + + // |new_parent_map| must have exactly one outgoing transition to |new_map|. + { + TransitionsAccessor ta(isolate, new_parent_map); + CHECK_EQ(ta.NumberOfTransitions(), 1); + CHECK_EQ(ta.GetTarget(0), *new_map); + } + + // Deletion of the property from |object1| must migrate it to |new_parent_map| + // which is an up-to-date version of the |parent_map|. The |new_map|'s "x" + // property should be marked as mutable. + CHECK_EQ(object1->map(isolate), *map); + CHECK(Runtime::DeleteObjectProperty(isolate, object1, name_x, + LanguageMode::kSloppy) + .ToChecked()); + CHECK_EQ(object1->map(isolate), *new_parent_map); + CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(), + PropertyConstness::kMutable); + + // Now add transitions to "x" and "y" properties from |new_parent_map|. + std::vector> transitions; + Handle value = handle(Smi::FromInt(0), isolate); + for (int i = 0; i < kPropertyAttributesCombinationsCount; i++) { + PropertyAttributes attributes = static_cast(i); + + Handle tmp; + // Add some transitions to "x" and "y". + tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_x, value, + attributes, PropertyConstness::kConst, + StoreOrigin::kNamed); + CHECK(!tmp->map(isolate).is_dictionary_map()); + transitions.push_back(tmp); + + tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_y, value, + attributes, PropertyConstness::kConst, + StoreOrigin::kNamed); + CHECK(!tmp->map(isolate).is_dictionary_map()); + transitions.push_back(tmp); + } + + // Deletion of the property from |object2| must migrate it to |new_parent_map| + // which is an up-to-date version of the |parent_map|. + // All outgoing transitions from |new_map| that add "x" must be marked as + // mutable, transitions to other properties must remain const. + CHECK_EQ(object2->map(isolate), *map); + CHECK(Runtime::DeleteObjectProperty(isolate, object2, name_x, + LanguageMode::kSloppy) + .ToChecked()); + CHECK_EQ(object2->map(isolate), *new_parent_map); + for (Handle m : transitions) { + if (m->GetLastDescriptorName(isolate) == *name_x) { + CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(), + PropertyConstness::kMutable); + + } else { + CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(), + PropertyConstness::kConst); + } + } +} + } // namespace test_field_type_tracking } // namespace compiler } // namespace internal diff --git a/test/mjsunit/regress/regress-crbug-1195331.js b/test/mjsunit/regress/regress-crbug-1195331.js index 1bced5623e..9f10604e76 100644 --- a/test/mjsunit/regress/regress-crbug-1195331.js +++ b/test/mjsunit/regress/regress-crbug-1195331.js @@ -27,9 +27,9 @@ assertFalse(%HasOwnConstDataProperty(o3, "b")); Object.defineProperty(o2, "a", { value:2, enumerable: false, configurable: true, writable: true, }); -assertFalse(%HasOwnConstDataProperty(o1, "a")); +assertTrue(%HasOwnConstDataProperty(o1, "a")); assertFalse(%HasOwnConstDataProperty(o1, "b")); -assertFalse(%HasOwnConstDataProperty(o3, "a")); +assertTrue(%HasOwnConstDataProperty(o3, "a")); assertFalse(%HasOwnConstDataProperty(o3, "b")); assertFalse(%HasOwnConstDataProperty(o2, "a"));