Bug 167091 - JavaScript for-of does not work on a lot of collection types (e.g. HTMLCollection)
Summary: JavaScript for-of does not work on a lot of collection types (e.g. HTMLCollec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 10
Hardware: Mac macOS 10.12
: P2 Normal
Assignee: Chris Dumez
URL: https://heycam.github.io/webidl/#es-i...
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-16 08:05 PST by Jimmy Joe Shabadoo
Modified: 2017-01-23 10:43 PST (History)
12 users (show)

See Also:


Attachments
WIP Patch (5.38 KB, patch)
2017-01-20 20:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (14.96 KB, patch)
2017-01-21 09:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (18.91 KB, patch)
2017-01-21 09:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (1.69 MB, application/zip)
2017-01-21 10:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (828.99 KB, application/zip)
2017-01-21 10:43 PST, Build Bot
no flags Details
Patch (38.18 KB, patch)
2017-01-21 11:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (370.72 KB, application/zip)
2017-01-21 12:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (4.30 MB, application/zip)
2017-01-21 12:45 PST, Build Bot
no flags Details
Patch (38.35 KB, patch)
2017-01-21 12:52 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (350.53 KB, application/zip)
2017-01-21 13:45 PST, Build Bot
no flags Details
Patch (38.37 KB, patch)
2017-01-21 14:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (39.21 KB, patch)
2017-01-21 20:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jimmy Joe Shabadoo 2017-01-16 08:05:08 PST
Consider the following code:

<html>
    <head>
        <meta charset="utf-8">
    </head>
    <body>
        <div class="cell">div 1 text</div>
        <div class="cell">div 2 text</div>
        <div class="cell">div 3 text</div>
        <script type="text/javascript">
            let list = document.getElementsByClassName("cell")
                for(let y of list){
                    console.log("y:", y)
                }
        </script>  
    </body>
</html>

When run in Chrome V55.0, it produces the following console output:

y:        <div class="cell">div 1 text</div>
y:        <div class="cell">div 2 text</div>
y:        <div class="cell">div 3 text</div>
Firefox V50.1 returns similar results as well.

However when run in Safari, this same file generates an error stating

TypeError: y of list is not a function. (In 'y of list', 'y of list' is >undefined).

This appears to be because the value returned to the list variable is not an array, but an HTML collection as described in the following link:

https://bugs.chromium.org/p/chromium/issues/detail?id=538558
Comment 1 Ryosuke Niwa 2017-01-17 18:30:00 PST
Yeah, we should do this.
Comment 2 Chris Dumez 2017-01-17 19:20:49 PST
Per https://heycam.github.io/webidl/#es-iterator:
There should be an @@iterator if:
"an indexed property getter and an integer-typed attribute named “length”"

This applies to HTMLCollection for e.g. but I believe our bindings generator currently only adds an @@iterator if there is an maplike/setlike/iterable declaration in the IDL. However, I believe HTMLCollection is intentionally not marked as iterable<> in the IDL because this would add extra methods (forEach, keys, values, entries) that we do not want to expose because HTMLCollection also has a named property getter.

I can implement this soon unless someone gets to it first.
Comment 3 Ryosuke Niwa 2017-01-17 19:39:43 PST
Oh yeah, I do recall some discussions about named properties and new methods. I think there was some compat. concern there.
Comment 4 Chris Dumez 2017-01-20 16:59:09 PST
Started working on this.
Comment 5 Chris Dumez 2017-01-20 20:10:14 PST
WebKit interfaces that seem to be impacted by this:
AudioTrackList
ClientRectList
CSSRuleList
CSSStyleDeclaration
DataTransferItemList
DeprecatedCSSOMValueList
DOMMimeTypeArray
DOMNamedFlowCollection
DOMPlugin
DOMPluginArray
DOMStringList
FileList
HTMLAllCollection
HTMLCollection
HTMLFormElement
HTMLOptionsCollection
HTMLSelectElement
MediaList
NamedNodeMap
SourceBufferList
StyleSheetList
TextTrackCueList
TextTrackList
TouchList
VideoTrackList
VTTRegionList
Comment 6 Chris Dumez 2017-01-20 20:54:49 PST
Created attachment 299421 [details]
WIP Patch

Needs tests.
Comment 7 Chris Dumez 2017-01-21 09:07:05 PST
Created attachment 299431 [details]
WIP Patch
Comment 8 Chris Dumez 2017-01-21 09:22:23 PST
Created attachment 299432 [details]
WIP Patch
Comment 9 Chris Dumez 2017-01-21 09:27:02 PST
Comment on attachment 299432 [details]
WIP Patch

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

> LayoutTests/fast/dom/collection-iterators-expected.txt:98
> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined

@Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
Comment 10 Chris Dumez 2017-01-21 09:29:25 PST
Comment on attachment 299432 [details]
WIP Patch

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

>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
> 
> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.

> Array.prototype.values.call(document.all)
< TypeError: Array.prototype.values requires that |this| not be null or undefined
Comment 11 Chris Dumez 2017-01-21 09:39:39 PST
Comment on attachment 299432 [details]
WIP Patch

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

>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
>> 
>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
> 
> 

From ArrayPrototype.js:

function values()
{
    "use strict";

    if (this == null)
        @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");

    return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
}

So somehow |this| ends up being null even though I am calling with an object:
Array.prototype.values.call(document.all)
Comment 12 Chris Dumez 2017-01-21 09:48:27 PST
Comment on attachment 299432 [details]
WIP Patch

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

>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
>>> 
>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
>> 
>> 
> 
> From ArrayPrototype.js:
> 
> function values()
> {
>     "use strict";
> 
>     if (this == null)
>         @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
> 
>     return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
> }
> 
> So somehow |this| ends up being null even though I am calling with an object:
> Array.prototype.values.call(document.all)

Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined.

Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow.
I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.
Comment 13 Chris Dumez 2017-01-21 09:53:00 PST
Comment on attachment 299432 [details]
WIP Patch

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

>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
>>>> 
>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
>>> 
>>> 
>> 
>> From ArrayPrototype.js:
>> 
>> function values()
>> {
>>     "use strict";
>> 
>>     if (this == null)
>>         @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
>> 
>>     return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
>> }
>> 
>> So somehow |this| ends up being null even though I am calling with an object:
>> Array.prototype.values.call(document.all)
> 
> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined.
> 
> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow.
> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.

From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all:
"""
The object returned for all has several unusual behaviors:

The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all.

The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.)

The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all.
"""

So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not.
Comment 14 Chris Dumez 2017-01-21 10:17:21 PST
Comment on attachment 299432 [details]
WIP Patch

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

>>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
>>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
>>>>> 
>>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
>>>> 
>>>> 
>>> 
>>> From ArrayPrototype.js:
>>> 
>>> function values()
>>> {
>>>     "use strict";
>>> 
>>>     if (this == null)
>>>         @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
>>> 
>>>     return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
>>> }
>>> 
>>> So somehow |this| ends up being null even though I am calling with an object:
>>> Array.prototype.values.call(document.all)
>> 
>> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined.
>> 
>> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow.
>> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.
> 
> From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all:
> """
> The object returned for all has several unusual behaviors:
> 
> The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all.
> 
> The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.)
> 
> The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all.
> """
> 
> So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not.

EcmaScript spec is at:
https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array.prototype.values

It says:
"""
Let O be ? ToObject(this value).
Return CreateArrayIterator(O, "value").
"""

And the section for ToObject says to throw a TypeError if the argument type is undefined or null.

Our code does a "this == null" check which returns true if the type masquerades as undefined. I think changing the check to (this === null || this === undefined) would match the EcmaScript specification and allow the Array prototype functions to be used on types that masquerades as undefined (such as document.all).
Comment 15 Build Bot 2017-01-21 10:37:52 PST
Comment on attachment 299432 [details]
WIP Patch

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

New failing tests:
fast/dom/FileList-iterator.html
Comment 16 Build Bot 2017-01-21 10:37:57 PST
Created attachment 299433 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-01-21 10:43:00 PST
Comment on attachment 299432 [details]
WIP Patch

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

New failing tests:
fast/dom/collection-iterators.html
Comment 18 Build Bot 2017-01-21 10:43:05 PST
Created attachment 299434 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 19 Chris Dumez 2017-01-21 11:22:38 PST
Created attachment 299437 [details]
Patch
Comment 20 Build Bot 2017-01-21 12:15:45 PST
Comment on attachment 299437 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 21 Build Bot 2017-01-21 12:15:50 PST
Created attachment 299439 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Chris Dumez 2017-01-21 12:43:14 PST
(In reply to comment #14)
> Comment on attachment 299432 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299432&action=review
> 
> >>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
> >>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
> >>>>> 
> >>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
> >>>> 
> >>>> 
> >>> 
> >>> From ArrayPrototype.js:
> >>> 
> >>> function values()
> >>> {
> >>>     "use strict";
> >>> 
> >>>     if (this == null)
> >>>         @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
> >>> 
> >>>     return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
> >>> }
> >>> 
> >>> So somehow |this| ends up being null even though I am calling with an object:
> >>> Array.prototype.values.call(document.all)
> >> 
> >> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined.
> >> 
> >> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow.
> >> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.
> > 
> > From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all:
> > """
> > The object returned for all has several unusual behaviors:
> > 
> > The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all.
> > 
> > The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.)
> > 
> > The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all.
> > """
> > 
> > So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not.
> 
> EcmaScript spec is at:
> https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array.
> prototype.values
> 
> It says:
> """
> Let O be ? ToObject(this value).
> Return CreateArrayIterator(O, "value").
> """
> 
> And the section for ToObject says to throw a TypeError if the argument type
> is undefined or null.
> 
> Our code does a "this == null" check which returns true if the type
> masquerades as undefined. I think changing the check to (this === null ||
> this === undefined) would match the EcmaScript specification and allow the
> Array prototype functions to be used on types that masquerades as undefined
> (such as document.all).

Looks like the debug Build does not like my proposal:
stderr:
Bad global capture in builtin: 'undefined'
(function ()
{
    "use strict";
    if (this === null || this === undefined)
        @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
    return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
})
1   0x10ee82740 WTFCrash
2   0x10eb074ba JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)
3   0x10df8a333 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char>
Comment 23 Build Bot 2017-01-21 12:45:04 PST
Comment on attachment 299437 [details]
Patch

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

New failing tests:
fast/dom/collection-iterators.html
Comment 24 Build Bot 2017-01-21 12:45:10 PST
Created attachment 299440 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 25 Chris Dumez 2017-01-21 12:52:07 PST
Created attachment 299441 [details]
Patch
Comment 26 Chris Dumez 2017-01-21 13:04:37 PST
(In reply to comment #22)
> (In reply to comment #14)
> > Comment on attachment 299432 [details]
> > WIP Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=299432&action=review
> > 
> > >>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98
> > >>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined
> > >>>>> 
> > >>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
> > >>>> 
> > >>>> 
> > >>> 
> > >>> From ArrayPrototype.js:
> > >>> 
> > >>> function values()
> > >>> {
> > >>>     "use strict";
> > >>> 
> > >>>     if (this == null)
> > >>>         @throwTypeError("Array.prototype.values requires that |this| not be null or undefined");
> > >>> 
> > >>>     return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext);
> > >>> }
> > >>> 
> > >>> So somehow |this| ends up being null even though I am calling with an object:
> > >>> Array.prototype.values.call(document.all)
> > >> 
> > >> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined.
> > >> 
> > >> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow.
> > >> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.
> > > 
> > > From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all:
> > > """
> > > The object returned for all has several unusual behaviors:
> > > 
> > > The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all.
> > > 
> > > The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.)
> > > 
> > > The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all.
> > > """
> > > 
> > > So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not.
> > 
> > EcmaScript spec is at:
> > https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array.
> > prototype.values
> > 
> > It says:
> > """
> > Let O be ? ToObject(this value).
> > Return CreateArrayIterator(O, "value").
> > """
> > 
> > And the section for ToObject says to throw a TypeError if the argument type
> > is undefined or null.
> > 
> > Our code does a "this == null" check which returns true if the type
> > masquerades as undefined. I think changing the check to (this === null ||
> > this === undefined) would match the EcmaScript specification and allow the
> > Array prototype functions to be used on types that masquerades as undefined
> > (such as document.all).
> 
> Looks like the debug Build does not like my proposal:
> stderr:
> Bad global capture in builtin: 'undefined'
> (function ()
> {
>     "use strict";
>     if (this === null || this === undefined)
>         @throwTypeError("Array.prototype.values requires that |this| not be
> null or undefined");
>     return new @createArrayIterator(@Object(this), "value",
> @arrayIteratorValueNext);
> })
> 1   0x10ee82740 WTFCrash
> 2   0x10eb074ba JSC::Parser<JSC::Lexer<unsigned char>
> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)
> 3   0x10df8a333 std::__1::unique_ptr<JSC::ProgramNode,
> std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned
> char>

The asserting code looks like:
#ifndef NDEBUG
    if (m_parsingBuiltin && isProgramParseMode(parseMode)) {
        VariableEnvironment& lexicalVariables = scope->lexicalVariables();
        const HashSet<UniquedStringImpl*>& closedVariableCandidates = scope->closedVariableCandidates();
        for (UniquedStringImpl* candidate : closedVariableCandidates) {
            if (!lexicalVariables.contains(candidate) && !varDeclarations.contains(candidate) && !candidate->isSymbol()) {
                dataLog("Bad global capture in builtin: '", candidate, "'\n");
                dataLog(m_source->view());
                CRASH();
            }
        }
    }
#endif // NDEBUG

Could a person more knowledgeable with JSC help me out? Why doesn't it allow me to do a "this === null || this === undefined" in ArrayPrototype.js ? Based on the error message, looks like it does not like the part with "undefined", but this looks valid to me.
Comment 27 youenn fablet 2017-01-21 13:25:33 PST
Try @undefined
Comment 28 Build Bot 2017-01-21 13:45:39 PST
Comment on attachment 299441 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 29 Build Bot 2017-01-21 13:45:43 PST
Created attachment 299443 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 30 Chris Dumez 2017-01-21 14:38:16 PST
Created attachment 299445 [details]
Patch
Comment 31 Chris Dumez 2017-01-21 14:38:52 PST
(In reply to comment #27)
> Try @undefined

Thanks, this seems to do the trick.
Comment 32 Darin Adler 2017-01-21 19:01:52 PST
Comment on attachment 299445 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        Update Array methods to throw a TypeError when (this === null || this === undefined)
> +        instead of when (this == null). This is because (this == null) returns true for types
> +        that masquerades as undefined (such as document.all) and this prevented use of the
> +        Array API on such types. The specification only stays to use ToObject(), which throws
> +        when the input is undefined or null.

I hope the JavaScript compiler still optimizes this new case well.

> Source/WebCore/bindings/js/JSDOMIterator.cpp:70
> +    JSC::ArrayPrototype* arrayPrototype = globalObject.arrayPrototype();
> +    ASSERT(arrayPrototype);
> +
> +    JSC::ExecState* state = globalObject.globalExec();
> +    ASSERT(state);
> +    JSC::VM& vm = state->vm();
> +
> +    auto copyProperty = [&] (const JSC::Identifier& arrayIdentifier, const JSC::Identifier& otherIdentifier, unsigned attributes = 0) {
> +        JSC::JSValue value = arrayPrototype->getDirect(vm, arrayIdentifier);
> +        ASSERT(value);
> +        prototype.putDirect(vm, otherIdentifier, value, attributes);
> +    };
> +
>      copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum);

I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly.

If you apply all of that thinking, then here is my version of this function:

    auto& vm = globalObject.globalExec()->vm();
    auto& builtinNames = vm.propertyNames->builtinNames();
    auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName());
    prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum);

