Bug 171150 - JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong because it does not do the necessary checks on the base object
Summary: JSArray::isArrayPrototypeIteratorProtocolFastAndNonObservable is wrong becaus...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 171158 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-04-21 16:31 PDT by Sam Weinig
Modified: 2017-04-25 17:52 PDT (History)
18 users (show)

See Also:


Attachments
Test Case (see console for results) (827 bytes, text/html)
2017-04-21 16:31 PDT, Sam Weinig
no flags Details
Test Case (984 bytes, text/html)
2017-04-21 16:46 PDT, Sam Weinig
no flags Details
WIP (1.59 KB, patch)
2017-04-21 22:25 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (12.51 KB, patch)
2017-04-22 00:13 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
WIP (14.46 KB, patch)
2017-04-23 19:37 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (946.16 KB, application/zip)
2017-04-23 20:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (864.71 KB, application/zip)
2017-04-23 21:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.78 MB, application/zip)
2017-04-23 21:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (742.35 KB, application/zip)
2017-04-23 21:25 PDT, Build Bot
no flags Details
WIP (24.12 KB, patch)
2017-04-24 16:31 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1021.51 KB, application/zip)
2017-04-24 17:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.14 MB, application/zip)
2017-04-24 18:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.69 MB, application/zip)
2017-04-24 18:10 PDT, Build Bot
no flags Details
patch (30.68 KB, patch)
2017-04-25 11:58 PDT, Saam Barati
sam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.10 MB, application/zip)
2017-04-25 13:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (976.87 KB, application/zip)
2017-04-25 13:26 PDT, Build Bot
no flags Details
patch for landing (31.64 KB, patch)
2017-04-25 15:24 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (31.64 KB, patch)
2017-04-25 15:31 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-04-21 16:31:54 PDT
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).
Comment 1 Sam Weinig 2017-04-21 16:46:26 PDT
Created attachment 307838 [details]
Test Case

Updated test.
Comment 2 Sam Weinig 2017-04-21 16:48:14 PDT
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?
Comment 3 Saam Barati 2017-04-21 20:13:17 PDT
(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).
Comment 4 Saam Barati 2017-04-21 20:14:19 PDT
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.
Comment 5 Boris Zbarsky 2017-04-21 21:25:39 PDT
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.
Comment 6 Saam Barati 2017-04-21 21:41:32 PDT
(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.
Comment 7 Radar WebKit Bug Importer 2017-04-21 22:02:13 PDT
<rdar://problem/31771880>
Comment 8 Saam Barati 2017-04-21 22:09:30 PDT
(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.
Comment 9 Saam Barati 2017-04-21 22:25:47 PDT
Created attachment 307886 [details]
WIP

I suspect we want something like this for JSArray::isArrayIteratorProtocolFastAndNonObservable
Comment 10 Saam Barati 2017-04-21 22:26:40 PDT
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 !=
Comment 11 Saam Barati 2017-04-22 00:13:26 PDT
Created attachment 307898 [details]
WIP

Might be the entirety of the fix.
Comment 12 Saam Barati 2017-04-22 00:21:31 PDT
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.
Comment 13 Build Bot 2017-04-22 00:56:37 PDT
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
Comment 14 Sam Weinig 2017-04-22 07:09:57 PDT
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.
Comment 15 Sam Weinig 2017-04-22 07:18:27 PDT
(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 :).
Comment 16 Boris Zbarsky 2017-04-23 11:10:56 PDT
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.
Comment 17 Yusuke Suzuki 2017-04-23 11:19:54 PDT
(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.
Comment 18 Saam Barati 2017-04-23 13:16:11 PDT
(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?
Comment 19 Sam Weinig 2017-04-23 15:44:53 PDT
(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.
Comment 20 Sam Weinig 2017-04-23 17:52:58 PDT
> > 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.
Comment 21 Saam Barati 2017-04-23 19:12:34 PDT
(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?
Comment 22 Saam Barati 2017-04-23 19:20:54 PDT
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?
Comment 23 Saam Barati 2017-04-23 19:37:27 PDT
Created attachment 307948 [details]
WIP

Still need to change the sequence converter code a bit.
Comment 24 Build Bot 2017-04-23 20:57:30 PDT
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
Comment 25 Build Bot 2017-04-23 20:57:32 PDT
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
Comment 26 Build Bot 2017-04-23 21:03:39 PDT
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
Comment 27 Build Bot 2017-04-23 21:03:40 PDT
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
Comment 28 Build Bot 2017-04-23 21:14:22 PDT
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
Comment 29 Build Bot 2017-04-23 21:14:24 PDT
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
Comment 30 Build Bot 2017-04-23 21:25:03 PDT
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
Comment 31 Build Bot 2017-04-23 21:25:04 PDT
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
Comment 32 Saam Barati 2017-04-24 12:35:00 PDT
*** Bug 171158 has been marked as a duplicate of this bug. ***
Comment 33 Saam Barati 2017-04-24 16:31:37 PDT
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.
Comment 34 Saam Barati 2017-04-24 16:36:11 PDT
Boris, let me know if you think there is anything that is unsound w.r.t ECMA spec in this patch.
Comment 35 Build Bot 2017-04-24 17:56:41 PDT
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
Comment 36 Build Bot 2017-04-24 17:56:43 PDT
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
Comment 37 Build Bot 2017-04-24 18:06:01 PDT
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
Comment 38 Build Bot 2017-04-24 18:06:03 PDT
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
Comment 39 Build Bot 2017-04-24 18:10:47 PDT
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
Comment 40 Build Bot 2017-04-24 18:10:50 PDT
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
Comment 41 Sam Weinig 2017-04-25 09:23:41 PDT
(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.
Comment 42 Saam Barati 2017-04-25 11:58:17 PDT
Created attachment 308124 [details]
patch
Comment 43 Sam Weinig 2017-04-25 12:01:04 PDT
Comment on attachment 308124 [details]
patch

r=me
Comment 44 Build Bot 2017-04-25 13:16:48 PDT
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
Comment 45 Build Bot 2017-04-25 13:16:50 PDT
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
Comment 46 Build Bot 2017-04-25 13:26:42 PDT
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
Comment 47 Build Bot 2017-04-25 13:26:44 PDT
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
Comment 48 Saam Barati 2017-04-25 13:27:17 PDT
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?
Comment 49 Saam Barati 2017-04-25 13:30:14 PDT
Alternatively, there is an argument to be made that this is just a slow test, and we have the correct behavior.
Comment 50 Saam Barati 2017-04-25 15:24:01 PDT
Created attachment 308152 [details]
patch for landing
Comment 51 Saam Barati 2017-04-25 15:26:06 PDT
(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.
Comment 52 Saam Barati 2017-04-25 15:31:27 PDT
Created attachment 308153 [details]
patch for landing
Comment 53 Saam Barati 2017-04-25 15:33:39 PDT
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?
Comment 54 WebKit Commit Bot 2017-04-25 17:52:38 PDT
Comment on attachment 308153 [details]
patch for landing

Clearing flags on attachment: 308153

Committed r215777: <http://trac.webkit.org/changeset/215777>
Comment 55 WebKit Commit Bot 2017-04-25 17:52:41 PDT
All reviewed patches have been landed.  Closing bug.