Clean up frozen(), sealed(), nonExtensible() and propertiesFrozen()

They all had some interesting bugs and duplicated each other:
a, propertiesFrozen() changed each property individually, creating a lot
of unnecessary intermediate classes. frozen() changed them all at once.
b, If a class happened to contain only properties that matched the
characteristics of being "sealed" or "frozen", sealed(), frozen() and
propertiesFrozen() would set the flags in place and return the same
class. This is bad because it violates the assumption that an
InternalClass is immutable and it breaks the recursive freezing
algorithm we rely on for the global object. It would stop freezing child
objects at any such class, even if the children were not frozen.
c, propertiesFrozen() did not set any of the flags even though it
effectively sealed and froze the class. Therefore, when requesting the
same class as frozen() it would iterate through all the properties
again.
d, frozen() implicitly also sealed the object and made it
non-extensible. sealed() also implicitly made it non-extensible. This is
impractical as we want to allow objects to be extensible even though all
their properties are frozen. Therefore we only set the flag that belongs
to each method now. We do know, however, that a frozen object is
implicitly sealed. Therefore we can short-circuit this transition.

Furthermore, we need to remove the assert in InternalClass::init() as
you can indeed use frozen objects as prototypes for others, but that
needs to be recorded in the original InternalClass via the isUsedAsProto
flag. In order to set this flag, we need to perform a transition and
therefore, derive from the old InternalClass.

The JavaScript isFrozen() method asks for an _implicitly_, "duck typed",
frozen state, which is different from what our "isFrozen" flag denotes.
Therefore we add a separate const method that just checks whether all
properties are frozen.

Task-number: QTBUG-76033
Change-Id: I375fef83fb99035d470490fdf2348766b090831e
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
 

4 files changed, 45 insertions, 65 deletions

diff --git a/src/qml/jsruntime/qv4engine.cpp b/src/qml/jsruntime/qv4engine.cpp
index cd77ebd22a..0ad4a4f136 100644
--- a/src/qml/jsruntime/qv4engine.cpp
+++ b/src/qml/jsruntime/qv4engine.cpp
@@ -683,7 +683,7 @@ ExecutionEngine::ExecutionEngine(QJSEngine *jsEngine)
 
     ScopedFunctionObject t(scope, memoryManager->allocate<FunctionObject>(rootContext(), nullptr, ::throwTypeError));
     t->defineReadonlyProperty(id_length(), Value::fromInt32(0));
-    t->setInternalClass(t->internalClass()->frozen());
+    t->setInternalClass(t->internalClass()->cryopreserved());
     jsObjects[ThrowerObject] = t;
 
     ScopedProperty pd(scope);
