We don't optimize for-in properly in baseline JIT (maybe other JITs too) with an...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Jul 2016 21:11:09 +0000 (21:11 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Jul 2016 21:11:09 +0000 (21:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160211
<rdar://problem/27572612>

Reviewed by Geoffrey Garen.

The fast for-in iteration mode assumes all inline/out-of-line properties
can be iterated in linear order. This is not true if we have Symbols
because Symbols should not be iterated by for-in.

* runtime/Structure.cpp:
(JSC::Structure::add):
* tests/stress/symbol-should-not-break-for-in.js: Added.
(assert):
(foo):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/tests/stress/symbol-should-not-break-for-in.js [new file with mode: 0644]

index 0c6c768..c0b741d 100644 (file)
@@ -1,3 +1,21 @@
+2016-07-27  Saam Barati  <sbarati@apple.com>
+
+        We don't optimize for-in properly in baseline JIT (maybe other JITs too) with an object with symbols
+        https://bugs.webkit.org/show_bug.cgi?id=160211
+        <rdar://problem/27572612>
+
+        Reviewed by Geoffrey Garen.
+
+        The fast for-in iteration mode assumes all inline/out-of-line properties
+        can be iterated in linear order. This is not true if we have Symbols
+        because Symbols should not be iterated by for-in.
+
+        * runtime/Structure.cpp:
+        (JSC::Structure::add):
+        * tests/stress/symbol-should-not-break-for-in.js: Added.
+        (assert):
+        (foo):
+
 2016-07-27  Mark Lam  <mark.lam@apple.com>
 
         The second argument for Function.prototype.apply should be array-like or null/undefined.
index d83e802..1e4594a 100644 (file)
@@ -995,7 +995,7 @@ PropertyOffset Structure::add(VM& vm, PropertyName propertyName, unsigned attrib
     ASSERT(!JSC::isValidOffset(get(vm, propertyName)));
 
     checkConsistency();
-    if (attributes & DontEnum)
+    if (attributes & DontEnum || propertyName.isSymbol())
         setIsQuickPropertyAccessAllowedForEnumeration(false);
 
     auto rep = propertyName.uid();
@@ -1055,6 +1055,7 @@ void Structure::getPropertyNamesFromStructure(VM& vm, PropertyNameArray& propert
     PropertyTable::iterator end = propertyTable()->end();
     for (PropertyTable::iterator iter = propertyTable()->begin(); iter != end; ++iter) {
         ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !(iter->attributes & DontEnum));
+        ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !iter->key->isSymbol());
         if (!(iter->attributes & DontEnum) || mode.includeDontEnumProperties()) {
             if (iter->key->isSymbol() && !propertyNames.includeSymbolProperties())
                 continue;
@@ -1346,6 +1347,7 @@ void Structure::checkConsistency()
         PropertyTable::iterator end = propertyTable()->end();
         for (PropertyTable::iterator iter = propertyTable()->begin(); iter != end; ++iter) {
             ASSERT(!(iter->attributes & DontEnum));
+            ASSERT(!iter->key->isSymbol());
         }
     }
 
diff --git a/Source/JavaScriptCore/tests/stress/symbol-should-not-break-for-in.js b/Source/JavaScriptCore/tests/stress/symbol-should-not-break-for-in.js
new file mode 100644 (file)
index 0000000..87fb16b
--- /dev/null
@@ -0,0 +1,29 @@
+function assert(b) {
+    if (!b)
+        throw new Error("bad assertion.");
+}
+
+function foo(o) {
+    let r = [];
+    for (let p in o)
+        r.push(o[p]);
+    return r;
+}
+noInline(foo);
+
+let o = {};
+o[Symbol()] = "symbol";
+o.prop = "prop";
+for (let i = 0; i < 1000; i++) {
+    let arr = foo(o);
+    assert(arr.length === 1);
+    assert(arr[0] === "prop");
+}
+
+o.prop2 = "prop2";
+for (let i = 0; i < 1000; i++) {
+    let arr = foo(o);
+    assert(arr.length === 2);
+    assert(arr[0] === "prop");
+    assert(arr[1] === "prop2");
+}