JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Apr 2017 00:52:35 +0000 (00:52 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Apr 2017 00:52:35 +0000 (00:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171150
<rdar://problem/31771880>

Reviewed by Sam Weinig.

JSTests:

* stress/spread-optimized-properly.js: Added.
(assert):
(test):
(shallowEq):
(makeArrayIterator):
(test.bar):
(test.callback):
(test.arr.__proto__.Symbol.iterator):
(test.arr.Symbol.iterator):
(test.get bar):
(test.hackedNext):
(test.test):
(test.):

Source/JavaScriptCore:

This patch fixes a huge oversight from the patch that introduced
op_spread/Spread. The original patch did not account for the
base object having Symbol.iterator or getters that could
change the iterator protocol. This patch fixes the oversight both
in the C code, as well as the DFG/FTL backends. We only perform
the memcpy version of spread if we've proven that it's guaranteed
to be side-effect free (no indexed getters), and if the iterator
protocol is guaranteed to be the original protocol. To do this, we
must prove that:
1. The protocol on Array.prototype hasn't changed (this is the same as the
introductory patch for op_spread).
2. The base object's __proto__ is Array.prototype
3. The base object does not have indexed getters
4. The base object does not have Symbol.iterator property.

* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::canDoFastSpread):
* dfg/DFGGraph.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileSpread):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileSpread):
* runtime/JSArray.cpp:
(JSC::JSArray::isIteratorProtocolFastAndNonObservable):
* runtime/JSArray.h:
* runtime/JSArrayInlines.h:
(JSC::JSArray::isIteratorProtocolFastAndNonObservable): Deleted.
* runtime/JSGlobalObject.h:
* runtime/JSGlobalObjectInlines.h:
(JSC::JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable):
(JSC::JSGlobalObject::isArrayIteratorProtocolFastAndNonObservable): Deleted.

Source/WebCore:

This patch moves the sequence converters to use the now fixed
JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable test
inside JSC.

This patch also fixes a few bugs:
1. Converting to a sequence of numbers must prove that the JSArray
is filled only with Int32/Double. If there is a chance the array
contains objects, the conversion to a numeric IDLType can be observable
(via valueOf()), and can change the iterator protocol.
2. There are other conversions that can have side effects a-la valueOf().
This patch introduces a new static constant in the various Converter
classes that tell the sequence converter if the conversion operation
can have JS side effects. If it does have side effects, we fall back to
the generic conversion that uses the iterator protocol. If not, we can
do a faster version that iterates over each element of the array,
reading it directly, and converting it.

Tests: js/sequence-iterator-protocol-2.html
       js/sequence-iterator-protocol.html

* bindings/js/JSDOMConvertAny.h: Does not have side effects.
* bindings/js/JSDOMConvertBase.h: We pessimistically assume inside DefaultConverter that converions have side effects.
* bindings/js/JSDOMConvertBoolean.h: Does not have side effects.
* bindings/js/JSDOMConvertCallbacks.h: Does not have side effects.
* bindings/js/JSDOMConvertObject.h: Does not have side effects.
* bindings/js/JSDOMConvertSequences.h:
(WebCore::Detail::NumericSequenceConverter::convert):
(WebCore::Detail::SequenceConverter::convert):

LayoutTests:

* js/sequence-iterator-protocol-2-expected.txt: Added.
* js/sequence-iterator-protocol-2.html: Added.
* js/sequence-iterator-protocol-expected.txt: Added.
* js/sequence-iterator-protocol.html: Added.

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

24 files changed:
JSTests/ChangeLog
JSTests/stress/spread-optimized-properly.js [new file with mode: 0644]
LayoutTests/ChangeLog
LayoutTests/js/sequence-iterator-protocol-2-expected.txt [new file with mode: 0644]
LayoutTests/js/sequence-iterator-protocol-2.html [new file with mode: 0644]
LayoutTests/js/sequence-iterator-protocol-expected.txt [new file with mode: 0644]
LayoutTests/js/sequence-iterator-protocol.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGGraph.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSArray.h
Source/JavaScriptCore/runtime/JSArrayInlines.h
Source/JavaScriptCore/runtime/JSGlobalObject.h
Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMConvertAny.h
Source/WebCore/bindings/js/JSDOMConvertBase.h
Source/WebCore/bindings/js/JSDOMConvertBoolean.h
Source/WebCore/bindings/js/JSDOMConvertCallbacks.h
Source/WebCore/bindings/js/JSDOMConvertObject.h
Source/WebCore/bindings/js/JSDOMConvertSequences.h