Or the last two lines could be combined and we could do away with the local variable.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2990
> +    #return 1 if $interface->maplike;
> +    #return 1 if $interface->setlike;

These commented-out lines of code need a comment explaining why they are commented out.
Comment 33 Darin Adler 2017-01-21 19:02:55 PST
Comment on attachment 299445 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMIterator.cpp:70
>>      copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum);
> 
> I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly.
> 
> If you apply all of that thinking, then here is my version of this function:
> 
>     auto& vm = globalObject.globalExec()->vm();
>     auto& builtinNames = vm.propertyNames->builtinNames();
>     auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName());
>     prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum);
> 
> Or the last two lines could be combined and we could do away with the local variable.

Meant to say "much prefer" not "must prefer".
Comment 34 Darin Adler 2017-01-21 19:04:19 PST
Comment on attachment 299445 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSDOMIterator.cpp:70
>>>      copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum);
>> 
>> I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly.
>> 
>> If you apply all of that thinking, then here is my version of this function:
>> 
>>     auto& vm = globalObject.globalExec()->vm();
>>     auto& builtinNames = vm.propertyNames->builtinNames();
>>     auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName());
>>     prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum);
>> 
>> Or the last two lines could be combined and we could do away with the local variable.
> 
> Meant to say "much prefer" not "must prefer".

