From 95e1ec4abc5cb20cf8f242e20e20418cee0fb7ef Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Tue, 20 May 2025 23:42:17 +0300
Subject: [PATCH] LibJS: Skip caching get_by_id() if object's shape is changed
 by a getter

Fixes a bug that reproduces with the following steps:
1. Create an object with a getter for property "a" in its prototype,
   where the getter adds an "a" property to the object itself.
2. Call the "a" getter in a loop for the first time. This triggers
   caching of metadata indicating that the "a" property is located in
   the prototype chain.
3. Call the "a" getter in a loop for the second time. Oops, the cache
   says the getter is in the prototype chain, but the object now
   also has its own "a" property that was added by the first getter
   call.
---
 Libraries/LibJS/Bytecode/Interpreter.cpp      | 43 +++++++++++--------
 ...-with-the-same-from-getter-in-prototype.js | 30 +++++++++++++
 2 files changed, 56 insertions(+), 17 deletions(-)
 create mode 100644 Libraries/LibJS/Tests/regress/add-property-with-the-same-from-getter-in-prototype.js

diff --git a/Libraries/LibJS/Bytecode/Interpreter.cpp b/Libraries/LibJS/Bytecode/Interpreter.cpp
index 9a690c0f4ae..c91d6cc330e 100644
--- a/Libraries/LibJS/Bytecode/Interpreter.cpp
+++ b/Libraries/LibJS/Bytecode/Interpreter.cpp
@@ -1022,23 +1022,28 @@ inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<IdentifierTableIndex>
     CacheablePropertyMetadata cacheable_metadata;
     auto value = TRY(base_obj->internal_get(executable.get_identifier(property), this_value, &cacheable_metadata));
 
-    auto get_cache_slot = [&] -> PropertyLookupCache::Entry& {
-        for (size_t i = cache.entries.size() - 1; i >= 1; --i) {
-            cache.entries[i] = cache.entries[i - 1];
+    // If internal_get() caused object's shape change, we can no longer be sure
+    // that collected metadata is valid, e.g. if getter in prototype chain added
+    // property with the same name into the object itself.
+    if (&shape == &base_obj->shape()) {
+        auto get_cache_slot = [&] -> PropertyLookupCache::Entry& {
+            for (size_t i = cache.entries.size() - 1; i >= 1; --i) {
+                cache.entries[i] = cache.entries[i - 1];
+            }
+            cache.entries[0] = {};
+            return cache.entries[0];
+        };
+        if (cacheable_metadata.type == CacheablePropertyMetadata::Type::OwnProperty) {
+            auto& entry = get_cache_slot();
+            entry.shape = shape;
+            entry.property_offset = cacheable_metadata.property_offset.value();
+        } else if (cacheable_metadata.type == CacheablePropertyMetadata::Type::InPrototypeChain) {
+            auto& entry = get_cache_slot();
+            entry.shape = &base_obj->shape();
+            entry.property_offset = cacheable_metadata.property_offset.value();
+            entry.prototype = *cacheable_metadata.prototype;
+            entry.prototype_chain_validity = *cacheable_metadata.prototype->shape().prototype_chain_validity();
         }
-        cache.entries[0] = {};
-        return cache.entries[0];
-    };
-    if (cacheable_metadata.type == CacheablePropertyMetadata::Type::OwnProperty) {
-        auto& entry = get_cache_slot();
-        entry.shape = shape;
-        entry.property_offset = cacheable_metadata.property_offset.value();
-    } else if (cacheable_metadata.type == CacheablePropertyMetadata::Type::InPrototypeChain) {
-        auto& entry = get_cache_slot();
-        entry.shape = &base_obj->shape();
-        entry.property_offset = cacheable_metadata.property_offset.value();
-        entry.prototype = *cacheable_metadata.prototype;
-        entry.prototype_chain_validity = *cacheable_metadata.prototype->shape().prototype_chain_validity();
     }
 
     return value;
@@ -1227,6 +1232,7 @@ inline ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value thi
         break;
     }
     case Op::PropertyKind::KeyValue: {
+        auto& shape = object->shape();
         if (caches) {
             for (auto& cache : caches->entries) {
                 if (cache.prototype) {
@@ -1262,7 +1268,10 @@ inline ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value thi
         CacheablePropertyMetadata cacheable_metadata;
         bool succeeded = TRY(object->internal_set(name, value, this_value, &cacheable_metadata));
 
-        if (succeeded && caches) {
+        // If internal_set() caused object's shape change, we can no longer be sure
+        // that collected metadata is valid, e.g. if setter in prototype chain added
+        // property with the same name into the object itself.
+        if (succeeded && caches && &shape == &object->shape()) {
             auto get_cache_slot = [&] -> PropertyLookupCache::Entry& {
                 for (size_t i = caches->entries.size() - 1; i >= 1; --i) {
                     caches->entries[i] = caches->entries[i - 1];
diff --git a/Libraries/LibJS/Tests/regress/add-property-with-the-same-from-getter-in-prototype.js b/Libraries/LibJS/Tests/regress/add-property-with-the-same-from-getter-in-prototype.js
new file mode 100644
index 00000000000..ffc4d91e93c
--- /dev/null
+++ b/Libraries/LibJS/Tests/regress/add-property-with-the-same-from-getter-in-prototype.js
@@ -0,0 +1,30 @@
+test("Mutation of object in getter should result in skipping of collecting inline cache data", () => {
+    class Color {
+        get rgb() {
+            const value = [];
+            Object.defineProperty(this, "rgb", { value });
+            return value;
+        }
+
+        set rgb(value) {
+            Object.defineProperty(this, "rgb", { value });
+        }
+    }
+
+    function testGetting() {
+        const c = new Color();
+        for (let i = 0; i < 2; i++) {
+            c.rgb;
+        }
+    }
+
+    function testSetting() {
+        const c = new Color();
+        for (let i = 0; i < 2; i++) {
+            c.rgb = i;
+        }
+    }
+
+    expect(testGetting).not.toThrow();
+    expect(testSetting).not.toThrow();
+});