Make updatePropertyFlagsWithoutTransitions always create a dictionary…

… mode class

Summary:
Generally, HiddenClasses are either in dictionary mode, or part of a tree of transitions such that walking the parents chain passes through all properties of the class.

However, HiddenClass::updatePropertyFlagsWithoutTransitions would create oddball classes that fit neither of these categories: they were non-dictionary non-empty roots. In particular, it was not safe to clear their property map (nor that of any descendants).

Change this method to always create a dictionary mode class. Such a class is still eligible for property caching. This method is currently only used to freeze the static builtins, so no further transitions from such classes were actually being created.

Reviewed By: tmikov

Differential Revision: D17779975

fbshipit-source-id: 4884b5ea26afb7ac15acae6687a0c6278e2b1b80

include/hermes/VM/HiddenClass.h

@@ -436,7 +436,7 @@ class HiddenClass final : public GCCell {
 
   /// Update the flags for the properties in the list \p props with \p
   /// flagsToClear and \p flagsToSet. If in dictionary mode, the properties are
-  /// updated on the hidden class directly; otherwise, create only one new
+  /// updated on the hidden class directly; otherwise, create a new dictionary
   /// hidden class as result. Updating the properties mutates the property map
   /// directly without creating transitions.
   /// \p flagsToClear and \p flagsToSet are masks for updating the property

lib/VM/HiddenClass.cpp

@@ -183,6 +183,9 @@ CallResult<HermesValue> HiddenClass::create(
     SymbolID symbolID,
     PropertyFlags propertyFlags,
     unsigned numProperties) {
+  assert(
+      (flags.dictionaryMode || numProperties == 0 || *parent) &&
+      "non-empty non-dictionary orphan");
   void *mem = runtime->allocLongLived<HasFinalizer::Yes>(sizeof(HiddenClass));
   return HermesValue::encodeObjectValue(new (mem) HiddenClass(
       runtime, flags, parent, symbolID, propertyFlags, numProperties));
@@ -737,28 +740,12 @@ Handle<HiddenClass> HiddenClass::updatePropertyFlagsWithoutTransitions(
     PropertyFlags flagsToClear,
     PropertyFlags flagsToSet,
     OptValue<llvm::ArrayRef<SymbolID>> props) {
-  // Allocate the property map.
-  if (LLVM_UNLIKELY(!selfHandle->propertyMap_))
-    initializeMissingPropertyMap(selfHandle, runtime);
-
+  // Result must be in dictionary mode, since it's a non-empty orphan.
   MutableHandle<HiddenClass> classHandle{runtime};
   if (selfHandle->isDictionary()) {
     classHandle = *selfHandle;
   } else {
-    // To create an orphan hidden class with updated properties, first clone the
-    // old one, and make it a root.
-    classHandle = vmcast<HiddenClass>(
-        runtime->ignoreAllocationFailure(HiddenClass::create(
-            runtime,
-            selfHandle->flags_,
-            Runtime::makeNullHandle<HiddenClass>(),
-            SymbolID{},
-            PropertyFlags{},
-            selfHandle->numProperties_)));
-    // Move the property map to the new hidden class.
-    classHandle->propertyMap_.set(
-        runtime, selfHandle->propertyMap_.get(runtime), &runtime->getHeap());
-    selfHandle->propertyMap_ = nullptr;
+    classHandle = *copyToNewDictionary(selfHandle, runtime);
   }
 
   auto mapHandle =

unittests/VMRuntime/HiddenClassTest.cpp

@@ -311,7 +311,7 @@ TEST_F(HiddenClassTest, UpdatePropertyFlagsWithoutTransitionsTest) {
       y, runtime, clearFlags, setFlags, llvm::None);
   ASSERT_NE(*y, *yClone);
   ASSERT_EQ(y->getNumProperties(), yClone->getNumProperties());
-  ASSERT_FALSE(yClone->isDictionary());
+  ASSERT_TRUE(yClone->isDictionary());
   // Check each property
   auto found = HiddenClass::findProperty(
       yClone, runtime, *aHnd, PropertyFlags::invalid(), desc);
@@ -365,7 +365,7 @@ TEST_F(HiddenClassTest, UpdatePropertyFlagsWithoutTransitionsTest) {
 
   ASSERT_NE(*y, *partlyFrozenSingleton);
   ASSERT_EQ(y->getNumProperties(), partlyFrozenSingleton->getNumProperties());
-  ASSERT_FALSE(partlyFrozenSingleton->isDictionary());
+  ASSERT_TRUE(partlyFrozenSingleton->isDictionary());
   // Check each property
   found = HiddenClass::findProperty(
       partlyFrozenSingleton, runtime, *aHnd, PropertyFlags::invalid(), desc);
@@ -396,7 +396,7 @@ TEST_F(HiddenClassTest, UpdatePropertyFlagsWithoutTransitionsTest) {
       PropertyFlags::defaultNewNamedPropertyFlags());
   ASSERT_RETURNED(addRes);
   ASSERT_EQ(3u, addRes->second);
-  ASSERT_NE(*addRes->first, *partlyFrozenSingleton);
+  ASSERT_EQ(*addRes->first, *partlyFrozenSingleton);
   ASSERT_EQ(addRes->first->getNumProperties(), 4);
 }