Bug 81588 – Array.prototype.toString should be generic

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Erik Arvidsson]

The following fails in JSC

Array.prototype.toString.call(document.getElementsByTagName('*'))

with a TypeError.

The spec is pretty clear here that the above should work.

15.4.4.2	Array.prototype.toString ( )

NOTE	The toString function is intentionally generic; it does not require that its this value be an Array object. Therefore it can be transferred to other kinds of objects for use as a method. Whether the toString function can be applied successfully to a host object is implementation-dependent.

[Hojong Han]

Created attachment 134723 [details]
Patch

[Erik Arvidsson]

Comment on attachment 134723 [details]
Patch

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

Please add tests that ensures the correct behavior.

var obj = {__proto__: Array.prototype, 0: 'a', 1: 'b', 2: 'c', length: 3}
shouldBeEqualToString('obj.toString()', 'a,b,c');

var join = {join: function() { return 'join' }}
shouldBeEqualToString('Array.prototype.toString.call(join)', 'join')

shouldBeEqualToString('Array.prototype.toString.call(new Date)', '[object Date]')

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:260
> +            JSValue value = thisValue.asCell()->toPrimitive(exec, PreferString);

This does not look correct. How does this check for "join" etc?

[Hojong Han]

(In reply to comment #2)
> (From update of attachment 134723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134723&action=review
> 
> Please add tests that ensures the correct behavior.
> 
> var obj = {__proto__: Array.prototype, 0: 'a', 1: 'b', 2: 'c', length: 3}
> shouldBeEqualToString('obj.toString()', 'a,b,c');
> 
> var join = {join: function() { return 'join' }}
> shouldBeEqualToString('Array.prototype.toString.call(join)', 'join')
> 
> shouldBeEqualToString('Array.prototype.toString.call(new Date)', '[object Date]')
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:260
> > +            JSValue value = thisValue.asCell()->toPrimitive(exec, PreferString);
> 
> This does not look correct. How does this check for "join" etc?

Could you explain how Array.prototype.toString.call(join) results join , not [object Object]?
(and also Array.prototype.toString.call(new Date) results [object Date], not current date information like Mon Apr 02 2012 12:24:05 GMT+0900 (KST))

[Erik Arvidsson]

The spec is pretty clear here. (Sorry for the brevity, I'm using my phone)

[Hojong Han]

(In reply to comment #4)
> The spec is pretty clear here. (Sorry for the brevity, I'm using my phone)

I did misunderstand the spec. It's clear now. thanks, Erik.

[Hojong Han]

Created attachment 135074 [details]
Patch

[Darin Adler]

Comment on attachment 135074 [details]
Patch

A bug fix like this needs test cases. And it needs performance testing.

[Darin Adler]

Comment on attachment 135074 [details]
Patch

Please submit a new patch that adds tests for all the new code paths.

[Erik Arvidsson]

Comment on attachment 135074 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:270
> +                return JSValue::encode(jsMakeNontrivialString(exec, "[object ", thisObject->methodTable()->className(thisObject), "]"));

This does not seem right. You should call the original Object.prototype.toString instead of duplicating its implementation.


...and add tests

[Hojong Han]

(In reply to comment #9)
> (From update of attachment 135074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135074&action=review
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:270
> > +                return JSValue::encode(jsMakeNontrivialString(exec, "[object ", thisObject->methodTable()->className(thisObject), "]"));
> This does not seem right. You should call the original Object.prototype.toString instead of duplicating its implementation.
> ...and add tests

Thanks for reveiw. I'll apply what you recommend and tests to the new patch.

[Hojong Han]

Created attachment 135542 [details]
Patch

[Early Warning System Bot]

Comment on attachment 135542 [details]
Patch

Attachment 135542 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12317837

[Early Warning System Bot]

Comment on attachment 135542 [details]
Patch

Attachment 135542 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12320742

[Hojong Han]

Created attachment 135546 [details]
Patch

[Erik Arvidsson]

Olli, Gavin: This bug is blocking 81573 so your feedback is highly appreciated.

[Erik Arvidsson]

The reason why this is blocking 81573 is that NodeList now has Array.prototype on its prototype chain. This will cause toString on a NodeList to call Array.prototype.toString which throws in JSC. I'm tempted to submit 81573 without waiting for this but the JSC behavior until this one is fixed is pretty much unacceptable.

[Erik Arvidsson]

I think this patch might not be sufficient. If someone overrides Array.prototype.join, arrays need to reflect that. For example:

Array.prototype.join = function() { return 'join' }
shouldBeEqualToString('[0, 1, 2].toString()', 'join');

It seems like the right thing to do is to not special case real arrays and always call join as the spec says.

[Hojong Han]

(In reply to comment #17)
> I think this patch might not be sufficient. If someone overrides Array.prototype.join, arrays need to reflect that. For example:
> 
> Array.prototype.join = function() { return 'join' }
> shouldBeEqualToString('[0, 1, 2].toString()', 'join');
> 
> It seems like the right thing to do is to not special case real arrays and always call join as the spec says.

I think you are right on the basic principle. It'd better call join as your explanation.
but some considerations, such as arguments, will be necessay if join were called in toString.
I'll patch again with these.

[Gavin Barraclough]

(In reply to comment #17)
> I think this patch might not be sufficient. If someone overrides Array.prototype.join, arrays need to reflect that. For example:
> 
> Array.prototype.join = function() { return 'join' }
> shouldBeEqualToString('[0, 1, 2].toString()', 'join');

Agreed - please add this test case.  Also, due to the 'if (thisValue.isCell()) {'check, I think this will do the wrong think for non-cell this values, e.g.:

Number.prototype.join = function() { return "number join"; }
Array.prototype.toString.call(42);

> It seems like the right thing to do is to not special case real arrays and always call join as the spec says.

From a quick glance it looks like toString is more optimized that join, we could look at similarly optimizing join, but without doing so we wouldn't want to ditch the optimization from toString, and changing functionality and introducing optimizations in the same patch is a bad plan.  So it would be best for this patch to fix toString without either removing optimizations from toString or introducing them to join.

The 'inherits' check in the current patch is also bogus (not all functions inherit from JSFunction).  If we did need to check for functions, the right thing to do would be to would be to check .isFunction - however in this case the spec does not ask for the value to be a function, rather that it is callable (these may be one and the same for objects defined in the ES spec, but is not the same thing in the DOM).  The way to check for this is to check the returned CallType for CallTypeNone.  Rather than creating a new Identifier every time toString is called, one should be added to CommonIdentifiers.  The function call is passing undefined as the this value, this is also wrong.  When implementing something like this, I'd suggest the easiest way is to start by copying the text from the spec, and using this a comments to follow, to help make sure your steps are all correct!

[Gavin Barraclough]

Created attachment 136143 [details]
partial patch

Thought I'd try to help, think this might be a useful starting point.  I haven't run the existing regression tests, the new tests from your patch, Erik's proposed test case, or the one I suggest from my patch.  So there may be bugs. :-)  But hope this is useful.

[Hojong Han]

Created attachment 136203 [details]
Patch

[Hojong Han]

(In reply to comment #20)
> Created an attachment (id=136143) [details]
> partial patch
> 
> Thought I'd try to help, think this might be a useful starting point.  I haven't run the existing regression tests, the new tests from your patch, Erik's proposed test case, or the one I suggest from my patch.  So there may be bugs. :-)  But hope this is useful.

Thanks a lot, Gavin. I patched by steps you'd taught.

[Hojong Han]

Created attachment 136209 [details]
Patch

[Hojong Han]

Created attachment 136211 [details]
Patch

[Erik Arvidsson]

Comment on attachment 136211 [details]
Patch

This looks pretty good to me. I'll let Gavin or someone else that knows JSC take care of the r+

[Erik Arvidsson]

[ArrayClass] landed so when this lands JSC ports need some rebaselines

[Hojong Han]

(In reply to comment #26)
> [ArrayClass] landed so when this lands JSC ports need some rebaselines
When was it landed? I cannot find any landed codes affects this latest patch.

[Adam Barth]

> > [ArrayClass] landed so when this lands JSC ports need some rebaselines
>
> When was it landed? I cannot find any landed codes affects this latest patch.

http://trac.webkit.org/changeset/113931

[Erik Arvidsson]

The following tests probably needs rebaselines:

LayoutTests/fast/dom/HTMLSelectElement/named-options.html
LayoutTests/fast/dom/NodeList/node-list-array-class.html
LayoutTests/fast/dom/NodeList/nodelist-item-call-as-function.html

since they depend on the toString behavior of NodeList.

[Erik Arvidsson]

This is now causing Gmail to not working when building with JSC.

[WebKit Review Bot]

Comment on attachment 136211 [details]
Patch

Clearing flags on attachment: 136211

Committed r114185: <http://trac.webkit.org/changeset/114185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Anders Carlsson]

Unfortunately, this broke a bunch of JavaScript tests on the Mac bots (probably more bots as well):

fast/dom/HTMLSelectElement/named-options.html
fast/dom/NodeList/nodelist-item-call-as-function.html
fast/dom/everything-to-string.html
fast/js/array-functions-non-arrays.html
fast/js/array-prototype-properties.html
fast/js/recursion-limit-equal.html
fast/js/toString-overrides.html
inspector/console/command-line-api.html
inspector/console/console-dir.html
inspector/console/console-format-collections.html
jquery/traversing.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.2_Array_prototype_toString/S15.4.4.2_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.3_Array_prototype_toLocaleString/S15.4.4.3_A1_T1.html

I don't know if all of them are progressions so I'm going to roll out the patch.

[Alexey Proskuryakov]

This fix was rolled out in <http://trac.webkit.org/changeset/114195>, so reopening.

[Hojong Han]

(In reply to comment #30)
> This is now causing Gmail to not working when building with JSC.
Erik, could you let me know which one is "This", patch or bug?
Layout test has not been run under my development environmen
so that I've not been able to update the patch with rebaseline...
Sorry for this.

[Gavin Barraclough]

(In reply to comment #35)
> Layout test has not been run under my development environmen

If you want to contribute patches to WebKit, we do require that you run the regression tests on your changes.

[Gavin Barraclough]

Created attachment 137427 [details]
Fix

[Gavin Barraclough]

Created attachment 137430 [details]
Fix inc layout test changes!

[Hojong Han]

(In reply to comment #36)
> (In reply to comment #35)
> > Layout test has not been run under my development environmen
> 
> If you want to contribute patches to WebKit, we do require that you run the regression tests on your changes.

Yes, of course. What I meant is that my system had some problems while running layout tests for rebaseline so that I couldn't patch right away. I should have let people know this before landing.

[Gavin Barraclough]

Fixed in r114405