Bug 159296 – DOM value iterable interfaces should use Array prototype methods

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Value iterators should be using Array prototype methods (values, entries, keys, Symbol.Iterator, forEach)

[youenn fablet]

Created attachment 282432 [details]
Patch

[youenn fablet]

Created attachment 282538 [details]
Rebasing and adding chrome test

[Chris Dumez]

Adding some JSC folks. The relevant spec seems to be at:
https://heycam.github.io/webidl/#es-iterable

[youenn fablet]

Ping review?

(In reply to comment #3)
> Adding some JSC folks. The relevant spec seems to be at:
> https://heycam.github.io/webidl/#es-iterable

Right, in particular the lines:
//Entries
If the interface has a value iterator, then the Function object is the initial value of the “entries” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
//Keys
If the interface has a value iterator, then the Function object is the initial value of the “keys” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
...

The only downside I see here is that this path is making use of the "current value" of the prototype not, the "initial" one.
A follow-up patch may fix this for instance by adding private properties to the array prototype that would be used to populate value-iterable interfaces.

[Mark Lam]

Comment on attachment 282538 [details]
Rebasing and adding chrome test

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

> Source/WebCore/ChangeLog:14
> +        For value iterators, copy the iterator methods from Array prototype.
> +        This change applies only to NodeList at the moment.
> +        Copy of Array prototype iterator methods is disabled if the interface has no indexed getter.
> +

Please quote the spec justification after this for easy future reference.  Specifically:

According to https://heycam.github.io/webidl/#es-iterable,
[re: Entries] If the interface has a value iterator, then the Function object is the initial value of the “entries” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
[re: Keys] If the interface has a value iterator, then the Function object is the initial value of the “keys” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).

> Source/WebCore/bindings/js/JSDOMIterator.cpp:38
> +static inline void copyProperty(JSC::ExecState& state, JSC::JSObject& from, JSC::JSObject& to, const JSC::Identifier& identifier, unsigned attributes = 0)
> +{
> +    JSC::JSValue value = from.get(&state, identifier);
> +    ASSERT(value);
> +    to.putDirect(state.vm(), identifier, value, attributes);
> +}

Since this function is only used as a convenience function inside addValueIterableMethods(), you can implement it as a lambda in addValueIterableMethods() like so:

    JSC::VM& vm = state->vm();
    auto copyProperty = [&] (const JSC::Identifier& identifier, unsigned attributes = 0) {
        JSC::JSValue = arrayPrototype->get(state, identifier);
        ASSERT(value);
        prototype.putDirect(vm, identifier, value, attributes);
    };

... and then call it like so:

    copyProperty(JSC::Identifier::fromString(state, "entries"));

Using a lambda here reduces the verbosity of the code as well as communicates that this function is bounded in scope to addValueIterableMethods().

> Source/WebCore/bindings/js/JSDOMIterator.cpp:52
> +    JSC::ExecState* state = globalObject.globalExec();
> +    ASSERT(state);
> +
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "entries"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "keys"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "values"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "forEach"));
> +    copyProperty(*state, *arrayPrototype, prototype, vm.propertyNames->iteratorSymbol, JSC::DontEnum);

In https://bugs.webkit.org/show_bug.cgi?id=159296#c4, you said:
The only downside I see here is that this path is making use of the "current value" of the prototype not, the "initial" one.
A follow-up patch may fix this for instance by adding private properties to the array prototype that would be used to populate value-iterable interfaces.

I think you can address that right now by doing the following:

1. In JSDOMGlobalObject, add WriteBarrier<JSFunction> fields for entries, keys, values , forEach, and iteratorSymbol.
2. In JSDOMGlobalObject::finishCreation(), copy those values from the ArrayPrototype.
3. In JSDOMGlobalObject::visitChildren(), visit those newly added fields.  See JSGlobalObject::visitChildren() for an example of how to do it.
4. In addValueIterableMethods(), copy the values from JSDOMGlobalObject's fields.

What do you think?

[youenn fablet]

Thanks for the review.

