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.
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:
- The protocol on Array.prototype hasn't changed (this is the same as the
introductory patch for op_spread).
- The base object's proto is Array.prototype
- The base object does not have indexed getters
- The base object does not have Symbol.iterator property.
(JSC::DFG::Graph::canDoFastSpread):
- dfg/DFGGraph.h:
- dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileSpread):
(JSC::FTL::DFG::LowerDFGToB3::compileSpread):
(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:
- 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.
- 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.
