LibJS: Object.preventExtensions should allow property modfication

Existing properties on a non-extensible object should be changable and
deletable.

3 files changed, 39 insertions, 17 deletions

diff --git a/Libraries/LibJS/Runtime/Object.cpp b/Libraries/LibJS/Runtime/Object.cpp
index f2fd036eaf..09649fc074 100644
--- a/Libraries/LibJS/Runtime/Object.cpp
+++ b/Libraries/LibJS/Runtime/Object.cpp
@@ -462,7 +462,10 @@ bool Object::put_own_property(Object& this_object, const FlyString& property_nam
 {
     ASSERT(!(mode == PutOwnPropertyMode::Put && value.is_accessor()));
 
-    if (!is_extensible()) {
+    auto metadata = shape().lookup(property_name);
+    bool new_property = !metadata.has_value();
+
+    if (!is_extensible() && new_property) {
 #ifdef OBJECT_DEBUG
         dbg() << "Disallow define_property of non-extensible object";
 #endif
@@ -479,8 +482,6 @@ bool Object::put_own_property(Object& this_object, const FlyString& property_nam
             attributes.set_has_setter();
     }
 
-    auto metadata = shape().lookup(property_name);
-    bool new_property = !metadata.has_value();
 
     if (new_property) {
         if (!m_shape->is_unique() && shape().property_count() > 100) {
@@ -544,7 +545,10 @@ bool Object::put_own_property_by_index(Object& this_object, u32 property_index,
 {
     ASSERT(!(mode == PutOwnPropertyMode::Put && value.is_accessor()));
 
-    if (!is_extensible()) {
+    auto existing_property = m_indexed_properties.get(nullptr, property_index, false);
+    auto new_property = !existing_property.has_value();
+
+    if (!is_extensible() && new_property) {
 #ifdef OBJECT_DEBUG
         dbg() << "Disallow define_property of non-extensible object";
 #endif
@@ -561,8 +565,6 @@ bool Object::put_own_property_by_index(Object& this_object, u32 property_index,
             attributes.set_has_setter();
     }
 
-    auto existing_property = m_indexed_properties.get(nullptr, property_index, false);
-    auto new_property = !existing_property.has_value();
     PropertyAttributes existing_attributes = new_property ? 0 : existing_property.value().attributes;
 
     if (!new_property && mode == PutOwnPropertyMode::DefineProperty && !existing_attributes.is_configurable() && attributes != existing_attributes) {
diff --git a/Libraries/LibJS/Tests/builtins/Object/Object.preventExtensions.js b/Libraries/LibJS/Tests/builtins/Object/Object.preventExtensions.js
index 776fc81d3f..292c3f5999 100644
--- a/Libraries/LibJS/Tests/builtins/Object/Object.preventExtensions.js
+++ b/Libraries/LibJS/Tests/builtins/Object/Object.preventExtensions.js
@@ -24,6 +24,20 @@ describe("correct behavior", () => {
         o.baz = "baz";
         expect(o.baz).toBeUndefined();
     });
+
+    test("modifying existing properties", () => {
+        const o = { foo: "bar" };
+        Object.preventExtensions(o);
+        o.foo = "baz";
+        expect(o.foo).toBe("baz");
+    });
+
+    test("deleting existing properties", () => {
+        const o = { foo: "bar" };
+        Object.preventExtensions(o);
+        delete o.foo;
+        expect(o).not.toHaveProperty("foo");
+    });
 });
 
 describe("errors", () => {
diff --git a/Libraries/LibJS/Tests/builtins/Reflect/Reflect.preventExtensions.js b/Libraries/LibJS/Tests/builtins/Reflect/Reflect.preventExtensions.js
index 0055ef7741..6b0bfdc1bc 100644
--- a/Libraries/LibJS/Tests/builtins/Reflect/Reflect.preventExtensions.js
+++ b/Libraries/LibJS/Tests/builtins/Reflect/Reflect.preventExtensions.js
@@ -1,7 +1,3 @@
-test("length is 1", () => {
-    expect(Reflect.preventExtensions).toHaveLength(1);
-});
-
 describe("errors", () => {
     test("target must be an object", () => {
         [null, undefined, "foo", 123, NaN, Infinity].forEach(value => {
@@ -16,6 +12,10 @@ describe("errors", () => {
 });
 
 describe("normal behavior", () => {
+    test("length is 1", () => {
+        expect(Reflect.preventExtensions).toHaveLength(1);
+    });
+
     test("properties cannot be added", () => {
         var o = {};
         o.foo = "foo";
@@ -25,12 +25,18 @@ describe("normal behavior", () => {
         expect(o.bar).toBeUndefined();
     });
 
-    test("property values can still be changed", () => {
-        // FIXME: This doesn't work even though it should (the value remains unchanged)
-        // var o = {};
-        // o.foo = "foo";
-        // expect(Reflect.preventExtensions(o)).toBeTrue();
-        // o.foo = "bar";
-        // expect(o.foo).toBe("bar");
+    test("modifying existing properties", () => {
+        const o = {};
+        o.foo = "foo";
+        expect(Reflect.preventExtensions(o)).toBeTrue();
+        o.foo = "bar";
+        expect(o.foo).toBe("bar");
+    });
+
+    test("deleting existing properties", () => {
+        const o = { foo: "bar" };
+        Reflect.preventExtensions(o);
+        delete o.foo;
+        expect(o).not.toHaveProperty("foo");
     });
 });