[runtime] PreventExtensionsWithTransition: before adding the new

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index ae1cd6a5ae91ae16c4c05d50045e55e10e95dfbf..e25e0e82cdcef52d9d295aa359cccea5cb961693 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -7421,6 +7421,12 @@ Maybe<bool> JSReceiver::SetIntegrityLevel(Handle<JSReceiver> receiver,
 
   if (receiver->IsJSObject()) {
     Handle<JSObject> object = Handle<JSObject>::cast(receiver);
+
+    // prevent memory leaks by not adding unnecessary transitions
+    Maybe<bool> test = JSObject::TestIntegrityLevel(object, level);
+    MAYBE_RETURN(test, Nothing<bool>());
+    if (test.FromJust()) return Just(true);
+
     if (!object->HasSloppyArgumentsElements()) {  // Fast path.
       if (level == SEALED) {
         return JSObject::PreventExtensionsWithTransition<SEALED>(object,
@@ -7477,16 +7483,11 @@ Maybe<bool> JSReceiver::SetIntegrityLevel(Handle<JSReceiver> receiver,
   return Just(true);
 }
 
-
-Maybe<bool> JSReceiver::TestIntegrityLevel(Handle<JSReceiver> object,
-                                           IntegrityLevel level) {
-  DCHECK(level == SEALED || level == FROZEN);
+namespace {
+Maybe<bool> GenericTestDescriptorsIntegrity(Handle<JSReceiver> object,
+                                            PropertyAttributes level) {
   Isolate* isolate = object->GetIsolate();
 
-  Maybe<bool> extensible = JSReceiver::IsExtensible(object);
-  MAYBE_RETURN(extensible, Nothing<bool>());
-  if (extensible.FromJust()) return Just(false);
-
   Handle<FixedArray> keys;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate, keys, JSReceiver::OwnPropertyKeys(object), Nothing<bool>());
@@ -7509,6 +7510,119 @@ Maybe<bool> JSReceiver::TestIntegrityLevel(Handle<JSReceiver> object,
   return Just(true);
 }
 
+template <typename Dictionary>
+bool TestDictionaryPropertyIntegrity(Dictionary dict, Isolate* isolate,
+                                     PropertyAttributes level) {
+  DisallowHeapAllocation no_gc;
+  uint32_t capacity = dict->Capacity();
+  FOR_WITH_HANDLE_SCOPE(isolate, uint32_t, j = 0, j, j < capacity, j++, {

[Toon Verwaest, 2017/06/20 08:43:23]

Since you don't create handles here you don't need a FOR_WITH_HANDLE_SCOPE. Just
use a regular for instead.

[kris.selden, 2017/06/20 19:16:21]

sorry, that was a bit of copy & paste from some other 'Fast' method.

+    Object* k = dict->KeyAt(j);
+    if (!dict->IsKey(isolate, k)) continue;

[Toon Verwaest, 2017/06/20 08:43:23]

You also need to check whether the key is "deleted". In the case of global
properties we actually create "deleted" entries so we can cache property lookups
that find properties behind the global object.

Below I describe that we probably shouldn't support the global object on this
fast path, but even so I think we should add the IsDeleted check here to avoid
forgetting to add it later if we ever use this for other purposes.

+    PropertyDetails details = dict->DetailsAt(j);
+    if (details.IsConfigurable()) return false;
+    if (level == FROZEN && details.kind() == kData && !details.IsReadOnly()) {
+      return false;
+    }
+  });
+  return true;
+}
+
+bool TestElementsIntegrityLevel(JSObject* object, PropertyAttributes level) {
+  ElementsKind kind = object->GetElementsKind();
+
+  DCHECK(!IsSloppyArgumentsElementsKind(kind));
+
+  if (IsDictionaryElementsKind(kind)) {
+    return TestDictionaryPropertyIntegrity(
+        SeededNumberDictionary::cast(object->elements()), object->GetIsolate(),
+        level);
+  }
+
+  ElementsAccessor* accessor = ElementsAccessor::ForKind(kind);
+  // Only DICTIONARY_ELEMENTS and SLOW_SLOPPY_ARGUMENTS_ELEMENTS have
+  // PropertyAttributes so just test if empty
+  return accessor->NumberOfElements(object) == 0;
+}
+
+bool TestFastPropertiesIntegrityLevel(Map* map, PropertyAttributes level) {
+  DescriptorArray* descriptors = map->instance_descriptors();
+  int number_of_own_descriptors = map->NumberOfOwnDescriptors();
+  for (int i = 0; i < number_of_own_descriptors; i++) {
+    PropertyDetails details = descriptors->GetDetails(i);
+    if (details.IsConfigurable()) return false;
+    if (level == FROZEN && details.kind() == kData && !details.IsReadOnly()) {
+      return false;
+    }
+  }
+  return true;
+}
+
+bool TestPropertiesIntegrityLevel(JSObject* object, PropertyAttributes level) {
+  if (object->HasFastProperties()) {
+    return TestFastPropertiesIntegrityLevel(object->map(), level);
+  }
+
+  if (object->IsJSGlobalObject()) {
+    return TestDictionaryPropertyIntegrity(object->global_dictionary(),
+                                           object->GetIsolate(), level);
+  }
+
+  return TestDictionaryPropertyIntegrity(object->property_dictionary(),
+                                         object->GetIsolate(), level);
+}
+
+}  // namespace
+
+Maybe<bool> JSReceiver::TestIntegrityLevel(Handle<JSReceiver> object,
+                                           IntegrityLevel level) {
+  if (object->IsJSObject()) {

[Toon Verwaest, 2017/06/20 08:43:23]

object->instance_type() > LAST_CUSTOM_ELEMENTS_RECEIVER

This makes sure that objects with indexed and named interceptors are dealt with
generically (e.g., localStorage); and we only take the fast path in case we only
need to deal with regular-type properties are you're doing below.

+    return JSObject::TestIntegrityLevel(Handle<JSObject>::cast(object), level);
+  }
+
+  DCHECK(level == SEALED || level == FROZEN);
+
+  Maybe<bool> extensible = JSReceiver::IsExtensible(object);
+  MAYBE_RETURN(extensible, Nothing<bool>());
+  if (extensible.FromJust()) return Just(false);
+
+  return GenericTestDescriptorsIntegrity(object, level);
+}
+
+Maybe<bool> JSObject::TestIntegrityLevel(Handle<JSObject> object,
+                                         IntegrityLevel level) {
+  DCHECK(level == SEALED || level == FROZEN);
+
+  Isolate* isolate = object->GetIsolate();
+  if (object->IsAccessCheckNeeded() &&

[Toon Verwaest, 2017/06/20 08:43:23]

If you only support > LAST_CUSTOM_ELEMENTS_RECEIVER as described above, this
shouldn't be necessary.

I presume the performance of Object.freeze(window) isn't of vital importance ...
:)

+      !isolate->MayAccess(handle(isolate->context()), object)) {
+    return Just(false);
+  }
+
+  if (object->IsJSGlobalProxy()) {

[Toon Verwaest, 2017/06/20 08:43:23]

And in that case you can also drop this.

+    PrototypeIterator iter(isolate, object);
+    if (iter.IsAtEnd()) return Just(true);
+    DCHECK(PrototypeIterator::GetCurrent(iter)->IsJSGlobalObject());
+    return TestIntegrityLevel(PrototypeIterator::GetCurrent<JSObject>(iter),
+                              level);
+  }
+
+  if (object->map()->is_extensible()) return Just(false);
+
+  if (object->HasSloppyArgumentsElements()) {
+    return GenericTestDescriptorsIntegrity(Handle<JSReceiver>::cast(object),
+                                           level);
+  }
+
+  if (!TestElementsIntegrityLevel(*object, level)) {

[Toon Verwaest, 2017/06/20 08:43:23]

return Just(!object->map()->is_extensible() && 
            TestElementsIntegrityLevel(*object, level) &&
            TestPropertiesIntegrityLevel(*object, level));

+    return Just(false);
+  }
+
+  if (!TestPropertiesIntegrityLevel(*object, level)) {
+    return Just(false);
+  }
+
+  return Just(true);
+}
 
 Maybe<bool> JSReceiver::PreventExtensions(Handle<JSReceiver> object,
                                           ShouldThrow should_throw) {