Reland "Fix accessor update of non-extensible maps."

This is a reland of 1a3a2bc335,
fixed an infinite loop in Map::TryUpdateSlow and added
a relevant test.

Original change's description:
> Fix accessor update of non-extensible maps.
>
> When installing getter/setter of non-extensible map with existing
> setter/getter of the same name, we introduce a new transition
> (so we have two transitions with the same name!). This triggers
> an assertion in map updater.
>
> This fix carefully checks that on the back-pointer path from
> non-extensible map to the extensible map there are only
> integrity level transitions. Otherwise, we just bail out.
>
> Bug: chromium:932953
> Change-Id: I02e91c3b652428a84a9f5c58b6691ea9b1fc44d6
> Reviewed-on: https://chromium-review.googlesource.com/c/1477067
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#59667}

Bug: chromium:932953
Change-Id: I015ee3795f816c8eabb5b5c5cb0ee30f365cc972
Reviewed-on: https://chromium-review.googlesource.com/c/1477675
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59715}

src/map-updater.cc

@@ -9,6 +9,7 @@
 #include "src/isolate.h"
 #include "src/objects-inl.h"
 #include "src/objects.h"
+#include "src/property-details.h"
 #include "src/transitions.h"
 
 namespace v8 {
@@ -254,39 +255,41 @@ MapUpdater::State MapUpdater::TryReconfigureToDataFieldInplace() {
 }
 
 bool MapUpdater::TrySaveIntegrityLevelTransitions() {
-  // Skip integrity level transitions.
-  integrity_source_map_ = old_map_;
-  while (!integrity_source_map_->is_extensible()) {
-    integrity_source_map_ =
-        handle(Map::cast(integrity_source_map_->GetBackPointer()), isolate_);
-  }
-
-  // If there are some non-integrity-level transitions after the first
-  // non-extensible transitions, e.g., if there were private symbols transitions
-  // after the first integrity level transition, then we just bail out and
-  // generalize all the fields.
-  if (old_map_->NumberOfOwnDescriptors() !=
-      integrity_source_map_->NumberOfOwnDescriptors()) {
-    return false;
-  }
-
   // Figure out the most restrictive integrity level transition (it should
   // be the last one in the transition tree).
-  ReadOnlyRoots roots(isolate_);
-  TransitionsAccessor transitions(
-      isolate_, handle(Map::cast(old_map_->GetBackPointer()), isolate_));
-  if (transitions.SearchSpecial(roots.frozen_symbol()) == *old_map_) {
-    integrity_level_ = FROZEN;
-    integrity_level_symbol_ = isolate_->factory()->frozen_symbol();
-  } else if (transitions.SearchSpecial(roots.sealed_symbol()) == *old_map_) {
-    integrity_level_ = SEALED;
-    integrity_level_symbol_ = isolate_->factory()->sealed_symbol();
-  } else {
-    CHECK_EQ(transitions.SearchSpecial(roots.nonextensible_symbol()),
-             *old_map_);
-    integrity_level_ = NONE;
-    integrity_level_symbol_ = isolate_->factory()->nonextensible_symbol();
+  Handle<Map> previous =
+      handle(Map::cast(old_map_->GetBackPointer()), isolate_);
+  Symbol integrity_level_symbol;
+  TransitionsAccessor last_transitions(isolate_, previous);
+  if (!last_transitions.HasIntegrityLevelTransitionTo(
+          *old_map_, &integrity_level_symbol, &integrity_level_)) {
+    // The last transition was not integrity level transition - just bail out.
+    // This can happen in the following cases:
+    // - there are private symbol transitions following the integrity level
+    //   transitions (see crbug.com/v8/8854).
+    // - there is a getter added in addition to an existing setter (or a setter
+    //   in addition to an existing getter).
+    return false;
   }
+  integrity_level_symbol_ = handle(integrity_level_symbol, isolate_);
+  integrity_source_map_ = previous;
+
+  // Now walk up the back pointer chain and skip all integrity level
+  // transitions. If we encounter any non-integrity level transition interleaved
+  // with integrity level transitions, just bail out.
+  while (!integrity_source_map_->is_extensible()) {
+    previous =
+        handle(Map::cast(integrity_source_map_->GetBackPointer()), isolate_);
+    TransitionsAccessor transitions(isolate_, previous);
+    if (!transitions.HasIntegrityLevelTransitionTo(*integrity_source_map_)) {
+      return false;
+    }
+    integrity_source_map_ = previous;
+  }
+
+  // Integrity-level transitions never change number of descriptors.
+  CHECK_EQ(old_map_->NumberOfOwnDescriptors(),
+           integrity_source_map_->NumberOfOwnDescriptors());
 
   has_integrity_level_transition_ = true;
   old_descriptors_ =

src/objects/map.cc

@@ -21,7 +21,6 @@
 #include "src/objects/oddball.h"
 #include "src/ostreams.h"
 #include "src/property.h"
-#include "src/roots.h"
 #include "src/transitions-inl.h"
 #include "src/zone/zone-containers.h"
 
@@ -938,40 +937,40 @@ IntegrityLevelTransitionInfo DetectIntegrityLevelTransitions(
     Map map, Isolate* isolate, DisallowHeapAllocation* no_allocation) {
   IntegrityLevelTransitionInfo info(map);
 
+  // Figure out the most restrictive integrity level transition (it should
+  // be the last one in the transition tree).
   DCHECK(!map->is_extensible());
-  Map source_map = map;
-  // Skip integrity level transitions.
-  while (!source_map->is_extensible()) {
-    source_map = Map::cast(source_map->GetBackPointer());
-  }
-
-  // If there are some non-integrity-level transitions after the first
-  // non-extensible transitions, e.g., if there were private symbols transitions
-  // after the first integrity level transition, then we just bail out.
-  if (map->NumberOfOwnDescriptors() != source_map->NumberOfOwnDescriptors()) {
+  Map previous = Map::cast(map->GetBackPointer());
+  TransitionsAccessor last_transitions(isolate, previous, no_allocation);
+  if (!last_transitions.HasIntegrityLevelTransitionTo(
+          map, &(info.integrity_level_symbol), &(info.integrity_level))) {
+    // The last transition was not integrity level transition - just bail out.
+    // This can happen in the following cases:
+    // - there are private symbol transitions following the integrity level
+    //   transitions (see crbug.com/v8/8854).
+    // - there is a getter added in addition to an existing setter (or a setter
+    //   in addition to an existing getter).
     return info;
   }
 
-  // Figure out the most restrictive integrity level transition (it should
-  // be the last one in the transition tree).
-  ReadOnlyRoots roots(isolate);
-  TransitionsAccessor transitions(isolate, Map::cast(map->GetBackPointer()),
-                                  no_allocation);
-  if (transitions.SearchSpecial(roots.frozen_symbol()) == map) {
-    info.integrity_level = FROZEN;
-    info.integrity_level_symbol = roots.frozen_symbol();
-  } else if (transitions.SearchSpecial(roots.sealed_symbol()) == map) {
-    info.integrity_level = SEALED;
-    info.integrity_level_symbol = roots.sealed_symbol();
-  } else {
-    CHECK_EQ(transitions.SearchSpecial(roots.nonextensible_symbol()), map);
-    info.integrity_level = NONE;
-    info.integrity_level_symbol = roots.nonextensible_symbol();
+  Map source_map = previous;
+  // Now walk up the back pointer chain and skip all integrity level
+  // transitions. If we encounter any non-integrity level transition interleaved
+  // with integrity level transitions, just bail out.
+  while (!source_map->is_extensible()) {
+    previous = Map::cast(source_map->GetBackPointer());
+    TransitionsAccessor transitions(isolate, previous, no_allocation);
+    if (!transitions.HasIntegrityLevelTransitionTo(source_map)) {
+      return info;
+    }
+    source_map = previous;
   }
 
+  // Integrity-level transitions never change number of descriptors.
+  CHECK_EQ(map->NumberOfOwnDescriptors(), source_map->NumberOfOwnDescriptors());
+
   info.has_integrity_level_transition = true;
   info.integrity_level_source_map = source_map;
-
   return info;
 }
 

src/transitions.cc

@@ -690,5 +690,23 @@ void TransitionArray::Sort() {
   DCHECK(IsSortedNoDuplicates());
 }
 
+bool TransitionsAccessor::HasIntegrityLevelTransitionTo(
+    Map to, Symbol* out_symbol, PropertyAttributes* out_integrity_level) {
+  ReadOnlyRoots roots(isolate_);
+  if (SearchSpecial(roots.frozen_symbol()) == to) {
+    if (out_integrity_level) *out_integrity_level = FROZEN;
+    if (out_symbol) *out_symbol = roots.frozen_symbol();
+  } else if (SearchSpecial(roots.sealed_symbol()) == to) {
+    if (out_integrity_level) *out_integrity_level = SEALED;
+    if (out_symbol) *out_symbol = roots.sealed_symbol();
+  } else if (SearchSpecial(roots.nonextensible_symbol()) == to) {
+    if (out_integrity_level) *out_integrity_level = NONE;
+    if (out_symbol) *out_symbol = roots.nonextensible_symbol();
+  } else {
+    return false;
+  }
+  return true;
+}
+
 }  // namespace internal
 }  // namespace v8

src/transitions.h

@@ -86,6 +86,10 @@ class TransitionsAccessor {
   static bool IsMatchingMap(Map target, Name name, PropertyKind kind,
                             PropertyAttributes attributes);
 
+  bool HasIntegrityLevelTransitionTo(
+      Map to, Symbol* out_symbol = nullptr,
+      PropertyAttributes* out_integrity_level = nullptr);
+
   // ===== ITERATION =====
   typedef void (*TraverseCallback)(Map map, void* data);
 

test/mjsunit/regress/regress-932953.js

@@ -0,0 +1,59 @@
+// 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
+
+(function NonExtensibleBetweenSetterAndGetter() {
+  o = {};
+  o.x = 42;
+  o.__defineGetter__("y", function() { });
+  Object.preventExtensions(o);
+  o.__defineSetter__("y", function() { });
+  o.x = 0.1;
+})();
+
+(function InterleavedIntegrityLevel() {
+  o = {};
+  o.x = 42;
+  o.__defineSetter__("y", function() { });
+  Object.preventExtensions(o);
+  o.__defineGetter__("y", function() { return 44; });
+  Object.seal(o);
+  o.x = 0.1;
+  assertEquals(44, o.y);
+})();
+
+(function TryUpdateRepeatedIntegrityLevel() {
+  function C() {
+    this.x = 0;
+    this.x = 1;
+    Object.preventExtensions(this);
+    Object.seal(this);
+  }
+
+  const o1 = new C();
+  const o2 = new C();
+  const o3 = new C();
+
+  function f(o) {
+    return o.x;
+  }
+
+  // Warm up the IC.
+  f(o1);
+  f(o1);
+  f(o1);
+
+  // Reconfigure to double field.
+  o3.x = 0.1;
+
+  // Migrate o2 to the new shape.
+  f(o2);
+
+  %OptimizeFunctionOnNextCall(f);
+  f(o1);
+
+  assertTrue(%HaveSameMap(o1, o2));
+  assertTrue(%HaveSameMap(o1, o3));
+})();