Bug 160801 - Conversion to sequence<T> is broken for iterable objects
Summary: Conversion to sequence<T> is broken for iterable objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 160800
Blocks: 160797
  Show dependency treegraph
 
Reported: 2016-08-12 00:18 PDT by Ryosuke Niwa
Modified: 2016-08-16 08:57 PDT (History)
14 users (show)

See Also:


Attachments
Fixes the bug (64.99 KB, patch)
2016-08-15 14:42 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (772.77 KB, application/zip)
2016-08-15 15:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (832.31 KB, application/zip)
2016-08-15 15:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.41 MB, application/zip)
2016-08-15 15:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (648.19 KB, application/zip)
2016-08-15 17:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (837.70 KB, application/zip)
2016-08-15 18:25 PDT, Build Bot
no flags Details
Fixed tests (74.85 KB, patch)
2016-08-15 19:07 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (80.10 KB, patch)
2016-08-15 21:35 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (79.06 KB, patch)
2016-08-15 23:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-08-12 00:18:09 PDT
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.
Comment 1 Ryosuke Niwa 2016-08-15 14:42:56 PDT
Created attachment 286093 [details]
Fixes the bug
Comment 2 Chris Dumez 2016-08-15 15:21:21 PDT
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 ]
Comment 3 Build Bot 2016-08-15 15:36:43 PDT
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
Comment 4 Build Bot 2016-08-15 15:36:48 PDT
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
Comment 5 Build Bot 2016-08-15 15:39:43 PDT
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
Comment 6 Build Bot 2016-08-15 15:39:47 PDT
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
Comment 7 Build Bot 2016-08-15 15:49:08 PDT
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
Comment 8 Build Bot 2016-08-15 15:49:12 PDT
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
Comment 9 Build Bot 2016-08-15 17:07:02 PDT
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
Comment 10 Build Bot 2016-08-15 17:07:06 PDT
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
Comment 11 Build Bot 2016-08-15 18:25:30 PDT
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
Comment 12 Build Bot 2016-08-15 18:25:34 PDT
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
Comment 13 Ryosuke Niwa 2016-08-15 19:07:39 PDT
Created attachment 286135 [details]
Fixed tests
Comment 14 Sam Weinig 2016-08-15 19:27:58 PDT
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?
Comment 15 Sam Weinig 2016-08-15 19:29:11 PDT
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?
Comment 16 Chris Dumez 2016-08-15 19:32:47 PDT
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.
Comment 17 Ryosuke Niwa 2016-08-15 19:35:36 PDT
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.
Comment 18 Chris Dumez 2016-08-15 19:52:29 PDT
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
Comment 19 Ryosuke Niwa 2016-08-15 20:06:32 PDT
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.
Comment 20 Chris Dumez 2016-08-15 20:11:32 PDT
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().
Comment 21 Chris Dumez 2016-08-15 20:12:58 PDT
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
Comment 22 Ryosuke Niwa 2016-08-15 21:35:55 PDT
Created attachment 286148 [details]
Patch
Comment 23 Darin Adler 2016-08-15 22:25:32 PDT
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?
Comment 24 Ryosuke Niwa 2016-08-15 22:48:07 PDT
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.
Comment 25 Ryosuke Niwa 2016-08-15 23:21:34 PDT
Created attachment 286154 [details]
Patch for landing
Comment 26 WebKit Commit Bot 2016-08-15 23:52:33 PDT
Comment on attachment 286154 [details]
Patch for landing

Clearing flags on attachment: 286154

Committed r204500: <http://trac.webkit.org/changeset/204500>
Comment 27 WebKit Commit Bot 2016-08-15 23:52:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Csaba Osztrogonác 2016-08-16 03:30:31 PDT
(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.
Comment 29 Darin Adler 2016-08-16 08:57:01 PDT
(In reply to comment #28)
> It broke the bindings generation tests, see build.webkit.org for details.

Fixed in r204503