Or maybe this version:

    auto& vm = globalObject.globalExec()->vm();
    auto& names = vm.propertyNames->builtinNames();
    prototype.putDirect(vm, names.iteratorSymbol(), globalObject.arrayPrototype()->getDirect(vm, names.valuesPrivateName()), JSC::DontEnum);
Comment 35 Darin Adler 2017-01-21 19:05:36 PST
Comment on attachment 299445 [details]
Patch

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

>>>> Source/WebCore/bindings/js/JSDOMIterator.cpp:70
>>>>      copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum);
>>> 
>>> I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly.
>>> 
>>> If you apply all of that thinking, then here is my version of this function:
>>> 
>>>     auto& vm = globalObject.globalExec()->vm();
>>>     auto& builtinNames = vm.propertyNames->builtinNames();
>>>     auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName());
>>>     prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum);
>>> 
>>> Or the last two lines could be combined and we could do away with the local variable.
>> 
>> Meant to say "much prefer" not "must prefer".
> 
> Or maybe this version:
> 
>     auto& vm = globalObject.globalExec()->vm();
>     auto& names = vm.propertyNames->builtinNames();
>     prototype.putDirect(vm, names.iteratorSymbol(), globalObject.arrayPrototype()->getDirect(vm, names.valuesPrivateName()), JSC::DontEnum);