index 25b0713..767d77b 100644 (file)
@@ -1,3 +1,25 @@
+2017-04-25  Saam Barati  <sbarati@apple.com>
+
+        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
+        https://bugs.webkit.org/show_bug.cgi?id=171150
+        <rdar://problem/31771880>
+
+        Reviewed by Sam Weinig.
+
+        * stress/spread-optimized-properly.js: Added.
+        (assert):
+        (test):
+        (shallowEq):
+        (makeArrayIterator):
+        (test.bar):
+        (test.callback):
+        (test.arr.__proto__.Symbol.iterator):
+        (test.arr.Symbol.iterator):
+        (test.get bar):
+        (test.hackedNext):
+        (test.test):
+        (test.):
+
 2017-04-25  Mark Lam  <mark.lam@apple.com>
 
         [Follow up] Array.prototype.slice() should ensure that end >= begin.
diff --git a/JSTests/stress/spread-optimized-properly.js b/JSTests/stress/spread-optimized-properly.js
new file mode 100644 (file)
index 0000000..120fe9e
--- /dev/null
@@ -0,0 +1,155 @@
+function assert(b) {
+    if (!b)
+        throw new Error("Bad assertion");
+}
+
+function test(f) { f(); }
+
+function shallowEq(a, b) {
+    if (a.length !== b.length)
+        return false;
+    for (let i = 0; i < a.length; ++i) {
+        if (a[i] !== b[i])
+            return false;
+    }
+    return true;
+}
+
+function makeArrayIterator(arr, f) {
+    let i = 0;
+    return {
+        next() {
+            f();
+            if (i >= arr.length)
+                return {done: true};
+            return {value: arr[i++], done: false};
+        }
+    };
+} 
+
+test(function() {
+    let arr = [10, 20];
+    arr.__proto__ = {[Symbol.iterator]: Array.prototype[Symbol.iterator]};
+    function bar(a) {
+        a.x;
+        return [...a];
+    }
+    noInline(bar);
+    for (let i = 0; i < 10000; i++) {
+        assert(shallowEq(bar(arr), arr));
+    }
+});
+
+test(function() {
+    let arr = [10, 20];
+    let count = 0;
+    function callback() {
+        count++;
+    }
+
+    arr.__proto__ = {
+        [Symbol.iterator]: function() {
+            return makeArrayIterator(this, callback);
+        }
+    };
+
+    function bar(a) {
+        a.x;
+        return [...a];
+    }
+    noInline(bar);
+    for (let i = 0; i < 10000; i++) {
+        let t = bar(arr);
+        assert(count === 3);
+        count = 0;
+        assert(shallowEq(t, arr));
+    }
+});
+
+test(function() {
+    let arr = [10, 20];
+    let count = 0;
+    function callback() {
+        count++;
+    }
+
+    arr[Symbol.iterator] = function() {
+        return makeArrayIterator(this, callback);
+    };
+
+    function bar(a) {
+        a.x;
+        return [...a];
+    }
+    noInline(bar);
+    for (let i = 0; i < 10000; i++) {
+        let t = bar(arr);
+        assert(count === 3);
+        count = 0;
+        assert(shallowEq(t, arr));
+    }
+});
+
+test(function() {
+    let arr = [10, 20];
+    arr[Symbol.iterator] = Array.prototype[Symbol.iterator];
+    function bar(a) {
+        a.x;
+        return [...a];
+    }
+    noInline(bar);
+    for (let i = 0; i < 10000; i++) {
+        assert(shallowEq(bar(arr), arr));
+    }
+});
+
+test(function() {
+    let arr = [, 20];
+    let callCount = 0;
+    Object.defineProperty(arr, 0, {get() { ++callCount; return 10; }});
+    function bar(a) {
+        a.x;
+        return [...a];
+    }
+    noInline(bar);
+    for (let i = 0; i < 10000; i++) {
+        let t = bar(arr);
+        assert(callCount === 1);
+        assert(shallowEq(t, arr));
+        assert(callCount === 2);
+        callCount = 0;
+    }
+});
+
+// We run this test last since it fires watchpoints for the protocol.
+test(function() {
+    let iter = [][Symbol.iterator]();
+    let iterProto = Object.getPrototypeOf(iter);
+    let oldNext = iterProto.next;
+
+    function hackedNext() {
+        let val = oldNext.call(this);
+        if ("value" in val) {
+            val.value++;
+        }
+        return val;
+    }
+
+    function test(a) {
+        a.x;
+        return [...a];
+    }
+
+    for (let i = 0; i < 10000; ++i) {
+        let arr = [1,,3];
+        let callCount = 0;
+        Object.defineProperty(arr, 1, { get: function() { ++callCount; iterProto.next = hackedNext; return 2; } });
+        let t = test(arr);
+        assert(callCount === 1);
+        assert(t.length === 3);
+        assert(t[0] === 1);
+        assert(t[1] === 2);
+        assert(t[2] === 4);
+        iterProto.next = oldNext;
+    }
+});
index 187b238..c394ed3 100644 (file)
@@ -1,3 +1,16 @@
+2017-04-25  Saam Barati  <sbarati@apple.com>
+
+        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
+        https://bugs.webkit.org/show_bug.cgi?id=171150
+        <rdar://problem/31771880>
+
+        Reviewed by Sam Weinig.
+
+        * js/sequence-iterator-protocol-2-expected.txt: Added.
+        * js/sequence-iterator-protocol-2.html: Added.
+        * js/sequence-iterator-protocol-expected.txt: Added.
+        * js/sequence-iterator-protocol.html: Added.
+
 2017-04-25  Ryan Haddad  <ryanhaddad@apple.com>
 
         Mark media/modern-media-controls/pip-support/pip-support-click.html as flaky.
