Bug 171150 – JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Sam Weinig]

Created attachment 307835 [details]
Test Case (see console for results)

As noted in a jsbin posted in https://github.com/w3c/web-platform-tests/issues/5566 (http://jsbin.com/nemazayubu/edit?html,console,output), WebKit does not honor an iterator that added to an array instance when converting to a sequence (test case attached).

[Sam Weinig]

Created attachment 307838 [details]
Test Case

Updated test.

[Sam Weinig]

Saam, I'm using the isArrayIteratorProtocolFastAndNonObservable() function to determine if the there is an overriden iterator, but that only checks the prototype. Is there a good way to check the instance as well?

[Saam Barati]

(In reply to Sam Weinig from comment #2)
> Saam, I'm using the isArrayIteratorProtocolFastAndNonObservable() function
> to determine if the there is an overriden iterator, but that only checks the
> prototype. Is there a good way to check the instance as well?

Just read the code for spread and it also looks to suffer from this problem as well. I think there are a few ways to fix this, that I'll think about more. We could check if it's an original array structure, and we can also check if the object itself has the property. Perhaps we should do both, but the second check requires a hashtable lookup. Maybe it's worth it. I'll work to fix this when I'm at a computer (typing this from my phone).

[Saam Barati]

To clarify a bit:
I think that function on JSArray should do the checks for its clients. Clearly its wrong to return true even if the array has a self [Symbol.iterator] property.

[Boris Zbarsky]

For what it's worth, there's other unsoundness in the consumers of isArrayIteratorProtocolFastAndNonObservable.  I filed bug 171158 on the case that's not quite the same as this issue, but may end up being fixed by the changes here, depending on what checks end up happening on the array.

[Saam Barati]

(In reply to Boris Zbarsky from comment #5)
> For what it's worth, there's other unsoundness in the consumers of
> isArrayIteratorProtocolFastAndNonObservable.  I filed bug 171158 on the case
> that's not quite the same as this issue, but may end up being fixed by the
> changes here, depending on what checks end up happening on the array.

I'm pretty sure this is the same bug. Thanks for filing one though. I'll mark as duplicate once I verify it's the same.

[Radar WebKit Bug Importer]

<rdar://problem/31771880>

[Saam Barati]

(In reply to Saam Barati from comment #6)
> (In reply to Boris Zbarsky from comment #5)
> > For what it's worth, there's other unsoundness in the consumers of
> > isArrayIteratorProtocolFastAndNonObservable.  I filed bug 171158 on the case
> > that's not quite the same as this issue, but may end up being fixed by the
> > changes here, depending on what checks end up happening on the array.
> 
> I'm pretty sure this is the same bug. Thanks for filing one though. I'll
> mark as duplicate once I verify it's the same.

This is sort of the same bug. Really, the main issue here is that isArrayIteratorProtocolFastAndNonObservable() only considers the prototype chain. It doesn't bother checking the base object. This means a few things:
1. It doesn't check if the base object has indexed getters.
2. It doesn't check if the base object has the Array.prototype as its __proto__
3. It doesn't check if the base object has the Symbol.iterator property.

This is a pretty huge oversight by me. I'm not sure what I was thinking. I'll fix this shortly.

[Saam Barati]

Created attachment 307886 [details]
WIP

I suspect we want something like this for JSArray::isArrayIteratorProtocolFastAndNonObservable

[Saam Barati]

Comment on attachment 307886 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=307886&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:1423
> +    if (getDirectOffset(globalObject->vm(), globalObject->vm().propertyNames->iteratorSymbol) == invalidOffset)

should be !=

[Saam Barati]

Created attachment 307898 [details]
WIP

Might be the entirety of the fix.

[Saam Barati]

Comment on attachment 307898 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=307898&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:1427
> +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;
> +}

I'm fairly certain this is a sufficient proof, however, I'll think it over a bit more.

[Build Bot]

Comment on attachment 307898 [details]
WIP

Attachment 307898 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3582940

New failing tests:
ChakraCore.yaml/ChakraCore/test/es6/ES6SubclassableAsync.js.default
jsc-layout-tests.yaml/js/script-tests/arraybuffer-wrappers.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arraybuffer-wrappers.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/es6-function-properties.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/es6-function-properties.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-strict.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/arrowfunction-lexical-bind-arguments-non-strict.js.layout-no-ftl

[Sam Weinig]

Thanks for looking into this Saam! I think you will also have to change JSDOMConvertSequencess.h to call the JSArray:: isArrayIteratorProtocolFastAndNonObservable() explicitly, It is currently doing:

    if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())

in a few places.

[Sam Weinig]

(In reply to Sam Weinig from comment #14)
> Thanks for looking into this Saam! I think you will also have to change
> JSDOMConvertSequencess.h to call the JSArray::
> isArrayIteratorProtocolFastAndNonObservable() explicitly, It is currently
> doing:
> 
>     if
> (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
> 
> in a few places.

Nevermind, I see you did that in the WIP :).

[Boris Zbarsky]

It's not clear to me that the JSDOMConvertSequences uses of even the new isArrayIteratorProtocolFastAndNonObservable are sound.  The premise of that function is that it verifies that doing a spread on the array is safe at the point when the function is called, after which spread can do a fast path.

But sequence conversions don't do a spread.  They do the equivalent of:

  let converted = [];
  for (let val of arr) {
    converted.length++;
    converted[converted.length-1] = convert(val);
  }

and "convert" can have side-effects.  So consider the original testcase in this bug, but with "array" defined like so:

  var iter = [][Symbol.iterator]();
  var iterProto = Object.getPrototypeOf(iter);
  var oldNext = iterProto.next;

  function hackedNext() {
    var val = oldNext.call(this);
    if ("value" in val) {
      val.value++;
    }
    return val;
  }
  const obj = {
    valueOf: function() {
      iterProto.next = hackedNext;
      return 2;
    }
  };
  const array = [1, obj, 3]

and no custom iterators defined anywhere.  The output of ctx.getLineDash() should be [1,2,4], but I'm pretty sure it's [1,2,3] even with the WIP patch.

[Yusuke Suzuki]

(In reply to Boris Zbarsky from comment #16)
> It's not clear to me that the JSDOMConvertSequences uses of even the new
> isArrayIteratorProtocolFastAndNonObservable are sound.  The premise of that
> function is that it verifies that doing a spread on the array is safe at the
> point when the function is called, after which spread can do a fast path.
> 
> But sequence conversions don't do a spread.  They do the equivalent of:
> 
>   let converted = [];
>   for (let val of arr) {
>     converted.length++;
>     converted[converted.length-1] = convert(val);
>   }
> 
> and "convert" can have side-effects.  So consider the original testcase in
> this bug, but with "array" defined like so:
> 
>   var iter = [][Symbol.iterator]();
>   var iterProto = Object.getPrototypeOf(iter);
>   var oldNext = iterProto.next;
> 
>   function hackedNext() {
>     var val = oldNext.call(this);
>     if ("value" in val) {
>       val.value++;
>     }
>     return val;
>   }
>   const obj = {
>     valueOf: function() {
>       iterProto.next = hackedNext;
>       return 2;
>     }
>   };
>   const array = [1, obj, 3]
> 
> and no custom iterators defined anywhere.  The output of ctx.getLineDash()
> should be [1,2,4], but I'm pretty sure it's [1,2,3] even with the WIP patch.

When I talked about it with Saam and Sam at IRC, IIRC, we concluded that we should perform array type check too. If we can know that the array is Int32 shape / Double shape etc., we can ensure that conversion is side effect free.

[Saam Barati]

(In reply to Boris Zbarsky from comment #16)
> It's not clear to me that the JSDOMConvertSequences uses of even the new
> isArrayIteratorProtocolFastAndNonObservable are sound.  The premise of that
> function is that it verifies that doing a spread on the array is safe at the
> point when the function is called, after which spread can do a fast path.
> 
> But sequence conversions don't do a spread.  They do the equivalent of:
> 
>   let converted = [];
>   for (let val of arr) {
>     converted.length++;
>     converted[converted.length-1] = convert(val);
>   }
> 
> and "convert" can have side-effects.  So consider the original testcase in
> this bug, but with "array" defined like so:
> 
>   var iter = [][Symbol.iterator]();
>   var iterProto = Object.getPrototypeOf(iter);
>   var oldNext = iterProto.next;
> 
>   function hackedNext() {
>     var val = oldNext.call(this);
>     if ("value" in val) {
>       val.value++;
>     }
>     return val;
>   }
>   const obj = {
>     valueOf: function() {
>       iterProto.next = hackedNext;
>       return 2;
>     }
>   };
>   const array = [1, obj, 3]
> 
> and no custom iterators defined anywhere.  The output of ctx.getLineDash()
> should be [1,2,4], but I'm pretty sure it's [1,2,3] even with the WIP patch.

I agree with you. The code should check if it's Int32/Double. What's the convert operation here? Just toNumber?

[Sam Weinig]

(In reply to Saam Barati from comment #18)
> (In reply to Boris Zbarsky from comment #16)
> > It's not clear to me that the JSDOMConvertSequences uses of even the new
> > isArrayIteratorProtocolFastAndNonObservable are sound.  The premise of that
> > function is that it verifies that doing a spread on the array is safe at the
> > point when the function is called, after which spread can do a fast path.
> > 
> > But sequence conversions don't do a spread.  They do the equivalent of:
> > 
> >   let converted = [];
> >   for (let val of arr) {
> >     converted.length++;
> >     converted[converted.length-1] = convert(val);
> >   }
> > 
> > and "convert" can have side-effects.  So consider the original testcase in
> > this bug, but with "array" defined like so:
> > 
> >   var iter = [][Symbol.iterator]();
> >   var iterProto = Object.getPrototypeOf(iter);
> >   var oldNext = iterProto.next;
> > 
> >   function hackedNext() {
> >     var val = oldNext.call(this);
> >     if ("value" in val) {
> >       val.value++;
> >     }
> >     return val;
> >   }
> >   const obj = {
> >     valueOf: function() {
> >       iterProto.next = hackedNext;
> >       return 2;
> >     }
> >   };
> >   const array = [1, obj, 3]
> > 
> > and no custom iterators defined anywhere.  The output of ctx.getLineDash()
> > should be [1,2,4], but I'm pretty sure it's [1,2,3] even with the WIP patch.
> 
> I agree with you. The code should check if it's Int32/Double. What's the
> convert operation here? Just toNumber?

Yeah, the specializations we support are just numeric. We can simplify the non-specialized SequenceConverter to call the GenericSequenceConverter only, and remove the ContiguousShape and getDirectIndex cases of NumericSequenceConverter.

[Sam Weinig]

> > I agree with you. The code should check if it's Int32/Double. What's the
> > convert operation here? Just toNumber?
> 
> Yeah, the specializations we support are just numeric. We can simplify the
> non-specialized SequenceConverter to call the GenericSequenceConverter only,
> and remove the ContiguousShape and getDirectIndex cases of
> NumericSequenceConverter.

Well, before throwing away all non-numeric optimizations, I am curious what current use of sequence<> we have that is optimizable.  

The subtypes that have converters that can call out to JS are:
- string (DOMString, ByteString, USVString) and enums: toString()
- numeric types: valueOf()
- record (probably just because of OwnPropertyKeys, but also if it is parametrized on a type that needs conversion)
- sequences and unions if any further subtype is in this list. 
(I may be leaving something out).

Subtypes that don't are:
- any
- boolean
- object
- interface
- callback

Of those we currently have:
- sequence<object> (for postMessage)
- sequence<interface> (in the form of sequence<PerformanceEntry>, among others)
- sequence<boolean> (in MediaTrackCapabilities.echoCancellation)

The vast majority seem to be sequence<DOMString>, which would be nice to optimize at some point, but not all that necessary.

[Saam Barati]

(In reply to Sam Weinig from comment #20)
> > > I agree with you. The code should check if it's Int32/Double. What's the
> > > convert operation here? Just toNumber?
> > 
> > Yeah, the specializations we support are just numeric. We can simplify the
> > non-specialized SequenceConverter to call the GenericSequenceConverter only,
> > and remove the ContiguousShape and getDirectIndex cases of
> > NumericSequenceConverter.
> 
> Well, before throwing away all non-numeric optimizations, I am curious what
> current use of sequence<> we have that is optimizable.  
> 
> The subtypes that have converters that can call out to JS are:
> - string (DOMString, ByteString, USVString) and enums: toString()
> - numeric types: valueOf()
> - record (probably just because of OwnPropertyKeys, but also if it is
> parametrized on a type that needs conversion)
> - sequences and unions if any further subtype is in this list. 
> (I may be leaving something out).
> 
> Subtypes that don't are:
> - any
> - boolean
> - object
> - interface
> - callback
> 
> Of those we currently have:
> - sequence<object> (for postMessage)
> - sequence<interface> (in the form of sequence<PerformanceEntry>, among
> others)
> - sequence<boolean> (in MediaTrackCapabilities.echoCancellation)
> 
> The vast majority seem to be sequence<DOMString>, which would be nice to
> optimize at some point, but not all that necessary.

Sam, are you ok with me making the code correct w.r.t side effects chaining the iterator protocol, and then opening a bug to do some optimizations in a separate pass?

[Saam Barati]

Sam, I'm a bit confused reading some of the code, for example:

This code is inside:
```
template<typename IDLType>
struct NumericSequenceConverter {
...
...
        if (indexingType == JSC::Int32Shape) {
            for (unsigned i = 0; i < length; i++) {
                auto indexValue = array->butterfly()->contiguousInt32()[i].get();
                ASSERT(!indexValue || indexValue.isInt32());
                if (!indexValue)
                    result.uncheckedAppend(0);
                else
                    result.uncheckedAppend(indexValue.asInt32());
            }
            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;
        }
```

The code checks for an exception after ::convert. How would the ::convert cause an exception?

[Saam Barati]

Created attachment 307948 [details]
WIP

Still need to change the sequence converter code a bit.

[Build Bot]

Comment on attachment 307948 [details]
WIP

Attachment 307948 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3592227

New failing tests:
js/sequence-iterator-protocol.html

[Build Bot]

Created attachment 307953 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307948 [details]
WIP

Attachment 307948 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3592237

New failing tests:
js/sequence-iterator-protocol.html

[Build Bot]

Created attachment 307954 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307948 [details]
WIP

Attachment 307948 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3592241

New failing tests:
js/sequence-iterator-protocol.html

[Build Bot]

Created attachment 307955 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307948 [details]
WIP

Attachment 307948 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3592255

New failing tests:
js/sequence-iterator-protocol.html

[Build Bot]

Created attachment 307956 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

*** Bug 171158 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 308021 [details]
WIP

Sam, I did a first pass to not making us always go down the slow path. Let me know what you think. The thing I do makes a decision at compile time if conversion can have a side effect. Let me know what you think.

However, if we are often converting to strings, we can come up with optimizations that optimistically assume that conversion does not have side effects, and then if we see an object, bail out. I think this should be a separate patch.

[Saam Barati]

Boris, let me know if you think there is anything that is unsound w.r.t ECMA spec in this patch.

[Build Bot]

Comment on attachment 308021 [details]
WIP

Attachment 308021 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3597740

New failing tests:
fast/canvas/webgl/uniform-array-length-overflow.html

[Build Bot]

Created attachment 308030 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308021 [details]
WIP

Attachment 308021 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3597790

New failing tests:
fast/canvas/webgl/uniform-array-length-overflow.html

[Build Bot]

Created attachment 308032 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308021 [details]
WIP

Attachment 308021 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3597768

New failing tests:
fast/canvas/webgl/uniform-array-length-overflow.html

[Build Bot]

Created attachment 308033 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

(In reply to Saam Barati from comment #33)
> Created attachment 308021 [details]
> WIP
> 
> Sam, I did a first pass to not making us always go down the slow path. Let
> me know what you think. The thing I do makes a decision at compile time if
> conversion can have a side effect. Let me know what you think.
> 

Sounds great.

[Saam Barati]

Created attachment 308124 [details]
patch

[Sam Weinig]

Comment on attachment 308124 [details]
patch

r=me

[Build Bot]

Comment on attachment 308124 [details]
patch

Attachment 308124 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3603438

New failing tests:
fast/canvas/webgl/uniform-array-length-overflow.html

[Build Bot]

Created attachment 308131 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308124 [details]
patch

Attachment 308124 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3603508

New failing tests:
fast/canvas/webgl/uniform-array-length-overflow.html

[Build Bot]

Created attachment 308133 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Saam Barati]

Comment on attachment 308124 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308124&action=review

> Source/WebCore/bindings/js/JSDOMConvertSequences.h:88
>          JSC::JSArray* array = JSC::asArray(object);
> -        if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
> +        if (!array->isIteratorProtocolFastAndNonObservable())
>              return GenericConverter::convert(state, object);
>  
> -        unsigned length = array->length();
> +        JSC::IndexingType indexingType = array->indexingType() & JSC::IndexingShapeMask;
> +        if (indexingType != JSC::Int32Shape && indexingType != JSC::DoubleShape)
> +            return GenericConverter::convert(state, object);
>  
> +        unsigned length = array->length();

This code slightly breaks the fast/canvas/webgl/uniform-array-length-overflow.html by making it run a really long time.
The reason is we call into the above GenericConverter before trying to reserve capacity.

Maybe it's worth trying to reserve the capacity up front.

Maybe we should assume that regardless of the observability of the protocol, we always try to reserve up to array->length() capacity.
If that fails, we OOM. This would break a program like:
- huge array length 
- override iterator protocol to not do anything, and terminate immediately.

Anybody have thoughts?

[Saam Barati]

Alternatively, there is an argument to be made that this is just a slow test, and we have the correct behavior.

[Saam Barati]

Created attachment 308152 [details]
patch for landing

[Saam Barati]

(In reply to Saam Barati from comment #48)
> Comment on attachment 308124 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308124&action=review
> 
> > Source/WebCore/bindings/js/JSDOMConvertSequences.h:88
> >          JSC::JSArray* array = JSC::asArray(object);
> > -        if (!array->globalObject()->isArrayIteratorProtocolFastAndNonObservable())
> > +        if (!array->isIteratorProtocolFastAndNonObservable())
> >              return GenericConverter::convert(state, object);
> >  
> > -        unsigned length = array->length();
> > +        JSC::IndexingType indexingType = array->indexingType() & JSC::IndexingShapeMask;
> > +        if (indexingType != JSC::Int32Shape && indexingType != JSC::DoubleShape)
> > +            return GenericConverter::convert(state, object);
> >  
> > +        unsigned length = array->length();
> 
> This code slightly breaks the
> fast/canvas/webgl/uniform-array-length-overflow.html by making it run a
> really long time.
> The reason is we call into the above GenericConverter before trying to
> reserve capacity.
> 
> Maybe it's worth trying to reserve the capacity up front.
> 
> Maybe we should assume that regardless of the observability of the protocol,
> we always try to reserve up to array->length() capacity.
> If that fails, we OOM. This would break a program like:
> - huge array length 
> - override iterator protocol to not do anything, and terminate immediately.
> 
> Anybody have thoughts?

I decided to go with this proposal of just reserving array->length() capacity up front, even if we could potentially call valueOf while performing the iterator protocol. We can change this in the future, but the array->length value is a very good proxy for the capacity we'll need in such situations. It's only not a good proxy when somebody has a valueOf that changes the iterator protocol. It's unlikely any sane program will do this.

[Saam Barati]

Created attachment 308153 [details]
patch for landing

[Saam Barati]

Sam, do you know what the non default ExceptionThrower for the various converters can do? Are they there just to throw different types of errors, e.g, TypeError vs RangeError. Or do some exception throwers call arbitrary JS code?

[WebKit Commit Bot]

Comment on attachment 308153 [details]
patch for landing

Clearing flags on attachment: 308153

Committed r215777: <http://trac.webkit.org/changeset/215777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.