Bug 160801 – Conversion to sequence<T> is broken for iterable objects

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

[Ryosuke Niwa]

We need to implement http://heycam.github.io/webidl/#es-sequence

An ECMAScript value V is converted to an IDL sequence<T> value as follows:

1. If V is not an object, throw a TypeError.
2. If V is a native RegEx object, throw a TypeError.
3. Let method be the result of GetMethod(V, @@iterator).
4. ReturnIfAbrupt(method).
5. If method is undefined, throw a TypeError.
7. Return the result of creating a sequence of type sequence<T> from V and method.

Creating a sequence from an iterable: http://heycam.github.io/webidl/#create-sequence-from-iterable

1. Let tier be GetIterator(iterable, method).
2. ReturnIfAbrupt(iter).
3. Initialize i to be 0.
4. Repeat
    1. Let next be IteratorStep(tier).
    2. ReturnIfAbrupt(next).
    3. If next is false, then return an IDL sequence value of type sequence<T> of length i, where the value of the element at index j is Sj.
    4. Let nextItem be IteratorValue(next).
    5. ReturnIfAbrupt(nextItem).
    6. Initialize Si to the result of converting nextItem to an IDL value of type T.
    7. Set i to i + 1.

[Ryosuke Niwa]

Created attachment 286093 [details]
Fixes the bug

[Chris Dumez]

Looks like there are a few failures on the bots:
Regressions: Unexpected text-only failures (5)
  http/tests/dom/window-open-about-webkit-org-and-access-document.html [ Failure ]
  http/tests/security/xssAuditor/block-does-not-leak-location.html [ Failure ]
  http/tests/security/xssAuditor/block-does-not-leak-referrer.html [ Failure ]
  imported/w3c/IndexedDB-private-browsing/keypath_invalid.html [ Failure ]
  imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm [ Failure ]

[Build Bot]

Comment on attachment 286093 [details]
Fixes the bug

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

New failing tests:
http/tests/dom/window-open-about-webkit-org-and-access-document.html
imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
imported/w3c/IndexedDB-private-browsing/keypath_invalid.html
http/tests/security/xssAuditor/block-does-not-leak-location.html

[Build Bot]

Created attachment 286099 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 286093 [details]
Fixes the bug

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

New failing tests:
http/tests/dom/window-open-about-webkit-org-and-access-document.html
imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
imported/w3c/IndexedDB-private-browsing/keypath_invalid.html
http/tests/security/xssAuditor/block-does-not-leak-location.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 286093 [details]
Fixes the bug

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

New failing tests:
http/tests/dom/window-open-about-webkit-org-and-access-document.html
imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
imported/w3c/IndexedDB-private-browsing/keypath_invalid.html
http/tests/security/xssAuditor/block-does-not-leak-location.html

[Build Bot]

Created attachment 286105 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Build Bot]

Comment on attachment 286093 [details]
Fixes the bug

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

New failing tests:
http/tests/dom/window-open-about-webkit-org-and-access-document.html
imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm
http/tests/security/xssAuditor/block-does-not-leak-location.html
imported/w3c/IndexedDB-private-browsing/keypath_invalid.html
http/tests/security/xssAuditor/block-does-not-leak-referrer.html

[Build Bot]

Created attachment 286119 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

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

[Build Bot]

Comment on attachment 286093 [details]
Fixes the bug

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

New failing tests:
http/tests/dom/window-open-about-webkit-org-and-access-document.html
imported/w3c/web-platform-tests/IndexedDB/keypath_invalid.htm
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
imported/w3c/IndexedDB-private-browsing/keypath_invalid.html
http/tests/security/xssAuditor/block-does-not-leak-location.html

[Build Bot]

Created attachment 286131 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

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

[Ryosuke Niwa]

Created attachment 286135 [details]
Fixed tests

[Sam Weinig]

Comment on attachment 286135 [details]
Fixed tests

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

