Changeset 215777 in webkit


Ignore:
Timestamp:
Apr 25, 2017 5:52:35 PM (7 years ago)
Author:
sbarati@apple.com
Message:

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:

  1. The protocol on Array.prototype hasn't changed (this is the same as the

introductory patch for op_spread).

  1. The base object's proto is Array.prototype
  2. The base object does not have indexed getters
  3. 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.

  1. 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.
Location:
trunk
Files:
5 added
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r215770 r215777  
     12017-04-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
     4        https://bugs.webkit.org/show_bug.cgi?id=171150
     5        <rdar://problem/31771880>
     6
     7        Reviewed by Sam Weinig.
     8
     9        * stress/spread-optimized-properly.js: Added.
     10        (assert):
     11        (test):
     12        (shallowEq):
     13        (makeArrayIterator):
     14        (test.bar):
     15        (test.callback):
     16        (test.arr.__proto__.Symbol.iterator):
     17        (test.arr.Symbol.iterator):
     18        (test.get bar):
     19        (test.hackedNext):
     20        (test.test):
     21        (test.):
     22
    1232017-04-25  Mark Lam  <mark.lam@apple.com>
    224

  • trunk/LayoutTests/ChangeLog

    r215776 r215777  
     12017-04-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
     4        https://bugs.webkit.org/show_bug.cgi?id=171150
     5        <rdar://problem/31771880>
     6
     7        Reviewed by Sam Weinig.
     8
     9        * js/sequence-iterator-protocol-2-expected.txt: Added.
     10        * js/sequence-iterator-protocol-2.html: Added.
     11        * js/sequence-iterator-protocol-expected.txt: Added.
     12        * js/sequence-iterator-protocol.html: Added.
     13
    1142017-04-25  Ryan Haddad  <ryanhaddad@apple.com>
    215

  • trunk/Source/JavaScriptCore/ChangeLog

    r215768 r215777  
     12017-04-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
     4        https://bugs.webkit.org/show_bug.cgi?id=171150
     5        <rdar://problem/31771880>
     6
     7        Reviewed by Sam Weinig.
     8
     9        This patch fixes a huge oversight from the patch that introduced
     10        op_spread/Spread. The original patch did not account for the
     11        base object having Symbol.iterator or getters that could
     12        change the iterator protocol. This patch fixes the oversight both
     13        in the C code, as well as the DFG/FTL backends. We only perform
     14        the memcpy version of spread if we've proven that it's guaranteed
     15        to be side-effect free (no indexed getters), and if the iterator
     16        protocol is guaranteed to be the original protocol. To do this, we
     17        must prove that:
     18        1. The protocol on Array.prototype hasn't changed (this is the same as the
     19        introductory patch for op_spread).
     20        2. The base object's __proto__ is Array.prototype
     21        3. The base object does not have indexed getters
     22        4. The base object does not have Symbol.iterator property.
     23
     24        * dfg/DFGGraph.cpp:
     25        (JSC::DFG::Graph::canDoFastSpread):
     26        * dfg/DFGGraph.h:
     27        * dfg/DFGSpeculativeJIT.cpp:
     28        (JSC::DFG::SpeculativeJIT::compileSpread):
     29        * ftl/FTLLowerDFGToB3.cpp:
     30        (JSC::FTL::DFG::LowerDFGToB3::compileSpread):
     31        * runtime/JSArray.cpp:
     32        (JSC::JSArray::isIteratorProtocolFastAndNonObservable):
     33        * runtime/JSArray.h:
     34        * runtime/JSArrayInlines.h:
     35        (JSC::JSArray::isIteratorProtocolFastAndNonObservable): Deleted.
     36        * runtime/JSGlobalObject.h:
     37        * runtime/JSGlobalObjectInlines.h:
     38        (JSC::JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable):
     39        (JSC::JSGlobalObject::isArrayIteratorProtocolFastAndNonObservable): Deleted.
     40
    1412017-04-25  Mark Lam  <mark.lam@apple.com>
    242

  • trunk/Source/JavaScriptCore/dfg/DFGGraph.cpp

    r211247 r215777  
    17241724}
    17251725
     1726bool Graph::canDoFastSpread(Node* node, const AbstractValue& value)
     1727{
     1728    // The parameter 'value' is the AbstractValue for child1 (the thing being spread).
     1729    ASSERT(node->op() == Spread);
     1730
     1731    if (node->child1().useKind() != ArrayUse) {
     1732        // Note: we only speculate on ArrayUse when we've set up the necessary watchpoints
     1733        // to prove that the iteration protocol is non-observable starting from ArrayPrototype.
     1734        return false;
     1735    }
     1736
     1737    // FIXME: We should add profiling of the incoming operand to Spread
     1738    // so we can speculate in such a way that we guarantee that this
     1739    // function would return true:
     1740    // https://bugs.webkit.org/show_bug.cgi?id=171198
     1741
     1742    if (!value.m_structure.isFinite())
     1743        return false;
     1744
     1745    ArrayPrototype* arrayPrototype = globalObjectFor(node->child1()->origin.semantic)->arrayPrototype();
     1746    bool allGood = true;
     1747    value.m_structure.forEach([&] (RegisteredStructure structure) {
     1748        allGood &= structure->storedPrototype() == arrayPrototype
     1749            && !structure->isDictionary()
     1750            && structure->getConcurrently(m_vm.propertyNames->iteratorSymbol.impl()) == invalidOffset
     1751            && !structure->mayInterceptIndexedAccesses();
     1752    });
     1753
     1754    return allGood;
     1755}
     1756
    17261757} } // namespace JSC::DFG
    17271758

  • trunk/Source/JavaScriptCore/dfg/DFGGraph.h

    r214323 r215777  
    885885    JSArrayBufferView* tryGetFoldableView(JSValue);
    886886    JSArrayBufferView* tryGetFoldableView(JSValue, ArrayMode arrayMode);
     887
     888    bool canDoFastSpread(Node*, const AbstractValue&);
    887889   
    888890    void registerFrozenValues();

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

    r215600 r215777  
    70347034    GPRReg argument = operand.gpr();
    70357035
    7036     if (node->child1().useKind() == ArrayUse) {
    7037         // Note: we only speculate on ArrayUse when we've set up the necessary watchpoints
    7038         // to prove that the iteration protocol is non-observable.
     7036    if (node->child1().useKind() == ArrayUse)
    70397037        speculateArray(node->child1(), argument);
    70407038
     7039    if (m_jit.graph().canDoFastSpread(node, m_state.forNode(node->child1()))) {
    70417040#if USE(JSVALUE64)
    70427041        GPRTemporary result(this);

  • trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

    r215720 r215777  
    46774677
    46784678        LValue result;
    4679         if (m_node->child1().useKind() == ArrayUse) {
     4679
     4680        if (m_node->child1().useKind() == ArrayUse)
    46804681            speculateArray(m_node->child1());
    46814682
     4683        if (m_graph.canDoFastSpread(m_node, m_state.forNode(m_node->child1()))) {
    46824684            LBasicBlock preLoop = m_out.newBlock();
    46834685            LBasicBlock loopSelection = m_out.newBlock();

  • trunk/Source/JavaScriptCore/runtime/JSArray.cpp

    r215451 r215777  
    14041404}
    14051405
     1406bool JSArray::isIteratorProtocolFastAndNonObservable()
     1407{
     1408    JSGlobalObject* globalObject = this->globalObject();
     1409    if (!globalObject->isArrayPrototypeIteratorProtocolFastAndNonObservable())
     1410        return false;
     1411
     1412    Structure* structure = this->structure();
     1413    // This is the fast case. Many arrays will be an original array.
     1414    if (globalObject->isOriginalArrayStructure(structure))
     1415        return true;
     1416
     1417    if (structure->mayInterceptIndexedAccesses())
     1418        return false;
     1419
     1420    if (structure->storedPrototype() != globalObject->arrayPrototype())
     1421        return false;
     1422
     1423    if (getDirectOffset(globalObject->vm(), globalObject->vm().propertyNames->iteratorSymbol) != invalidOffset)
     1424        return false;
     1425
     1426    return true;
     1427}
     1428
    14061429} // namespace JSC

  • trunk/Source/JavaScriptCore/runtime/JSArray.h

    r215345 r215777  
    151151    JS_EXPORT_PRIVATE void copyToArguments(ExecState*, VirtualRegister firstElementDest, unsigned offset, unsigned length);
    152152
    153     bool isIteratorProtocolFastAndNonObservable();
     153    JS_EXPORT_PRIVATE bool isIteratorProtocolFastAndNonObservable();
    154154
    155155    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype, IndexingType indexingType)

  • trunk/Source/JavaScriptCore/runtime/JSArrayInlines.h

    r211070 r215777  
    9494}
    9595
    96 ALWAYS_INLINE bool JSArray::isIteratorProtocolFastAndNonObservable()
    97 {
    98     return globalObject()->isArrayIteratorProtocolFastAndNonObservable();
    99 }
    100 
    10196} // namespace JSC

  • trunk/Source/JavaScriptCore/runtime/JSGlobalObject.h

    r215673 r215777  
    413413    std::unique_ptr<ArrayIteratorAdaptiveWatchpoint> m_arrayIteratorPrototypeNext;
    414414
    415     bool isArrayIteratorProtocolFastAndNonObservable();
     415    bool isArrayPrototypeIteratorProtocolFastAndNonObservable();
    416416
    417417    TemplateRegistry m_templateRegistry;

  • trunk/Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h

    r208637 r215777  
    5454
    5555
    56 ALWAYS_INLINE bool JSGlobalObject::isArrayIteratorProtocolFastAndNonObservable()
     56ALWAYS_INLINE bool JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable()
    5757{
    5858    // We're fast if we don't have any indexed properties on the prototype.

  • trunk/Source/WebCore/ChangeLog

    r215775 r215777  
     12017-04-25  Saam Barati  <sbarati@apple.com>
     2
     3        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
     4        https://bugs.webkit.org/show_bug.cgi?id=171150
     5        <rdar://problem/31771880>
     6
     7        Reviewed by Sam Weinig.
     8
     9        This patch moves the sequence converters to use the now fixed
     10        JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable test
     11        inside JSC.
     12       
     13        This patch also fixes a few bugs:
     14        1. Converting to a sequence of numbers must prove that the JSArray
     15        is filled only with Int32/Double. If there is a chance the array
     16        contains objects, the conversion to a numeric IDLType can be observable
     17        (via valueOf()), and can change the iterator protocol.
     18        2. There are other conversions that can have side effects a-la valueOf().
     19        This patch introduces a new static constant in the various Converter
     20        classes that tell the sequence converter if the conversion operation
     21        can have JS side effects. If it does have side effects, we fall back to
     22        the generic conversion that uses the iterator protocol. If not, we can
     23        do a faster version that iterates over each element of the array,
     24        reading it directly, and converting it.
     25
     26        Tests: js/sequence-iterator-protocol-2.html
     27               js/sequence-iterator-protocol.html
     28
     29        * bindings/js/JSDOMConvertAny.h: Does not have side effects.
     30        * bindings/js/JSDOMConvertBase.h: We pessimistically assume inside DefaultConverter that converions have side effects.
     31        * bindings/js/JSDOMConvertBoolean.h: Does not have side effects.
     32        * bindings/js/JSDOMConvertCallbacks.h: Does not have side effects.
     33        * bindings/js/JSDOMConvertObject.h: Does not have side effects.
     34        * bindings/js/JSDOMConvertSequences.h:
     35        (WebCore::Detail::NumericSequenceConverter::convert):
     36        (WebCore::Detail::SequenceConverter::convert):
     37
    1382017-04-25  Michael Saboff  <msaboff@apple.com>
    239

  • trunk/Source/WebCore/bindings/js/JSDOMConvertAny.h

    r211821 r215777  
    3333template<> struct Converter<IDLAny> : DefaultConverter<IDLAny> {
    3434    using ReturnType = JSC::JSValue;
    35    
     35
     36    static constexpr bool conversionHasSideEffects = false;
     37
    3638    static JSC::JSValue convert(JSC::ExecState&, JSC::JSValue value)
    3739    {

  • trunk/Source/WebCore/bindings/js/JSDOMConvertBase.h

    r211821 r215777  
    179179template<typename T> struct DefaultConverter {
    180180    using ReturnType = typename T::ImplementationType;
     181
     182    // We assume the worst, subtypes can override to be less pessimistic.
     183    // An example of something that can have side effects
     184    // is having a converter that does JSC::JSValue::toNumber.
     185    // toNumber() in JavaScript can call arbitrary JS functions.
     186    //
     187    // An example of something that does not have side effects
     188    // is something having a converter that does JSC::JSValue::toBoolean.
     189    // toBoolean() in JS can't call arbitrary functions.
     190    static constexpr bool conversionHasSideEffects = true;
    181191};
    182192

  • trunk/Source/WebCore/bindings/js/JSDOMConvertBoolean.h

    r211821 r215777  
    3232
    3333template<> struct Converter<IDLBoolean> : DefaultConverter<IDLBoolean> {
     34
     35    static constexpr bool conversionHasSideEffects = false;
     36
    3437    static bool convert(JSC::ExecState& state, JSC::JSValue value)
    3538    {

  • trunk/Source/WebCore/bindings/js/JSDOMConvertCallbacks.h

    r211821 r215777  
    3232
    3333template<typename T> struct Converter<IDLCallbackFunction<T>> : DefaultConverter<IDLCallbackFunction<T>> {
     34
     35    static constexpr bool conversionHasSideEffects = false;
     36
    3437    template<typename ExceptionThrower = DefaultExceptionThrower>
    3538    static RefPtr<T> convert(JSC::ExecState& state, JSC::JSValue value, JSDOMGlobalObject& globalObject, ExceptionThrower&& exceptionThrower = ExceptionThrower())

  • trunk/Source/WebCore/bindings/js/JSDOMConvertObject.h

    r211821 r215777  
    3232
    3333template<> struct Converter<IDLObject> : DefaultConverter<IDLObject> {
     34
     35    static constexpr bool conversionHasSideEffects = false;
     36
    3437    template<typename ExceptionThrower = DefaultExceptionThrower>
    3538    static JSC::Strong<JSC::JSObject> convert(JSC::ExecState& state, JSC::JSValue value, ExceptionThrower&& exceptionThrower = ExceptionThrower())

  • trunk/Source/WebCore/bindings/js/JSDOMConvertSequences.h

    r211821 r215777  
    4242    static ReturnType convert(JSC::ExecState& state, JSC::JSObject* jsObject)
    4343    {
    44         ReturnType result;
     44        return convert(state, jsObject, ReturnType());
     45    }
     46
     47    static ReturnType convert(JSC::ExecState& state, JSC::JSObject* jsObject, ReturnType&& result)
     48    {
    4549        forEachInIterable(&state, jsObject, [&result](JSC::VM& vm, JSC::ExecState* state, JSC::JSValue jsValue) {
    4650            auto scope = DECLARE_THROW_SCOPE(vm);
    … …  
    7983
    8084        JSC::JSArray* array = JSC::asArray(object);
    81         if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
     85        if (!array->isIteratorProtocolFastAndNonObservable())
    8286            return GenericConverter::convert(state, object);
    8387
    8488        unsigned length = array->length();
    85 
    8689        ReturnType result;
     90        // If we're not an int32/double array, it's possible that converting a
     91        // JSValue to a number could cause the iterator protocol to change, hence,
     92        // we may need more capacity, or less. In such cases, we use the length
     93        // as a proxy for the capacity we will most likely need (it's unlikely that
     94        // a program is written with a valueOf that will augment the iterator protocol).
     95        // If we are an int32/double array, then length is precisely the capacity we need.
    8796        if (!result.tryReserveCapacity(length)) {
    8897            // FIXME: Is the right exception to throw?
    … …  
    9099            return { };
    91100        }
    92 
     101       
    93102        JSC::IndexingType indexingType = array->indexingType() & JSC::IndexingShapeMask;
    94 
    95         if (indexingType == JSC::ContiguousShape) {
    96             for (unsigned i = 0; i < length; i++) {
    97                 auto indexValue = array->butterfly()->contiguous()[i].get();
    98                 if (!indexValue)
    99                     result.uncheckedAppend(0);
    100                 else {
    101                     auto convertedValue = Converter<IDLType>::convert(state, indexValue);
    102                     RETURN_IF_EXCEPTION(scope, { });
    103 
    104                     result.uncheckedAppend(convertedValue);
    105                 }
    106             }
    107             return result;
    108         }
    109        
     103        if (indexingType != JSC::Int32Shape && indexingType != JSC::DoubleShape)
     104            return GenericConverter::convert(state, object, WTFMove(result));
     105
    110106        if (indexingType == JSC::Int32Shape) {
    111107            for (unsigned i = 0; i < length; i++) {
    … …  
    120116        }
    121117
    122         if (indexingType == JSC::DoubleShape) {
    123             for (unsigned i = 0; i < length; i++) {
    124                 auto doubleValue = array->butterfly()->contiguousDouble()[i];
    125                 if (std::isnan(doubleValue))
    126                     result.uncheckedAppend(0);
    127                 else {
    128                     auto convertedValue = Converter<IDLType>::convert(state, scope, doubleValue);
    129                     RETURN_IF_EXCEPTION(scope, { });
    130 
    131                     result.uncheckedAppend(convertedValue);
    132                 }
    133             }
    134             return result;
    135         }
    136 
     118        ASSERT(indexingType == JSC::DoubleShape);
    137119        for (unsigned i = 0; i < length; i++) {
    138             auto indexValue = array->getDirectIndex(&state, i);
    139             RETURN_IF_EXCEPTION(scope, { });
    140            
    141             if (!indexValue)
     120            auto doubleValue = array->butterfly()->contiguousDouble()[i];
     121            if (std::isnan(doubleValue))
    142122                result.uncheckedAppend(0);
    143123            else {
    144                 auto convertedValue = Converter<IDLType>::convert(state, indexValue);
     124                auto convertedValue = Converter<IDLType>::convert(state, scope, doubleValue);
    145125                RETURN_IF_EXCEPTION(scope, { });
    146                
     126
    147127                result.uncheckedAppend(convertedValue);
    148128            }
    … …  
    168148
    169149        JSC::JSObject* object = JSC::asObject(value);
     150        if (Converter<IDLType>::conversionHasSideEffects)
     151            return GenericConverter::convert(state, object);
     152
    170153        if (!JSC::isJSArray(object))
    171154            return GenericConverter::convert(state, object);
    172155
    173156        JSC::JSArray* array = JSC::asArray(object);
    174         if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
     157        if (!array->isIteratorProtocolFastAndNonObservable())
    175158            return GenericConverter::convert(state, object);
    176159
Note: See TracChangeset for help on using the changeset viewer.