[map] Update map in PrepareForDataProperty

Deprecated maps might not be updated before being passed to
PrepareForDataProperty. If the target map is a dictionary map,
then adding the data property can fail.

As a drive-by, remove the dead ForTransitionHandler code, which
was another (potentially unsafe) caller of PrepareForDataProperty

Bug: chromium:977012
Change-Id: I894bbc9bca2001555474a3570eb03fe6b0f69ddd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1674029
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62377}

src/objects/lookup.cc

@@ -80,50 +80,6 @@ LookupIterator LookupIterator::PropertyOrElement(Isolate* isolate,
   return LookupIterator(isolate, receiver, name, configuration);
 }
 
-// TODO(ishell): Consider removing this way of LookupIterator creation.
-// static
-LookupIterator LookupIterator::ForTransitionHandler(
-    Isolate* isolate, Handle<Object> receiver, Handle<Name> name,
-    Handle<Object> value, MaybeHandle<Map> maybe_transition_map) {
-  Handle<Map> transition_map;
-  if (!maybe_transition_map.ToHandle(&transition_map) ||
-      !transition_map->IsPrototypeValidityCellValid()) {
-    // This map is not a valid transition handler, so full lookup is required.
-    return LookupIterator(isolate, receiver, name);
-  }
-
-  PropertyDetails details = PropertyDetails::Empty();
-  bool has_property;
-  if (transition_map->is_dictionary_map()) {
-    details = PropertyDetails(kData, NONE, PropertyCellType::kNoCell);
-    has_property = false;
-  } else {
-    details = transition_map->GetLastDescriptorDetails();
-    has_property = true;
-  }
-#ifdef DEBUG
-  if (name->IsPrivate()) {
-    DCHECK_EQ(DONT_ENUM, details.attributes());
-  } else {
-    DCHECK_EQ(NONE, details.attributes());
-  }
-#endif
-  LookupIterator it(isolate, receiver, name, transition_map, details,
-                    has_property);
-
-  if (!transition_map->is_dictionary_map()) {
-    int descriptor_number = transition_map->LastAdded();
-    Handle<Map> new_map =
-        Map::PrepareForDataProperty(isolate, transition_map, descriptor_number,
-                                    PropertyConstness::kConst, value);
-    // Reload information; this is no-op if nothing changed.
-    it.property_details_ =
-        new_map->instance_descriptors().GetDetails(descriptor_number);
-    it.transition_ = new_map;
-  }
-  return it;
-}
-
 LookupIterator::LookupIterator(Isolate* isolate, Handle<Object> receiver,
                                Handle<Name> name, Handle<Map> transition_map,
                                PropertyDetails details, bool has_property)
@@ -486,8 +442,12 @@ void LookupIterator::PrepareForDataProperty(Handle<Object> value) {
   }
 
   Handle<Map> old_map(holder_obj->map(), isolate_);
-  Handle<Map> new_map = Map::PrepareForDataProperty(
-      isolate(), old_map, descriptor_number(), new_constness, value);
+  DCHECK(!old_map->is_dictionary_map());
+
+  Handle<Map> new_map = Map::Update(isolate_, old_map);
+  if (!new_map->is_dictionary_map()) {
+    new_map = Map::PrepareForDataProperty(
+        isolate(), new_map, descriptor_number(), new_constness, value);
 
     if (old_map.is_identical_to(new_map)) {
       // Update the property details if the representation was None.
@@ -497,6 +457,10 @@ void LookupIterator::PrepareForDataProperty(Handle<Object> value) {
       }
       return;
     }
+  }
+  // We should only get here if the new_map is different from the old map,
+  // otherwise we would have falled through to the is_identical_to check above.
+  DCHECK_NE(*old_map, *new_map);
 
   JSObject::MigrateToMap(isolate_, holder_obj, new_map);
   ReloadPropertyInformation<false>();

src/objects/lookup.h

@@ -93,10 +93,6 @@ class V8_EXPORT_PRIVATE LookupIterator final {
       Isolate* isolate, Handle<Object> receiver, Handle<Object> key,
       bool* success, Configuration configuration = DEFAULT);
 
-  static LookupIterator ForTransitionHandler(
-      Isolate* isolate, Handle<Object> receiver, Handle<Name> name,
-      Handle<Object> value, MaybeHandle<Map> maybe_transition_map);
-
   void Restart() {
     InterceptorState state = InterceptorState::kUninitialized;
     IsElement() ? RestartInternal<true>(state) : RestartInternal<false>(state);

src/objects/map.cc

@@ -2077,11 +2077,11 @@ Handle<Map> Map::PrepareForDataProperty(Isolate* isolate, Handle<Map> map,
                                         int descriptor,
                                         PropertyConstness constness,
                                         Handle<Object> value) {
+  // Update to the newest map before storing the property.
+  map = Update(isolate, map);
   // Dictionaries can store any property value.
   DCHECK(!map->is_dictionary_map());
-  // Update to the newest map before storing the property.
-  return UpdateDescriptorForValue(isolate, Update(isolate, map), descriptor,
-                                  constness, value);
+  return UpdateDescriptorForValue(isolate, map, descriptor, constness, value);
 }
 
 Handle<Map> Map::TransitionToDataProperty(Isolate* isolate, Handle<Map> map,

test/mjsunit/regress/regress-crbug-977012.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.
+
+function foo(arg) {
+  var ret = { x: arg };
+  ret.__defineSetter__("y", function() { });
+  return ret;
+}
+
+// v1 creates a map with a Smi field, v2 deprecates v1's map.
+let v1 = foo(10);
+let v2 = foo(10.5);
+
+// Trigger a PrepareForDataProperty on v1, which also triggers an update to
+// dictionary due to the different accessors on v1 and v2's y property.
+v1.x = 20.5;