diff --git a/LayoutTests/js/sequence-iterator-protocol-2-expected.txt b/LayoutTests/js/sequence-iterator-protocol-2-expected.txt
new file mode 100644 (file)
index 0000000..a99e588
--- /dev/null
@@ -0,0 +1,11 @@
+CONSOLE MESSAGE: line 28: Using an array but with objects with valueOf()
+CONSOLE MESSAGE: line 29: 1,2,3,1,2,3
+CONSOLE MESSAGE: line 33: 1,2,3,1,2,3
+CONSOLE MESSAGE: line 38: callCount: 3
+CONSOLE MESSAGE: line 66: 1,2,4,1,2,4
+CONSOLE MESSAGE: line 71: 1,2,4,1,2,4
+CONSOLE MESSAGE: line 75: callCount: 2
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/sequence-iterator-protocol-2.html b/LayoutTests/js/sequence-iterator-protocol-2.html
new file mode 100644 (file)
index 0000000..ee243a9
--- /dev/null
@@ -0,0 +1,84 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <pre id='console'></pre>
+        <canvas></canvas>
+
+        <script>
+            "use strict";
+
+            const log = console.log.bind(console);
+            const canvas = document.querySelector("canvas");
+            const ctx = canvas.getContext("2d");
+
+            let callCount = 0;
+            function makeValue(x) {
+                return {
+                    valueOf() { ++callCount; return x; }
+                };
+            }
+
+            let array = [makeValue(1), makeValue(2), makeValue(3)];
+            ctx.setLineDash(array);
+            let a = ctx.getLineDash();
+            log("Using an array but with objects with valueOf()");
+            log(a);
+
+            ctx.setLineDash([1,2,3]);
+            let b = ctx.getLineDash();
+            log(b);
+
+            if (a.toString() !== b.toString())
+                throw new Error("Bad result. They should be equal.");
+
+            log(`callCount: ${callCount}`);
+            if (callCount !== 3)
+                throw new Error("Bad result. callCount should be 3.");
+
+
+            let iter = [][Symbol.iterator]();
+            let iterProto = Object.getPrototypeOf(iter);
+            let oldNext = iterProto.next;
+
+            callCount = 0;
+            function hackedNext() {
+                ++callCount;
+
+                let val = oldNext.call(this);
+                if ("value" in val) {
+                    val.value++;
+                }
+                return val;
+            }
+            const obj = {
+                valueOf: function() {
+                    iterProto.next = hackedNext;
+                    return 2;
+                }
+            };
+            array = [1, obj, 3];
+            ctx.setLineDash(array);
+            b = ctx.getLineDash();
+            log(`${b}`);
+
+            iterProto.next = oldNext;
+            ctx.setLineDash([1,2,4]);
+            a = ctx.getLineDash();
+            log(a);
+            if (a.toString() !== b.toString())
+                throw new Error("Bad result. They should be equal.");
+
+            log(`callCount: ${callCount}`);
+            if (callCount !== 2)
+                throw new Error("Bad result. callCount should be 2.");
+
+        </script>
+
+        <script src="../resources/js-test-post.js"></script>
+
+    </body>
+</html>
diff --git a/LayoutTests/js/sequence-iterator-protocol-expected.txt b/LayoutTests/js/sequence-iterator-protocol-expected.txt
new file mode 100644 (file)
index 0000000..e31b0bd
--- /dev/null
@@ -0,0 +1,10 @@
+CONSOLE MESSAGE: line 19: 
+CONSOLE MESSAGE: line 22: Using an array: 1,2,3,1,2,3
+CONSOLE MESSAGE: line 33: Using an iterator: 10,11,12,10,11,12
+CONSOLE MESSAGE: line 41: Using an array but with customized iteration (should be same as iterator): 10,11,12,10,11,12
+The second and third results should be the same.
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/sequence-iterator-protocol.html b/LayoutTests/js/sequence-iterator-protocol.html
new file mode 100644 (file)
index 0000000..cf9ef82
--- /dev/null
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <p>The second and third results should be the same.</p>
+        <pre id='console'></pre>
+        <canvas></canvas>
+
+        <script>
+            "use strict";
+
+            const log = console.log.bind(console);
+
+            const canvas = document.querySelector("canvas");
+            const ctx = canvas.getContext("2d");
+            log(ctx.getLineDash());
+
+            ctx.setLineDash([1, 2, 3]);
+            log("Using an array: " + ctx.getLineDash());
+
+            function* generator() {
+                yield 10;
+                yield 11;
+                yield 12;
+            }
+
+            const iterator = generator();
+            ctx.setLineDash(iterator);
+            let a = ctx.getLineDash();
+            log("Using an iterator: " + a);
+
+            const array = [1, 2, 3];
+            Object.defineProperty(array, Symbol.iterator, {
+                value: () => generator()
+            });
+            ctx.setLineDash(array);
+            let b = ctx.getLineDash();
+            log("Using an array but with customized iteration (should be same as iterator): " + b);
+
+            if (a.toString() !== b.toString())
+                throw new Error("Bad result. They should be equal.");
+
+        </script>
+
+        <script src="../resources/js-test-post.js"></script>
+
+    </body>
+</html>
index 9c25c1c..a7e4c9a 100644 (file)
@@ -1,3 +1,43 @@
+2017-04-25  Saam Barati  <sbarati@apple.com>
+
+        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
+        https://bugs.webkit.org/show_bug.cgi?id=171150
+        <rdar://problem/31771880>
+
+        Reviewed by Sam Weinig.
+
+        This patch fixes a huge oversight from the patch that introduced
+        op_spread/Spread. The original patch did not account for the
+        base object having Symbol.iterator or getters that could
+        change the iterator protocol. This patch fixes the oversight both
+        in the C code, as well as the DFG/FTL backends. We only perform
+        the memcpy version of spread if we've proven that it's guaranteed
+        to be side-effect free (no indexed getters), and if the iterator
+        protocol is guaranteed to be the original protocol. To do this, we
+        must prove that:
+        1. The protocol on Array.prototype hasn't changed (this is the same as the
+        introductory patch for op_spread).
+        2. The base object's __proto__ is Array.prototype
+        3. The base object does not have indexed getters
+        4. The base object does not have Symbol.iterator property.
+
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::canDoFastSpread):
+        * dfg/DFGGraph.h:
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileSpread):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileSpread):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::isIteratorProtocolFastAndNonObservable):
+        * runtime/JSArray.h:
+        * runtime/JSArrayInlines.h:
+        (JSC::JSArray::isIteratorProtocolFastAndNonObservable): Deleted.
+        * runtime/JSGlobalObject.h:
+        * runtime/JSGlobalObjectInlines.h:
+        (JSC::JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable):
+        (JSC::JSGlobalObject::isArrayIteratorProtocolFastAndNonObservable): Deleted.
+
 2017-04-25  Mark Lam  <mark.lam@apple.com>
 
         Array.prototype.slice() should ensure that end >= begin.