> Source/WebCore/bindings/js/JSDOMBinding.h:727
> +        ASSERT(!state->hadException());

Why can this be asserted here?

[Sam Weinig]

Comment on attachment 286135 [details]
Fixed tests

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:727
>> +        ASSERT(!state->hadException());
> 
> Why can this be asserted here?

Or rather, why is it ok for NativeValueTraits<T>::nativeValue to swallow the exception?

[Chris Dumez]

Comment on attachment 286135 [details]
Fixed tests

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

>>> Source/WebCore/bindings/js/JSDOMBinding.h:727
>>> +        ASSERT(!state->hadException());
>> 
>> Why can this be asserted here?
> 
> Or rather, why is it ok for NativeValueTraits<T>::nativeValue to swallow the exception?

I believe the idea is that NativeValueTraits<T>::nativeValue() should return false if an exception is thrown. Therefore we would have returned early on the previous line.

[Ryosuke Niwa]

Comment on attachment 286135 [details]
Fixed tests

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

>>>> Source/WebCore/bindings/js/JSDOMBinding.h:727
>>>> +        ASSERT(!state->hadException());
>>> 
>>> Why can this be asserted here?
>> 
>> Or rather, why is it ok for NativeValueTraits<T>::nativeValue to swallow the exception?
> 
> I believe the idea is that NativeValueTraits<T>::nativeValue() should return false if an exception is thrown. Therefore we would have returned early on the previous line.

Yeah, that's the idea.  All of this is a bit convoluted.
I think it's better for the caller to be checking this but I didn't want to turn this into a mega patch with all sorts of code changes like that.

[Chris Dumez]

Comment on attachment 286135 [details]
Fixed tests

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

Looks good overall. Just a couple of comments / questions.

> Source/WebCore/bindings/js/JSDOMBinding.h:-677
> -        if (!jsValue.isNumber())

This bug fix should probably be mentioned in the ChangeLog.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1821
> +        push(@implContent, "        if (distinguishingArg.isObject()) { // FIXME: Avoid invoking Get(object, Symbol.iterator) again in toNativeArray and toRefPtrNativeArray.\n");

Can we move the FIXME comment on the next line?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1822
> +        push(@implContent, "            JSValue iteratorFunction = distinguishingArg.get(state, state->propertyNames().iteratorSymbol);\n");

Would JSObject::hasProperty() work? We don't need the return value at the moment.

> LayoutTests/fast/dom/MutationObserver/observe-exceptions.html:32
> +    shouldThrow('observer.observe(document.body, {attributeFilter: 1})');

I introduced shouldThrowErrorName() so that it also checks which exception is thrown. Here:
shouldThrowErrorName('observer.observe(document.body, {attributeFilter: 1})', 'TypeError');

> LayoutTests/fast/text/font-face-set-javascript.html:19
> +shouldThrow("new FontFaceSet(1)");

shouldThrowErrorName

[Ryosuke Niwa]

Comment on attachment 286135 [details]
Fixed tests

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:-677
>> -        if (!jsValue.isNumber())
> 
> This bug fix should probably be mentioned in the ChangeLog.

Oh yeah, I forgot about this. Will do.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1821
>> +        push(@implContent, "        if (distinguishingArg.isObject()) { // FIXME: Avoid invoking Get(object, Symbol.iterator) again in toNativeArray and toRefPtrNativeArray.\n");
> 
> Can we move the FIXME comment on the next line?

Sure.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1822
>> +        push(@implContent, "            JSValue iteratorFunction = distinguishingArg.get(state, state->propertyNames().iteratorSymbol);\n");
> 
> Would JSObject::hasProperty() work? We don't need the return value at the moment.

We're using the return value to check if it's undefined or not.
In fact, you can define {[Symbol.iterator]: undefined} and that needs to be treated as if the property didn't exist.