@@ -1872,7 +1872,7 @@ void ExecutionEngine::setQmlEngine(QQmlEngine *engine)
 
 static void freeze_recursive(QV4::ExecutionEngine *v4, QV4::Object *object)
 {
-    if (object->as<QV4::QObjectWrapper>())
+    if (object->as<QV4::QObjectWrapper>() || object->internalClass()->isFrozen)
         return;
 
     QV4::Scope scope(v4);
@@ -1889,10 +1889,8 @@ static void freeze_recursive(QV4::ExecutionEngine *v4, QV4::Object *object)
     if (!instanceOfObject)
         return;
 
-    QV4::Heap::InternalClass *frozen = object->internalClass()->propertiesFrozen();
-    if (object->internalClass() == frozen)
-        return;
-    object->setInternalClass(frozen);
+    Heap::InternalClass *frozen = object->internalClass()->frozen();
+    object->setInternalClass(frozen); // Immediately assign frozen to prevent it from getting GC'd
 
     QV4::ScopedObject o(scope);
     for (uint i = 0; i < frozen->size; ++i) {
diff --git a/src/qml/jsruntime/qv4internalclass.cpp b/src/qml/jsruntime/qv4internalclass.cpp
index d597335031..70849775cb 100644
--- a/src/qml/jsruntime/qv4internalclass.cpp
+++ b/src/qml/jsruntime/qv4internalclass.cpp
@@ -290,7 +290,6 @@ void InternalClass::init(ExecutionEngine *engine)
 void InternalClass::init(Heap::InternalClass *other)
 {
     Base::init();
-    Q_ASSERT(!other->isFrozen);
     new (&propertyTable) PropertyHash(other->propertyTable);
     new (&nameMap) SharedInternalClassData<PropertyKey>(other->nameMap);
     new (&propertyData) SharedInternalClassData<PropertyAttributes>(other->propertyData);
@@ -564,22 +563,6 @@ Heap::InternalClass *InternalClass::sealed()
     if (isSealed)
         return this;
 
-    bool alreadySealed = !extensible;
-    for (uint i = 0; i < size; ++i) {
-        PropertyAttributes attrs = propertyData.at(i);
-        if (attrs.isEmpty())
-            continue;
-        if (attrs.isConfigurable()) {
-            alreadySealed = false;
-            break;
-        }
-    }
-
-    if (alreadySealed) {
-        isSealed = true;
-        return this;
-    }
-
     Transition temp = { { PropertyKey::invalid() }, nullptr, InternalClassTransition::Sealed };
     Transition &t = lookupOrInsertTransition(temp);
 
@@ -592,14 +575,15 @@ Heap::InternalClass *InternalClass::sealed()
     Scoped<QV4::InternalClass> ic(scope, engine->newClass(this));
     Heap::InternalClass *s = ic->d();
 
-    for (uint i = 0; i < size; ++i) {
-        PropertyAttributes attrs = propertyData.at(i);
-        if (attrs.isEmpty())
-            continue;
-        attrs.setConfigurable(false);
-        s->propertyData.set(i, attrs);
+    if (!isFrozen) { // freezing also makes all properties non-configurable
+        for (uint i = 0; i < size; ++i) {
+            PropertyAttributes attrs = propertyData.at(i);
+            if (attrs.isEmpty())
+                continue;
+            attrs.setConfigurable(false);
+            s->propertyData.set(i, attrs);
+        }
     }
-    s->extensible = false;
     s->isSealed = true;
 
     t.lookup = s;
@@ -611,28 +595,11 @@ Heap::InternalClass *InternalClass::frozen()
     if (isFrozen)
         return this;
 
-    bool alreadyFrozen = !extensible;
-    for (uint i = 0; i < size; ++i) {
-        PropertyAttributes attrs = propertyData.at(i);
-        if (attrs.isEmpty())
-            continue;
-        if ((attrs.isData() && attrs.isWritable()) || attrs.isConfigurable()) {
-            alreadyFrozen = false;
-            break;
-        }
-    }
-
-    if (alreadyFrozen) {
-        isSealed = true;
-        isFrozen = true;
-        return this;
-    }
-
     Transition temp = { { PropertyKey::invalid() }, nullptr, InternalClassTransition::Frozen };
     Transition &t = lookupOrInsertTransition(temp);
 
     if (t.lookup) {
-        Q_ASSERT(t.lookup && t.lookup->isSealed && t.lookup->isFrozen);
+        Q_ASSERT(t.lookup && t.lookup->isFrozen);
         return t.lookup;
     }
 
@@ -649,29 +616,42 @@ Heap::InternalClass *InternalClass::frozen()
         attrs.setConfigurable(false);
         f->propertyData.set(i, attrs);
     }
-    f->extensible = false;
-    f->isSealed = true;
     f->isFrozen = true;
 
     t.lookup = f;
     return f;
 }
 
-Heap::InternalClass *InternalClass::propertiesFrozen()
+InternalClass *InternalClass::canned()
 {
+    // scope the intermediate result to prevent it from getting garbage collected
     Scope scope(engine);
-    Scoped<QV4::InternalClass> frozen(scope, this);
+    Scoped<QV4::InternalClass> ic(scope, sealed());
+    return ic->d()->nonExtensible();
+}
+
+InternalClass *InternalClass::cryopreserved()
+{
+    // scope the intermediate result to prevent it from getting garbage collected
+    Scope scope(engine);
+    Scoped<QV4::InternalClass> ic(scope, frozen());
+    return ic->d()->canned();
+}
+
+bool InternalClass::isImplicitlyFrozen() const
+{
+    if (isFrozen)
+        return true;
+
     for (uint i = 0; i < size; ++i) {
-        PropertyAttributes attrs = propertyData.at(i);
-        if (!nameMap.at(i).isValid())
+        const PropertyAttributes attrs = propertyData.at(i);
+        if (attrs.isEmpty())
             continue;
-        if (!attrs.isEmpty()) {
-            attrs.setWritable(false);
-            attrs.setConfigurable(false);
-        }
-        frozen = frozen->changeMember(nameMap.at(i), attrs);
+        if ((attrs.isData() && attrs.isWritable()) || attrs.isConfigurable())
+            return false;
     }
-    return frozen->d();
+
+    return true;
 }
 
 Heap::InternalClass *InternalClass::asProtoClass()
diff --git a/src/qml/jsruntime/qv4internalclass_p.h b/src/qml/jsruntime/qv4internalclass_p.h
index 7bb10f47a3..403702ae55 100644
--- a/src/qml/jsruntime/qv4internalclass_p.h
+++ b/src/qml/jsruntime/qv4internalclass_p.h
@@ -432,7 +432,9 @@ struct InternalClass : Base {
 
     Q_REQUIRED_RESULT InternalClass *sealed();
     Q_REQUIRED_RESULT InternalClass *frozen();
-    Q_REQUIRED_RESULT InternalClass *propertiesFrozen();
+    Q_REQUIRED_RESULT InternalClass *canned();        // sealed + nonExtensible
+    Q_REQUIRED_RESULT InternalClass *cryopreserved(); // frozen + sealed + nonExtensible
+    bool isImplicitlyFrozen() const;
 
     Q_REQUIRED_RESULT InternalClass *asProtoClass();
 
diff --git a/src/qml/jsruntime/qv4objectproto.cpp b/src/qml/jsruntime/qv4objectproto.cpp
index 6b4c3ba71a..3d3b3f413f 100644
--- a/src/qml/jsruntime/qv4objectproto.cpp
+++ b/src/qml/jsruntime/qv4objectproto.cpp
@@ -422,7 +422,7 @@ ReturnedValue ObjectPrototype::method_seal(const FunctionObject *b, const Value
 
     Scope scope(b);
     ScopedObject o(scope, a);
-    o->setInternalClass(o->internalClass()->sealed());
+    o->setInternalClass(o->internalClass()->canned());
 
     if (o->arrayData()) {
         ArrayData::ensureAttributes(o);
@@ -448,7 +448,7 @@ ReturnedValue ObjectPrototype::method_freeze(const FunctionObject *b, const Valu
     if (ArgumentsObject::isNonStrictArgumentsObject(o))
         static_cast<ArgumentsObject *>(o.getPointer())->fullyCreate();
 
-    o->setInternalClass(o->internalClass()->frozen());
+    o->setInternalClass(o->internalClass()->cryopreserved());
 
     if (o->arrayData()) {
         ArrayData::ensureAttributes(o);
@@ -489,7 +489,7 @@ ReturnedValue ObjectPrototype::method_isSealed(const FunctionObject *b, const Va
     if (o->isExtensible())
         return  Encode(false);
 
-    if (o->internalClass() != o->internalClass()->sealed())
+    if (o->internalClass() != o->internalClass()->canned())
         return Encode(false);
 
     if (!o->arrayData() || !o->arrayData()->length())
@@ -521,7 +521,7 @@ ReturnedValue ObjectPrototype::method_isFrozen(const FunctionObject *b, const Va
     if (o->isExtensible())
         return Encode(false);
 
-    if (o->internalClass() != o->internalClass()->frozen())
+    if (!o->internalClass()->isImplicitlyFrozen())
         return Encode(false);
 
     if (!o->arrayData() || !o->arrayData()->length())