(In reply to comment #5)
> Comment on attachment 282538 [details]
> Rebasing and adding chrome test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282538&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        For value iterators, copy the iterator methods from Array prototype.
> > +        This change applies only to NodeList at the moment.
> > +        Copy of Array prototype iterator methods is disabled if the interface has no indexed getter.
> > +
> 
> Please quote the spec justification after this for easy future reference. 
> Specifically:
> 
> According to https://heycam.github.io/webidl/#es-iterable,
> [re: Entries] If the interface has a value iterator, then the Function
> object is the initial value of the “entries” data property of
> %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> [re: Keys] If the interface has a value iterator, then the Function object
> is the initial value of the “keys” data property of %ArrayPrototype%
> ([ECMA-262], section 6.1.7.4).

OK

> 
> > Source/WebCore/bindings/js/JSDOMIterator.cpp:38
> > +static inline void copyProperty(JSC::ExecState& state, JSC::JSObject& from, JSC::JSObject& to, const JSC::Identifier& identifier, unsigned attributes = 0)
> > +{
> > +    JSC::JSValue value = from.get(&state, identifier);
> > +    ASSERT(value);
> > +    to.putDirect(state.vm(), identifier, value, attributes);
> > +}
> 
> Since this function is only used as a convenience function inside
> addValueIterableMethods(), you can implement it as a lambda in
> addValueIterableMethods() like so:
> 
>     JSC::VM& vm = state->vm();
>     auto copyProperty = [&] (const JSC::Identifier& identifier, unsigned
> attributes = 0) {
>         JSC::JSValue = arrayPrototype->get(state, identifier);
>         ASSERT(value);
>         prototype.putDirect(vm, identifier, value, attributes);
>     };
> 
> ... and then call it like so:
> 
>     copyProperty(JSC::Identifier::fromString(state, "entries"));
> 
> Using a lambda here reduces the verbosity of the code as well as
> communicates that this function is bounded in scope to
> addValueIterableMethods().

OK.
I was wondering whether there was not already a routine like this somewhere.

> > Source/WebCore/bindings/js/JSDOMIterator.cpp:52
> > +    JSC::ExecState* state = globalObject.globalExec();
> > +    ASSERT(state);
> > +
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "entries"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "keys"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "values"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "forEach"));
> > +    copyProperty(*state, *arrayPrototype, prototype, vm.propertyNames->iteratorSymbol, JSC::DontEnum);
> 
> In https://bugs.webkit.org/show_bug.cgi?id=159296#c4, you said:
> The only downside I see here is that this path is making use of the "current
> value" of the prototype not, the "initial" one.
> A follow-up patch may fix this for instance by adding private properties to
> the array prototype that would be used to populate value-iterable interfaces.
> 
> I think you can address that right now by doing the following:
> 
> 1. In JSDOMGlobalObject, add WriteBarrier<JSFunction> fields for entries,
> keys, values , forEach, and iteratorSymbol.
> 2. In JSDOMGlobalObject::finishCreation(), copy those values from the
> ArrayPrototype.
> 3. In JSDOMGlobalObject::visitChildren(), visit those newly added fields. 
> See JSGlobalObject::visitChildren() for an example of how to do it.
> 4. In addValueIterableMethods(), copy the values from JSDOMGlobalObject's
> fields.
> 
> What do you think?

Sure, this can be done this way.
Having these functions as private slots has some benefits though:
- Less boiler plate code in JSDOMGlobalObject
- Access to these functions from JS built-in code
For instance, @Array.@forEach would have been/would be handy for FetchHeaders.js builtin code.

I can update the patch either way.

[Mark Lam]

(In reply to comment #6)
> Having these functions as private slots has some benefits though:
> - Less boiler plate code in JSDOMGlobalObject
> - Access to these functions from JS built-in code
> For instance, @Array.@forEach would have been/would be handy for
> FetchHeaders.js builtin code.

That sounds good too.

[youenn fablet]

Created attachment 283063 [details]
Beefing up log and using private slots approach

[youenn fablet]

(In reply to comment #7)
> (In reply to comment #6)
> > Having these functions as private slots has some benefits though:
> > - Less boiler plate code in JSDOMGlobalObject
> > - Access to these functions from JS built-in code
> > For instance, @Array.@forEach would have been/would be handy for
> > FetchHeaders.js builtin code.
> 
> That sounds good too.

Proposed patch follows that strategy.
I also beefed up a bit the test to check that.

[WebKit Commit Bot]

Attachment 283063 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

> For instance, @Array.@forEach would have been/would be handy for
> FetchHeaders.js builtin code.

See bug 159528 for a potential use of @forEach.

[Mark Lam]

Comment on attachment 283063 [details]
Beefing up log and using private slots approach

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:124
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().entriesPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().entriesPublicName()), ReadOnly);
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().forEachPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().forEachPublicName()), ReadOnly);
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().keysPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().keysPublicName()), ReadOnly);