index 34abd4d..05e9c1c 100644 (file)
@@ -1723,6 +1723,37 @@ bool Graph::willCatchExceptionInMachineFrame(CodeOrigin codeOrigin, CodeOrigin&
     RELEASE_ASSERT_NOT_REACHED();
 }
 
+bool Graph::canDoFastSpread(Node* node, const AbstractValue& value)
+{
+    // The parameter 'value' is the AbstractValue for child1 (the thing being spread).
+    ASSERT(node->op() == Spread);
+
+    if (node->child1().useKind() != ArrayUse) {
+        // Note: we only speculate on ArrayUse when we've set up the necessary watchpoints
+        // to prove that the iteration protocol is non-observable starting from ArrayPrototype.
+        return false;
+    }
+
+    // FIXME: We should add profiling of the incoming operand to Spread
+    // so we can speculate in such a way that we guarantee that this
+    // function would return true:
+    // https://bugs.webkit.org/show_bug.cgi?id=171198
+
+    if (!value.m_structure.isFinite())
+        return false;
+
+    ArrayPrototype* arrayPrototype = globalObjectFor(node->child1()->origin.semantic)->arrayPrototype();
+    bool allGood = true;
+    value.m_structure.forEach([&] (RegisteredStructure structure) {
+        allGood &= structure->storedPrototype() == arrayPrototype
+            && !structure->isDictionary()
+            && structure->getConcurrently(m_vm.propertyNames->iteratorSymbol.impl()) == invalidOffset
+            && !structure->mayInterceptIndexedAccesses();
+    });
+
+    return allGood;
+}
+
 } } // namespace JSC::DFG
 
 #endif // ENABLE(DFG_JIT)