>> LayoutTests/fast/dom/MutationObserver/observe-exceptions.html:32
>> +    shouldThrow('observer.observe(document.body, {attributeFilter: 1})');
> 
> I introduced shouldThrowErrorName() so that it also checks which exception is thrown. Here:
> shouldThrowErrorName('observer.observe(document.body, {attributeFilter: 1})', 'TypeError');

Will fix.

>> LayoutTests/fast/text/font-face-set-javascript.html:19
>> +shouldThrow("new FontFaceSet(1)");
> 
> shouldThrowErrorName

Will fix.

[Chris Dumez]

Comment on attachment 286135 [details]
Fixed tests

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

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1822
>>> +        push(@implContent, "            JSValue iteratorFunction = distinguishingArg.get(state, state->propertyNames().iteratorSymbol);\n");
>> 
>> Would JSObject::hasProperty() work? We don't need the return value at the moment.
> 
> We're using the return value to check if it's undefined or not.
> In fact, you can define {[Symbol.iterator]: undefined} and that needs to be treated as if the property didn't exist.

I looked at the Web IDL spec again and based on that it looks like we should use JSObject::getMethod()? It does some additional checks compared to get().

[Chris Dumez]

Comment on attachment 286135 [details]
Fixed tests

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

>>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1822
>>>> +        push(@implContent, "            JSValue iteratorFunction = distinguishingArg.get(state, state->propertyNames().iteratorSymbol);\n");
>>> 
>>> Would JSObject::hasProperty() work? We don't need the return value at the moment.
>> 
>> We're using the return value to check if it's undefined or not.
>> In fact, you can define {[Symbol.iterator]: undefined} and that needs to be treated as if the property didn't exist.
> 
> I looked at the Web IDL spec again and based on that it looks like we should use JSObject::getMethod()? It does some additional checks compared to get().

In particular, getMethod() will also return undefined if the property is null or not callable. See http://www.ecma-international.org/ecma-262/6.0/#sec-getmethod

[Ryosuke Niwa]

Created attachment 286148 [details]
Patch

[Darin Adler]

Comment on attachment 286148 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:678
> -        if (!jsValue.isNumber())
> -            return false;
> -
>          indexedValue = jsValue.toUInt32(&exec);
> -        if (exec.hadException())
> -            return false;
> -
> -        return true;
> +        return !exec.hadException();

The change log implies that this is just an optimization. But this changes behavior in a major way. Removing the isNumber check means that toUInt32 will now call toNumber, which will convert booleans, undefined, and null to 1 and 0, and will invoke functions like JSString::toNumber, and JSObject::toNumber. Is this change in behavior intentional? Do we have test coverage for it?

> Source/WebCore/bindings/js/JSDOMBinding.h:703
> +    if (!value.isObject()) {
> +        throwSequenceTypeError(exec);
>          return Vector<RefPtr<T>>();
> +    }

Annoying that we have to do this check. Seems to me that forEachInIterable already will throw an exception in this case, but I suppose maybe the wrong exception?

