Our for-in caching is wrong when we add indexed properties on things in the prototype...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 08:16:06 +0000 (08:16 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 08:16:06 +0000 (08:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181508

Reviewed by Yusuke Suzuki.

JSTests:

* stress/for-in-prototype-with-indexed-properties-should-prevent-caching.js: Added.
(assert):
(test1.foo):
(test1):
(test2.foo):
(test2):

Source/JavaScriptCore:

Our for-in caching would cache structure chains that had prototypes with
indexed properties. Clearly this is wrong. This caching breaks when a prototype
adds new indexed properties. We would continue to enumerate the old cached
state of properties, and not include the new indexed properties.

The old code used to prevent caching only if the base structure had
indexed properties. This patch extends it to prevent caching if the
base, or any structure in the prototype chain, has indexed properties.

* runtime/Structure.cpp:
(JSC::Structure::canCachePropertyNameEnumerator const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226767 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/for-in-prototype-with-indexed-properties-should-prevent-caching.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/Structure.cpp

index 766e3df9dd03e3f3ad87b2b988777f38e72bb142..83949236f489de2b28df4f312e844f4a2ec9f03a 100644 (file)
@@ -1,3 +1,17 @@
+2018-01-11  Saam Barati  <sbarati@apple.com>
+
+        Our for-in caching is wrong when we add indexed properties on things in the prototype chain
+        https://bugs.webkit.org/show_bug.cgi?id=181508
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/for-in-prototype-with-indexed-properties-should-prevent-caching.js: Added.
+        (assert):
+        (test1.foo):
+        (test1):
+        (test2.foo):
+        (test2):
+
 2018-01-09  Mark Lam  <mark.lam@apple.com>
 
         ASSERTION FAILED: pair.second->m_type & PropertyNode::Getter
diff --git a/JSTests/stress/for-in-prototype-with-indexed-properties-should-prevent-caching.js b/JSTests/stress/for-in-prototype-with-indexed-properties-should-prevent-caching.js
new file mode 100644 (file)
index 0000000..8c3bfa6
--- /dev/null
@@ -0,0 +1,77 @@
+"use strict";
+
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+
+
+function test1() {
+    function foo(o) {
+        let result = [];
+        for (let p in o)
+            result.push(p);
+        return result;
+    }
+    noInline(foo);
+
+    let p = {};
+    let x = {__proto__: p};
+    p[0] = 25;
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 1);
+        assert(result[0] === "0");
+    }
+
+    p[1] = 30;
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 2);
+        assert(result[0] === "0");
+        assert(result[1] === "1");
+    }
+
+    p[2] = {};
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 3);
+        assert(result[0] === "0");
+        assert(result[1] === "1");
+        assert(result[2] === "2");
+    }
+}
+test1();
+
+function test2() {
+    function foo(o) {
+        let result = [];
+        for (let p in o)
+            result.push(p);
+        return result;
+    }
+    noInline(foo);
+
+    let p = {};
+    let x = {__proto__: p};
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 0);
+    }
+
+    p[0] = 30;
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 1);
+        assert(result[0] === "0");
+    }
+
+    p[1] = {};
+    for (let i = 0; i < 20; ++i) {
+        let result = foo(x);
+        assert(result.length === 2);
+        assert(result[0] === "0");
+        assert(result[1] === "1");
+    }
+}
+test2();
index 9ccee9b1f562ed3e017184b1108befeaaef68512..c158bca5e869684cf34c0207db3aa12d561ed5a9 100644 (file)
@@ -1,3 +1,22 @@
+2018-01-11  Saam Barati  <sbarati@apple.com>
+
+        Our for-in caching is wrong when we add indexed properties on things in the prototype chain
+        https://bugs.webkit.org/show_bug.cgi?id=181508
+
+        Reviewed by Yusuke Suzuki.
+
+        Our for-in caching would cache structure chains that had prototypes with
+        indexed properties. Clearly this is wrong. This caching breaks when a prototype
+        adds new indexed properties. We would continue to enumerate the old cached
+        state of properties, and not include the new indexed properties.
+        
+        The old code used to prevent caching only if the base structure had
+        indexed properties. This patch extends it to prevent caching if the
+        base, or any structure in the prototype chain, has indexed properties.
+
+        * runtime/Structure.cpp:
+        (JSC::Structure::canCachePropertyNameEnumerator const):
+
 2018-01-10  JF Bastien  <jfbastien@apple.com>
 
         Poison small JSObject derivatives which only contain pointers
index 6f8dc0dd2995ba33aaa074a0be703bca740d4a73..974a7da6f980f58990f8925273b659b10ab55b15 100644 (file)
@@ -1246,13 +1246,17 @@ JSPropertyNameEnumerator* Structure::cachedPropertyNameEnumerator() const
 
 bool Structure::canCachePropertyNameEnumerator() const
 {
-    if (isDictionary())
-        return false;
-
-    if (hasIndexedProperties(indexingType()))
-        return false;
+    auto canCache = [] (const Structure* structure) {
+        if (structure->isDictionary())
+            return false;
+        if (hasIndexedProperties(structure->indexingType()))
+            return false;
+        if (structure->typeInfo().overridesGetPropertyNames())
+            return false;
+        return true;
+    };
 
-    if (typeInfo().overridesGetPropertyNames())
+    if (!canCache(this))
         return false;
 
     StructureChain* structureChain = m_cachedPrototypeChain.get();
@@ -1260,12 +1264,13 @@ bool Structure::canCachePropertyNameEnumerator() const
     WriteBarrier<Structure>* structure = structureChain->head();
     while (true) {
         if (!structure->get())
-            break;
-        if (structure->get()->isDictionary() || structure->get()->typeInfo().overridesGetPropertyNames())
+            return true;
+        if (!canCache(structure->get()))
             return false;
         structure++;
     }
-    
+
+    ASSERT_NOT_REACHED();
     return true;
 }