test case: --- > this.toISOString = function(){print("global toISOString")} function () {print("global toISOString");} > Date.prototype.toJSON.call(null) global toISOString > Date.prototype.toJSON.call(void 0) global toISOString > Date.prototype.toJSON.call({ valueOf: function(){print("ToPrimitive(valueOf) called"); return 0}, toISOString: function(){print("toISOString called")} }) toISOString called --- [ES5.1 - 15.9.5.44 Date.prototype.toJSON] steps 1-2 aren't executed properly: - the first two calls should raise a TypeError because ToObject() is called with an incompatible argument (step 1 of 15.9.5.44) - the last call should invoke the supplied valueOf() function (step 2 of 15.9.5.44)
As of build 246014, ToPrimitive is called with proper hint, but `this` value is still resolved using sloppy mode semantics, resulting in ToObject being called on global object. Test262: https://github.com/tc39/test262/pull/2190
Created attachment 371320 [details] Patch
Created attachment 372364 [details] Patch With test262 imported, this patch now fixes a test.
Comment on attachment 372364 [details] Patch lgtm (I wonder if the spec changed at some point?)
Comment on attachment 372364 [details] Patch Yes, but in a subtle way: ES5 introduced strict mode for built-in functions.
Comment on attachment 372364 [details] Patch Rejecting attachment 372364 [details] from review queue. oliver@nerget.com does not have reviewer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 372364 [details] Patch Rejecting attachment 372364 [details] from commit-queue. shvaikalesh@gmail.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
(In reply to ews-feeder from comment #6) > Comment on attachment 372364 [details] > Patch > > Rejecting attachment 372364 [details] from review queue. > > oliver@nerget.com does not have reviewer permissions according to > https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ > contributors.json. > Oh boo, I guess that list never got updated.
(In reply to Oliver Hunt from comment #8) > (In reply to ews-feeder from comment #6) > > Comment on attachment 372364 [details] > > Patch > > > > Rejecting attachment 372364 [details] from review queue. > > > > oliver@nerget.com does not have reviewer permissions according to > > https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ > > contributors.json. > > > > Oh boo, I guess that list never got updated. Completely my fault: I attempted commit-queue+ and miserably failed. I guess re-review would help.
Comment on attachment 372364 [details] Patch Just noticed this now. r=me but I think you'll need to rebase expectations.yaml.
Created attachment 378688 [details] Patch Rebase patch and simplify std::isfinite check.
Thank you for review. I took the liberty of tweaking time value finiteness check (step 3 of https://tc39.es/ecma262/#sec-date.prototype.tojson). While it removes weird isInt32() call, it doesn't justifies making a separate patch IMO.
(In reply to Alexey Shvayka from comment #12) > Thank you for review. > > I took the liberty of tweaking time value finiteness check (step 3 of > https://tc39.es/ecma262/#sec-date.prototype.tojson). > While it removes weird isInt32() call, it doesn't justifies making a > separate patch IMO. Yeah, that seems fine -- guess it's trying to avoid calling std::isfinite unnecessarily, but that should just amount to `return true` for integral types anyway.
Comment on attachment 378688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378688&action=review > Source/JavaScriptCore/ChangeLog:8 > + Resolve `this` value using strict mode semantics. Can you add a link to the relevant piece of the spec for this change?
Comment on attachment 378688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378688&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + Resolve `this` value using strict mode semantics. > > Can you add a link to the relevant piece of the spec for this change? nit: Please also describe what the issue is and why the fix works (if not obvious) here in the ChangeLog. I think the description in https://bugs.webkit.org/show_bug.cgi?id=105282#c0 covers this quite well. We prefer to make the ChangeLog as descriptive as possible so that if we need to investigate why a change was made, it provides the intent of the author. Granted, this info may also be present in the bugzilla, but often, the thinking about an issue changes over the period when the issue is being investigated. Capturing the final understanding on the nature of bug and its fix in the ChangeLog makes it more digestible than having to splunk through the bugzilla comment trail. Secondly, in my personal experience, when I have to write a descriptive ChangeLog, it forces me to make sure that I really understand what I'm changing, and serves as a sanity check to prevent bugs from creeping in. So, please add a descriptive ChangeLog, plus a link to the relevant section of the spec for reference. Thanks.
Created attachment 378761 [details] Patch Set reviewer and add descriptive ChangeLog.
Comment on attachment 378761 [details] Patch Thanks.
Comment on attachment 378761 [details] Patch Clearing flags on attachment: 378761 Committed r249861: <https://trac.webkit.org/changeset/249861>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55358552>
(In reply to Ross Kirsling from comment #13) > > Yeah, that seems fine -- guess it's trying to avoid calling std::isfinite > unnecessarily, but that should just amount to `return true` for integral > types anyway. Thanks, it didn't occur to me that std::isfinite might be slow. I've benchmarked code path that is largely dependent on finiteness check performance with: ``` const date = new Date(NaN); for (let i = 0; i < 1e6; ++i) date.toJSON(); ``` Proposed tweak and trunk did perform equally (within 1%). (In reply to Mark Lam from comment #15) > > Secondly, in my personal experience, when I have to write a descriptive > ChangeLog, it forces me to make sure that I really understand what I'm > changing, and serves as a sanity check to prevent bugs from creeping in. Thank you for detailed explanation. As outside contributor, I find ChangeLogs extremely useful as sources of motivation for some classes like JSCell or CustomGetterSetter. Also, descriptive ChangeLog for this patch in particular is highly important, because first comment and bug title are quite different from what the patch fixes.