Since we know that we putDirectWithoutTransition() the original values above, can you just getDirect the values instead of going thru get() which does a lot more work?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:162
> +void ArrayPrototype::copyIterableMethods(ExecState& state, JSObject& prototype)
> +{
> +    VM& vm = state.vm();
> +    auto copyProperty = [&] (const Identifier& arrayIdentifier, const Identifier& otherIdentifier, unsigned attributes = 0) {
> +        JSValue value = get(&state, arrayIdentifier);
> +        ASSERT(value);
> +        prototype.putDirect(vm, otherIdentifier, value, attributes);
> +    };
> +
> +    copyProperty(vm.propertyNames->builtinNames().entriesPrivateName(), vm.propertyNames->builtinNames().entriesPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().forEachPrivateName(), vm.propertyNames->builtinNames().forEachPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().keysPrivateName(), vm.propertyNames->builtinNames().keysPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().valuesPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), DontEnum);
> +}

Is there a reason for copyIterableMethods() in ArrayPrototype instead of inlining it in JSDOMIterator's addValueIterableMethods()?  It seems to me that the properties to be copied is defined by JSDOMIterator and not by ArrayPrototype.  Hence, I think belongs in JSDOMIterator than here.

[youenn fablet]

(In reply to comment #12)
> Comment on attachment 283063 [details]
> Beefing up log and using private slots approach
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283063&action=review
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:124
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().entriesPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().entriesPublicName()), ReadOnly);
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().forEachPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().forEachPublicName()), ReadOnly);
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().keysPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().keysPublicName()), ReadOnly);
> 
> Since we know that we putDirectWithoutTransition() the original values
> above, can you just getDirect the values instead of going thru get() which
> does a lot more work?

OK, I'll fix that

> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:162
> > +void ArrayPrototype::copyIterableMethods(ExecState& state, JSObject& prototype)
> > +{
> > +    VM& vm = state.vm();
> > +    auto copyProperty = [&] (const Identifier& arrayIdentifier, const Identifier& otherIdentifier, unsigned attributes = 0) {
> > +        JSValue value = get(&state, arrayIdentifier);
> > +        ASSERT(value);
> > +        prototype.putDirect(vm, otherIdentifier, value, attributes);
> > +    };
> > +
> > +    copyProperty(vm.propertyNames->builtinNames().entriesPrivateName(), vm.propertyNames->builtinNames().entriesPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().forEachPrivateName(), vm.propertyNames->builtinNames().forEachPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().keysPrivateName(), vm.propertyNames->builtinNames().keysPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().valuesPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), DontEnum);
> > +}
> 
> Is there a reason for copyIterableMethods() in ArrayPrototype instead of
> inlining it in JSDOMIterator's addValueIterableMethods()?  It seems to me
> that the properties to be copied is defined by JSDOMIterator and not by
> ArrayPrototype.  Hence, I think belongs in JSDOMIterator than here.

I agree it belongs naturally to WebCore/JSDOMIterator.
The issue is that we would need to get access to JSC private names from WebCore.
This would require marking a bunch of header files as private plus adding the related WebCore proxy header files.

[Mark Lam]

(In reply to comment #13)
> I agree it belongs naturally to WebCore/JSDOMIterator.
> The issue is that we would need to get access to JSC private names from
> WebCore.
> This would require marking a bunch of header files as private plus adding
> the related WebCore proxy header files.

Can you check how big a pain this actually is?  I would think that we probably want the JS bindings to be able to access this info.  Based on what you find, we can consider the options.  Thanks.

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #13)
> > I agree it belongs naturally to WebCore/JSDOMIterator.
> > The issue is that we would need to get access to JSC private names from
> > WebCore.
> > This would require marking a bunch of header files as private plus adding
> > the related WebCore proxy header files.
> 
> Can you check how big a pain this actually is?  I would think that we
> probably want the JS bindings to be able to access this info.  Based on what
> you find, we can consider the options.  Thanks.

OK, I will try it tomorrow.
I think it will be like 4 or 5 new forwarding headers, including the generated JSCBuiltins.h

[youenn fablet]

Created attachment 283398 [details]
Patch

[WebKit Commit Bot]

Attachment 283398 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMIterator.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 283398 [details]
Patch

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

LGTM (with suggested fixes), but please have someone who is more familiar with the DOM side take a closer look as well.  Thanks.

> Source/WebCore/ChangeLog:14
> +        [re: entries] If the interface has a value iterator, then the Function object is the initial value of the âentriesâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> +        [re: keys] If the interface has a value iterator, then the Function object is the initial value of the âkeysâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> +        [re: forEach] If the interface defines an indexed property getter, then the Function object is the initial value of the âforEachâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).