> Source/WebCore/bindings/js/JSDOMBinding.h:719
> +    if (!value.isObject()) {
> +        throwSequenceTypeError(exec);
>          return Vector<T>();

Ditto.

> Source/WebCore/bindings/js/JSDOMBinding.h:727
> +        ASSERT(!state->hadException());

Why can we assert this? Can’t nativeValue functions throw an exception? After the change above, the one for unsigned certainly can, but maybe that’s a mistake and the only one?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1832
> +        if (distinguishingArg.isObject()) {
> +            // FIXME: Avoid invoking GetMethod(object, Symbol.iterator) again in toNativeArray and toRefPtrNativeArray.
> +            JSObject* object = JSC::asObject(distinguishingArg);
> +            CallData callData;
> +            CallType callType;
> +            JSValue applyMethod = object->getMethod(state, callData, callType, state->propertyNames().iteratorSymbol, ASCIILiteral("Symbol.iterator property should be callable"));
> +            if (state->hadException())
> +                return JSValue::encode(jsUndefined());
> +            if (!applyMethod.isUndefined())
> +                return ${functionName}$overload->{overloadIndex}(state);
> +        }

Can we put all of this into a helper function? I don’t like having so much of this written out in the bindings. Normally I would want to use a function for everything except for that very last line where we call the function. Maybe the code above this already follows this pattern, but I don’t like it.

I also don’t understand why it’s correct that this function checks applyMethod to see if it’s defined, but doesn’t ever use the value.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:5710
> +            JSValue applyMethod = handler->getMethod(state, callData, callType, makeIdentifier(vm, "apply"), ASCIILiteral("Symbol.iterator property should be callable"));
> +            if (state->hadException())
> +                return JSValue::encode(jsUndefined());
> +            if (!iteratorFunction.isUndefined())
> +                return jsTestObjPrototypeFunctionOverloadedMethod7(state);

This does not look like it will compile. It’s setting a variable named applyMethod and then checking a variable named iteratorFunction. Do we need to regenerate this?

[Ryosuke Niwa]

Comment on attachment 286148 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:678
>> +        return !exec.hadException();
> 
> The change log implies that this is just an optimization. But this changes behavior in a major way. Removing the isNumber check means that toUInt32 will now call toNumber, which will convert booleans, undefined, and null to 1 and 0, and will invoke functions like JSString::toNumber, and JSObject::toNumber. Is this change in behavior intentional? Do we have test coverage for it?

That’s a good point.  This was intentionally change to match the conversion to unsigned long: http://heycam.github.io/webidl/#es-unsigned-long
which calls toNumber first without checking whether it’s a number or not.
As far as I can tell, we don’t currently have any API that uses sequence<unsigned long> at the moment so this code is untested.

>> Source/WebCore/bindings/js/JSDOMBinding.h:703
>> +    }
> 
> Annoying that we have to do this check. Seems to me that forEachInIterable already will throw an exception in this case, but I suppose maybe the wrong exception?

I don’t think it does. `Get(value, Symbol.iterator)` doesn’t throw when value is a number or a string as far as I can tell.
While we throw a TypeError when Get’s result is un-callable, a string will gladly return an iterator.

>> Source/WebCore/bindings/js/JSDOMBinding.h:719
>>          return Vector<T>();
> 
> Ditto.

Ditto.

>> Source/WebCore/bindings/js/JSDOMBinding.h:727
>> +        ASSERT(!state->hadException());
> 
> Why can we assert this? Can’t nativeValue functions throw an exception? After the change above, the one for unsigned certainly can, but maybe that’s a mistake and the only one?

The idea is that NativeValueTraits<T>::nativeValue always returns false when there is an exception.
This is a really convoluted code, as I noted earlier in the comment, but I didn’t refactor this code in this already giant patch.
Will try to clean up the mess in a separate patch since everyone seems to be getting confused about this code.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1832
>> +        }
> 
> Can we put all of this into a helper function? I don’t like having so much of this written out in the bindings. Normally I would want to use a function for everything except for that very last line where we call the function. Maybe the code above this already follows this pattern, but I don’t like it.
> 
> I also don’t understand why it’s correct that this function checks applyMethod to see if it’s defined, but doesn’t ever use the value.

Okay. I’m going to move all of it into a helper function called hasIteratorMethod for now.

[Ryosuke Niwa]

Created attachment 286154 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 286154 [details]
Patch for landing

Clearing flags on attachment: 286154

Committed r204500: <http://trac.webkit.org/changeset/204500>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #26)
> Comment on attachment 286154 [details]
> Patch for landing
> 
> Clearing flags on attachment: 286154
> 
> Committed r204500: <http://trac.webkit.org/changeset/204500>

It broke the bindings generation tests, see build.webkit.org for details.

[Darin Adler]

(In reply to comment #28)
> It broke the bindings generation tests, see build.webkit.org for details.

Fixed in r204503