index 8431d48..b476b1f 100644 (file)
@@ -884,6 +884,8 @@ public:
     
     JSArrayBufferView* tryGetFoldableView(JSValue);
     JSArrayBufferView* tryGetFoldableView(JSValue, ArrayMode arrayMode);
+
+    bool canDoFastSpread(Node*, const AbstractValue&);
     
     void registerFrozenValues();
     
index 82dcf8d..4700994 100644 (file)
@@ -7033,11 +7033,10 @@ void SpeculativeJIT::compileSpread(Node* node)
     SpeculateCellOperand operand(this, node->child1());
     GPRReg argument = operand.gpr();
 
-    if (node->child1().useKind() == ArrayUse) {
-        // Note: we only speculate on ArrayUse when we've set up the necessary watchpoints
-        // to prove that the iteration protocol is non-observable.
+    if (node->child1().useKind() == ArrayUse)
         speculateArray(node->child1(), argument);
 
+    if (m_jit.graph().canDoFastSpread(node, m_state.forNode(node->child1()))) {
 #if USE(JSVALUE64)
         GPRTemporary result(this);
         GPRTemporary scratch1(this);
index 20e04c4..b4c6724 100644 (file)
@@ -4676,9 +4676,11 @@ private:
         LValue argument = lowCell(m_node->child1());
 
         LValue result;
-        if (m_node->child1().useKind() == ArrayUse) {
+
+        if (m_node->child1().useKind() == ArrayUse)
             speculateArray(m_node->child1());
 
+        if (m_graph.canDoFastSpread(m_node, m_state.forNode(m_node->child1()))) {
             LBasicBlock preLoop = m_out.newBlock();
             LBasicBlock loopSelection = m_out.newBlock();
             LBasicBlock contiguousLoopStart = m_out.newBlock();
index 7d25267..3c26058 100644 (file)
@@ -1403,4 +1403,27 @@ void JSArray::copyToArguments(ExecState* exec, VirtualRegister firstElementDest,
     }
 }
 
+bool JSArray::isIteratorProtocolFastAndNonObservable()
+{
+    JSGlobalObject* globalObject = this->globalObject();
+    if (!globalObject->isArrayPrototypeIteratorProtocolFastAndNonObservable())
+        return false;
+
+    Structure* structure = this->structure();
+    // This is the fast case. Many arrays will be an original array.
+    if (globalObject->isOriginalArrayStructure(structure))
+        return true;
+
+    if (structure->mayInterceptIndexedAccesses())
+        return false;
+
+    if (structure->storedPrototype() != globalObject->arrayPrototype())
+        return false;
+
+    if (getDirectOffset(globalObject->vm(), globalObject->vm().propertyNames->iteratorSymbol) != invalidOffset)
+        return false;
+
+    return true;
+}
+
 } // namespace JSC
index aeaa10b..f7284dd 100644 (file)
@@ -150,7 +150,7 @@ public:
     JS_EXPORT_PRIVATE void fillArgList(ExecState*, MarkedArgumentBuffer&);
     JS_EXPORT_PRIVATE void copyToArguments(ExecState*, VirtualRegister firstElementDest, unsigned offset, unsigned length);
 
-    bool isIteratorProtocolFastAndNonObservable();
+    JS_EXPORT_PRIVATE bool isIteratorProtocolFastAndNonObservable();
 
     static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype, IndexingType indexingType)
     {
index 30d7e4f..8465177 100644 (file)
@@ -93,9 +93,4 @@ ALWAYS_INLINE double toLength(ExecState* exec, JSObject* obj)
     return lengthValue.toLength(exec);
 }
 
-ALWAYS_INLINE bool JSArray::isIteratorProtocolFastAndNonObservable()
-{
-    return globalObject()->isArrayIteratorProtocolFastAndNonObservable();
-}
-
 } // namespace JSC