Please fix the non-ascii characters around "âentriesâ","âkeysâ", and "âforEachâ".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.

Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.

[Alex Christensen]

Comment on attachment 283398 [details]
Patch

Chrome and Firefox fail these new tests in different ways.  Are they both wrong?

[youenn fablet]

> Chrome and Firefox fail these new tests in different ways.  Are they both
> wrong?

Firefox is only implementing for-of loop of NodeList, hence the various failures.

Chrome is passing fast/dom/NodeList/nodelist-iterable.html.
It is also passing most of nodelist-iterable.html, especially the bits that check the day-to-day behaviour of NodeList iterable.

The failing checks of nodelist-iterable.html are quite specific.
The first failing test is due to the fact that the function objects are not the same between NodeList and Array.
Although the spec mandates that, it can be seen more or less an implementation detail.

The second failing test in Chrome is the case of an iterator which is done=true after next() being called several time.
An item is then added to the array. Calling next() on the iterator should consistently return done=true.
This is the behaviour of Array and should also be the behaviour of DOM iterators.
WebKit is exhibiting this behaviour with the patch, chrome is not.

So, even though there is not full consistency with other browsers, I think we are safe.

[youenn fablet]

(In reply to comment #18)
> Comment on attachment 283398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283398&action=review
> 
> LGTM (with suggested fixes), but please have someone who is more familiar
> with the DOM side take a closer look as well.  Thanks.
> 
> > Source/WebCore/ChangeLog:14
> > +        [re: entries] If the interface has a value iterator, then the Function object is the initial value of the âentriesâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> > +        [re: keys] If the interface has a value iterator, then the Function object is the initial value of the âkeysâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> > +        [re: forEach] If the interface defines an indexed property getter, then the Function object is the initial value of the âforEachâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> 
> Please fix the non-ascii characters around "âentriesâ","âkeysâ", and
> "âforEachâ".
OK

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> > +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
> 
> Please create a bug to track this (if one does not already exists) and add
> the url here.  That way, we can remove this FIXME when the bug is resolved.

OK, bug number is 159140.

[Mark Lam]

Comment on attachment 283398 [details]
Patch

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

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
>>> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
>> 
>> Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.
> 
> OK, bug number is 159140.

Just to clarify.  Please add the bug url to the comment in the code before landing your patch.  Thanks.

[youenn fablet]

Created attachment 283497 [details]
Patch

[youenn fablet]

(In reply to comment #22)
> Comment on attachment 283398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283398&action=review
> 
> >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> >>> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
> >> 
> >> Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.
> > 
> > OK, bug number is 159140.
> 
> Just to clarify.  Please add the bug url to the comment in the code before
> landing your patch.  Thanks.

Sure, patch is updated accordingly.
I also tried to clarify a bit the binding generator by introducing IsKeyValueIterableInterface in addition to IsValueIterableInterface

[WebKit Commit Bot]

Attachment 283497 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMIterator.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 283497 [details]
Patch

r=me

[youenn fablet]

Created attachment 283612 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 283612 [details]
Patch for landing

Rejecting attachment 283612 [details] from commit-queue.

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-referrer.html
Full output: http://webkit-queues.webkit.org/results/1678811

[WebKit Commit Bot]

Created attachment 283623 [details]
Archive of layout-test-results from webkit-cq-02 for mac-yosemite

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

[youenn fablet]

Created attachment 283626 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 283626 [details]
Patch for landing

Clearing flags on attachment: 283626

Committed r203222: <http://trac.webkit.org/changeset/203222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.