I see now that this is using the idiom of the function above. I still like my version better!
Comment 36 youenn fablet 2017-01-21 19:15:05 PST
Comment on attachment 299445 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMIterator.cpp:66
> +        ASSERT(value);

I like this assert, it ensures that we have a meaningful property.
Maybe we should have a more precise check, for instance that this is a function.

> Source/WebCore/bindings/js/JSDOMIterator.h:37
> +void addValueIterator(JSC::JSGlobalObject&, JSC::JSObject&);

Maybe we should have  better names for these two, something like copyArrayIterableProperties and copyArrayIteratorProperty
Comment 37 Chris Dumez 2017-01-21 19:29:55 PST
(In reply to comment #36)
> Comment on attachment 299445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299445&action=review
> 
> > Source/WebCore/bindings/js/JSDOMIterator.cpp:66
> > +        ASSERT(value);
> 
> I like this assert, it ensures that we have a meaningful property.
> Maybe we should have a more precise check, for instance that this is a
> function.
> 
> > Source/WebCore/bindings/js/JSDOMIterator.h:37
> > +void addValueIterator(JSC::JSGlobalObject&, JSC::JSObject&);
> 
> Maybe we should have  better names for these two, something like
> copyArrayIterableProperties and copyArrayIteratorProperty

Actually, since addValueIterator is basically a one-liner, I think I'd prefer inlining it in the bindings generator instead of making a function call. Also, this is more consistent with the iterator for IsKeyValueIterableInterface() interfaces that is already inlined in the generator.
Comment 38 Chris Dumez 2017-01-21 20:38:53 PST
Created attachment 299455 [details]
Patch
Comment 39 WebKit Commit Bot 2017-01-21 21:49:42 PST
Comment on attachment 299455 [details]
Patch

Clearing flags on attachment: 299455

Committed r211024: <http://trac.webkit.org/changeset/211024>
Comment 40 WebKit Commit Bot 2017-01-21 21:49:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 41 Geoffrey Garen 2017-01-22 09:20:25 PST
Comment on attachment 299455 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        Update Array methods to throw a TypeError when (this === null || this === undefined)
> +        instead of when (this == null). This is because (this == null) returns true for types
> +        that masquerades as undefined (such as document.all) and this prevented use of the
> +        Array API on such types. The specification only stays to use ToObject(), which throws
> +        when the input is undefined or null.

Do we really want to edit new features to make them backwards-compatible with document.all? document.all is not standard, and we only support it for backwards compatibility.
Comment 42 Chris Dumez 2017-01-22 09:24:08 PST
(In reply to comment #41)
> Comment on attachment 299455 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299455&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        Update Array methods to throw a TypeError when (this === null || this === undefined)
> > +        instead of when (this == null). This is because (this == null) returns true for types
> > +        that masquerades as undefined (such as document.all) and this prevented use of the
> > +        Array API on such types. The specification only stays to use ToObject(), which throws
> > +        when the input is undefined or null.
> 
> Do we really want to edit new features to make them backwards-compatible
> with document.all? document.all is not standard, and we only support it for
> backwards compatibility.

As I mentioned above, for..of works on document.all in other browsers. Not supporting it would be a compatibility risk IMHO.
Comment 43 Chris Dumez 2017-01-22 09:30:46 PST
(In reply to comment #42)
> (In reply to comment #41)
> > Comment on attachment 299455 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=299455&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:12
> > > +        Update Array methods to throw a TypeError when (this === null || this === undefined)
> > > +        instead of when (this == null). This is because (this == null) returns true for types
> > > +        that masquerades as undefined (such as document.all) and this prevented use of the
> > > +        Array API on such types. The specification only stays to use ToObject(), which throws
> > > +        when the input is undefined or null.
> > 
> > Do we really want to edit new features to make them backwards-compatible
> > with document.all? document.all is not standard, and we only support it for
> > backwards compatibility.
> 
> As I mentioned above, for..of works on document.all in other browsers. Not
> supporting it would be a compatibility risk IMHO.

I guess it comes to this question: Does this make the Array API code worse to support document.all? I am not familiar enough JS to answer this question. For e.g. is the new code slower?
To me, it looks like the cost is very low to support document.all so I did it. If you disagree though, we can try not supporting for..of on document.all. While I try to be compatible with other browsers as much as possible, I think the compatibility risk here would be low given that this is a legacy API and DOM API iterators are new. Let me know what you think. I can put up a patch.
Comment 44 Joseph Pecoraro 2017-01-22 19:02:58 PST
(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > Comment on attachment 299455 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=299455&action=review
> > > 
> > > > Source/JavaScriptCore/ChangeLog:12
> > > > +        Update Array methods to throw a TypeError when (this === null || this === undefined)
> > > > +        instead of when (this == null). This is because (this == null) returns true for types
> > > > +        that masquerades as undefined (such as document.all) and this prevented use of the
> > > > +        Array API on such types. The specification only stays to use ToObject(), which throws
> > > > +        when the input is undefined or null.
> > > 
> > > Do we really want to edit new features to make them backwards-compatible
> > > with document.all? document.all is not standard, and we only support it for
> > > backwards compatibility.
> > 
> > As I mentioned above, for..of works on document.all in other browsers. Not
> > supporting it would be a compatibility risk IMHO.
> 
> I guess it comes to this question: Does this make the Array API code worse
> to support document.all? I am not familiar enough JS to answer this
> question.

The exact same case came up in bug 144367. I didn't land that because I was okay with ignoring document.all to discourage its use.

> For e.g. is the new code slower?

`this == null` was chosen over other patterns because it produced noticeably smaller bytecode (see bug 159899). In theory that could help the chances of bigger functions reaching higher tiers if this was inlined into the function. But I know of no test cases that saw performance benefits.

> To me, it looks like the cost is very low to support document.all so I did
> it. If you disagree though, we can try not supporting for..of on
> document.all. While I try to be compatible with other browsers as much as
> possible, I think the compatibility risk here would be low given that this
> is a legacy API and DOM API iterators are new. Let me know what you think. I
> can put up a patch.

If you want to keep this, I suggest updating Array.from to work as well (which was bug 144367 that has a near identical code change not addressed here).
Comment 45 Darin Adler 2017-01-22 22:23:53 PST
I’d lean toward not writing special code just for document.all. On the other hand, document.all behavior differently effectively exposes the fact that we used JavaScript and an explicit null check to implement these functions, which is definitely a bad thing.

Recall that the document.all trick is trying to trick website JavaScript code. Rather than writing the code differently, I could imagine us doing something at a lower level so that our built-in JavaScript code is not fooled by "masquerade as undefined".

Generally, though, it seems bad if document.all has any effect on the JavaScript code we write! Seems like the wrong thing to focus on.
Comment 46 Geoffrey Garen 2017-01-23 10:43:51 PST
> Generally, though, it seems bad if document.all has any effect on the
> JavaScript code we write! Seems like the wrong thing to focus on.

This is my main concern -- not any specific performance problem (although I agree with Joe's analysis of potential performance knock-on affects), but rather the problem of allowing a weird backwards-compatibility quirk to grow rather than shrink and eventually disappear.