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

This reverts commit 1a3a2bc3.

Reason for revert: Only CL in a reverted roll (https://chromium-review.googlesource.com/c/chromium/src/+/1477147)

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}

TBR=jarin@chromium.org,ishell@chromium.org

Change-Id: I9264423c605cebef87beb6c0f066e90b59faae48
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:932953
Reviewed-on: https://chromium-review.googlesource.com/c/1477219Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59680}

src/map-updater.cc

@@ -9,7 +9,6 @@
 #include "src/isolate.h"
 #include "src/objects-inl.h"
 #include "src/objects.h"
-#include "src/property-details.h"
 #include "src/transitions.h"
 
 namespace v8 {
@@ -255,41 +254,39 @@ MapUpdater::State MapUpdater::TryReconfigureToDataFieldInplace() {
 }
 
 bool MapUpdater::TrySaveIntegrityLevelTransitions() {
-  // Figure out the most restrictive integrity level transition (it should
-  // be the last one in the transition tree).
-  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.
+  // Skip integrity level transitions.
+  integrity_source_map_ = old_map_;
   while (!integrity_source_map_->is_extensible()) {
-    previous =
+    integrity_source_map_ =
         handle(Map::cast(integrity_source_map_->GetBackPointer()), isolate_);
-    TransitionsAccessor transitions(isolate_, previous);
-    if (!transitions.HasIntegrityLevelTransitionTo(*integrity_source_map_)) {
-      return false;
   }
-    integrity_source_map_ = previous;
+
+  // 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;
   }
 
-  // Integrity-level transitions never change number of descriptors.
-  CHECK_EQ(old_map_->NumberOfOwnDescriptors(),
-           integrity_source_map_->NumberOfOwnDescriptors());
+  // 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();
+  }
 
   has_integrity_level_transition_ = true;
   old_descriptors_ =

src/objects/map.cc

@@ -21,6 +21,7 @@
 #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"
 
@@ -937,39 +938,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 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;
+  Map source_map = map;
+  // Skip integrity level transitions.
+  while (!source_map->is_extensible()) {
+    source_map = Map::cast(source_map->GetBackPointer());
   }
 
-  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)) {
+  // 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()) {
     return info;
   }
-  }
 
-  // Integrity-level transitions never change number of descriptors.
-  CHECK_EQ(map->NumberOfOwnDescriptors(), source_map->NumberOfOwnDescriptors());
+  // 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();
+  }
 
   info.has_integrity_level_transition = true;
   info.integrity_level_source_map = source_map;
+
   return info;
 }
 

src/transitions.cc

@@ -690,23 +690,5 @@ 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,10 +86,6 @@ 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

-// 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 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);
-})();