index a286339..a835e3f 100644 (file)
@@ -412,7 +412,7 @@ public:
     std::unique_ptr<ArrayIteratorAdaptiveWatchpoint> m_arrayPrototypeSymbolIteratorWatchpoint;
     std::unique_ptr<ArrayIteratorAdaptiveWatchpoint> m_arrayIteratorPrototypeNext;
 
-    bool isArrayIteratorProtocolFastAndNonObservable();
+    bool isArrayPrototypeIteratorProtocolFastAndNonObservable();
 
     TemplateRegistry m_templateRegistry;
 
index 7e334eb..4c8287b 100644 (file)
@@ -53,7 +53,7 @@ ALWAYS_INLINE bool JSGlobalObject::stringPrototypeChainIsSane()
 }
 
 
-ALWAYS_INLINE bool JSGlobalObject::isArrayIteratorProtocolFastAndNonObservable()
+ALWAYS_INLINE bool JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable()
 {
     // We're fast if we don't have any indexed properties on the prototype.
     // We're non-observable if the iteration protocol hasn't changed.
index 5f77a4d..a7c910b 100644 (file)
@@ -1,3 +1,40 @@
+2017-04-25  Saam Barati  <sbarati@apple.com>
+
+        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
+        https://bugs.webkit.org/show_bug.cgi?id=171150
+        <rdar://problem/31771880>
+
+        Reviewed by Sam Weinig.
+
+        This patch moves the sequence converters to use the now fixed
+        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable test
+        inside JSC.
+        
+        This patch also fixes a few bugs:
+        1. Converting to a sequence of numbers must prove that the JSArray
+        is filled only with Int32/Double. If there is a chance the array
+        contains objects, the conversion to a numeric IDLType can be observable
+        (via valueOf()), and can change the iterator protocol.
+        2. There are other conversions that can have side effects a-la valueOf().
+        This patch introduces a new static constant in the various Converter
+        classes that tell the sequence converter if the conversion operation
+        can have JS side effects. If it does have side effects, we fall back to
+        the generic conversion that uses the iterator protocol. If not, we can
+        do a faster version that iterates over each element of the array,
+        reading it directly, and converting it.
+
+        Tests: js/sequence-iterator-protocol-2.html
+               js/sequence-iterator-protocol.html
+
+        * bindings/js/JSDOMConvertAny.h: Does not have side effects.
+        * bindings/js/JSDOMConvertBase.h: We pessimistically assume inside DefaultConverter that converions have side effects.
+        * bindings/js/JSDOMConvertBoolean.h: Does not have side effects.
+        * bindings/js/JSDOMConvertCallbacks.h: Does not have side effects.
+        * bindings/js/JSDOMConvertObject.h: Does not have side effects.
+        * bindings/js/JSDOMConvertSequences.h:
+        (WebCore::Detail::NumericSequenceConverter::convert):
+        (WebCore::Detail::SequenceConverter::convert):
+
 2017-04-25  Michael Saboff  <msaboff@apple.com>
 
         Call bmalloc scavenger first when handling a memory pressure event
index b27ab7c..4980fa0 100644 (file)
@@ -32,7 +32,9 @@ namespace WebCore {
 
 template<> struct Converter<IDLAny> : DefaultConverter<IDLAny> {
     using ReturnType = JSC::JSValue;
-    
+
+    static constexpr bool conversionHasSideEffects = false;
+
     static JSC::JSValue convert(JSC::ExecState&, JSC::JSValue value)
     {
         return value;
index bd23abe..dc1f1ce 100644 (file)
@@ -178,6 +178,16 @@ template<typename T, typename U> inline JSC::JSValue toJSNewlyCreated(JSC::ExecS
 
 template<typename T> struct DefaultConverter {
     using ReturnType = typename T::ImplementationType;
+
+    // We assume the worst, subtypes can override to be less pessimistic.
+    // An example of something that can have side effects
+    // is having a converter that does JSC::JSValue::toNumber.
+    // toNumber() in JavaScript can call arbitrary JS functions.
+    //
+    // An example of something that does not have side effects
+    // is something having a converter that does JSC::JSValue::toBoolean.
+    // toBoolean() in JS can't call arbitrary functions.
+    static constexpr bool conversionHasSideEffects = true;
 };
 
 } // namespace WebCore
index 853be0c..e6363f5 100644 (file)
@@ -31,6 +31,9 @@
 namespace WebCore {
 
 template<> struct Converter<IDLBoolean> : DefaultConverter<IDLBoolean> {
+
+    static constexpr bool conversionHasSideEffects = false;
+
     static bool convert(JSC::ExecState& state, JSC::JSValue value)
     {
         return value.toBoolean(&state);
index 3c1397a..570e84c 100644 (file)
@@ -31,6 +31,9 @@
 namespace WebCore {
 
 template<typename T> struct Converter<IDLCallbackFunction<T>> : DefaultConverter<IDLCallbackFunction<T>> {
+
+    static constexpr bool conversionHasSideEffects = false;
+
     template<typename ExceptionThrower = DefaultExceptionThrower>
     static RefPtr<T> convert(JSC::ExecState& state, JSC::JSValue value, JSDOMGlobalObject& globalObject, ExceptionThrower&& exceptionThrower = ExceptionThrower())
     {
index 379eb42..1bd9809 100644 (file)
@@ -31,6 +31,9 @@
 namespace WebCore {
 
 template<> struct Converter<IDLObject> : DefaultConverter<IDLObject> {
+
+    static constexpr bool conversionHasSideEffects = false;
+
     template<typename ExceptionThrower = DefaultExceptionThrower>
     static JSC::Strong<JSC::JSObject> convert(JSC::ExecState& state, JSC::JSValue value, ExceptionThrower&& exceptionThrower = ExceptionThrower())
     {
index f42f459..3a47067 100644 (file)
@@ -41,7 +41,11 @@ struct GenericSequenceConverter {
 
     static ReturnType convert(JSC::ExecState& state, JSC::JSObject* jsObject)
     {
-        ReturnType result;
+        return convert(state, jsObject, ReturnType());
+    }
+
+    static ReturnType convert(JSC::ExecState& state, JSC::JSObject* jsObject, ReturnType&& result)
+    {
         forEachInIterable(&state, jsObject, [&result](JSC::VM& vm, JSC::ExecState* state, JSC::JSValue jsValue) {
             auto scope = DECLARE_THROW_SCOPE(vm);
 
@@ -78,35 +82,27 @@ struct NumericSequenceConverter {
             return GenericConverter::convert(state, object);
 
         JSC::JSArray* array = JSC::asArray(object);
-        if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
+        if (!array->isIteratorProtocolFastAndNonObservable())
             return GenericConverter::convert(state, object);
 
         unsigned length = array->length();
-
         ReturnType result;
+        // If we're not an int32/double array, it's possible that converting a
+        // JSValue to a number could cause the iterator protocol to change, hence,
+        // we may need more capacity, or less. In such cases, we use the length
+        // as a proxy for the capacity we will most likely need (it's unlikely that 
+        // a program is written with a valueOf that will augment the iterator protocol).
+        // If we are an int32/double array, then length is precisely the capacity we need.
         if (!result.tryReserveCapacity(length)) {
             // FIXME: Is the right exception to throw?
             throwTypeError(&state, scope);
             return { };
         }
-
+        
         JSC::IndexingType indexingType = array->indexingType() & JSC::IndexingShapeMask;
+        if (indexingType != JSC::Int32Shape && indexingType != JSC::DoubleShape)
+            return GenericConverter::convert(state, object, WTFMove(result));
 
-        if (indexingType == JSC::ContiguousShape) {
-            for (unsigned i = 0; i < length; i++) {
-                auto indexValue = array->butterfly()->contiguous()[i].get();
-                if (!indexValue)
-                    result.uncheckedAppend(0);
-                else {
-                    auto convertedValue = Converter<IDLType>::convert(state, indexValue);
-                    RETURN_IF_EXCEPTION(scope, { });
-
-                    result.uncheckedAppend(convertedValue);
-                }
-            }
-            return result;
-        }
-        
         if (indexingType == JSC::Int32Shape) {
             for (unsigned i = 0; i < length; i++) {
                 auto indexValue = array->butterfly()->contiguousInt32()[i].get();
@@ -119,31 +115,15 @@ struct NumericSequenceConverter {
             return result;
         }
 
-        if (indexingType == JSC::DoubleShape) {
-            for (unsigned i = 0; i < length; i++) {
-                auto doubleValue = array->butterfly()->contiguousDouble()[i];
-                if (std::isnan(doubleValue))
-                    result.uncheckedAppend(0);
-                else {
-                    auto convertedValue = Converter<IDLType>::convert(state, scope, doubleValue);
-                    RETURN_IF_EXCEPTION(scope, { });
-
-                    result.uncheckedAppend(convertedValue);
-                }
-            }
-            return result;
-        }
-
+        ASSERT(indexingType == JSC::DoubleShape);
         for (unsigned i = 0; i < length; i++) {
-            auto indexValue = array->getDirectIndex(&state, i);
-            RETURN_IF_EXCEPTION(scope, { });
-            
-            if (!indexValue)
+            auto doubleValue = array->butterfly()->contiguousDouble()[i];
+            if (std::isnan(doubleValue))
                 result.uncheckedAppend(0);
             else {
-                auto convertedValue = Converter<IDLType>::convert(state, indexValue);
+                auto convertedValue = Converter<IDLType>::convert(state, scope, doubleValue);
                 RETURN_IF_EXCEPTION(scope, { });
-                
+
                 result.uncheckedAppend(convertedValue);
             }
         }
@@ -167,11 +147,14 @@ struct SequenceConverter {
         }
 
         JSC::JSObject* object = JSC::asObject(value);
+        if (Converter<IDLType>::conversionHasSideEffects)
+            return GenericConverter::convert(state, object);
+
         if (!JSC::isJSArray(object))
             return GenericConverter::convert(state, object);
 
         JSC::JSArray* array = JSC::asArray(object);
-        if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
+        if (!array->isIteratorProtocolFastAndNonObservable())
             return GenericConverter::convert(state, object);
 
         unsigned length = array->length();