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.
Created attachment 134723 [details] Patch
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?
(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))
The spec is pretty clear here. (Sorry for the brevity, I'm using my phone)
(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.
Created attachment 135074 [details] Patch
Comment on attachment 135074 [details] Patch A bug fix like this needs test cases. And it needs performance testing.
Comment on attachment 135074 [details] Patch Please submit a new patch that adds tests for all the new code paths.
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
(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.
Created attachment 135542 [details] Patch
Comment on attachment 135542 [details] Patch Attachment 135542 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12317837
Comment on attachment 135542 [details] Patch Attachment 135542 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12320742
Created attachment 135546 [details] Patch
Olli, Gavin: This bug is blocking 81573 so your feedback is highly appreciated.
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.
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.
(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.
(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!
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.
Created attachment 136203 [details] Patch
(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.
Created attachment 136209 [details] Patch
Created attachment 136211 [details] Patch
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+
[ArrayClass] landed so when this lands JSC ports need some rebaselines
(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.
> > [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
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.
This is now causing Gmail to not working when building with JSC.
Comment on attachment 136211 [details] Patch Clearing flags on attachment: 136211 Committed r114185: <http://trac.webkit.org/changeset/114185>
All reviewed patches have been landed. Closing bug.
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.
This fix was rolled out in <http://trac.webkit.org/changeset/114195>, so reopening.
(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.
(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.
Created attachment 137427 [details] Fix
Created attachment 137430 [details] Fix inc layout test changes!
(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.
Fixed in r114405