Bug 105282 – Date.prototype.toJSON does not execute steps 1-2

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[André Bargull]

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)

[Alexey Shvayka]

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

[Alexey Shvayka]

Created attachment 371320 [details]
Patch

[Alexey Shvayka]

Created attachment 372364 [details]
Patch

With test262 imported, this patch now fixes a test.

[Oliver Hunt]

Comment on attachment 372364 [details]
Patch

lgtm (I wonder if the spec changed at some point?)

[Alexey Shvayka]

Comment on attachment 372364 [details]
Patch

Yes, but in a subtle way: ES5 introduced strict mode for built-in functions.

[EWS]

Comment on attachment 372364 [details]
Patch

Rejecting attachment 372364 [details] from review queue.

[e-mail] 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.

[EWS]

Comment on attachment 372364 [details]
Patch

Rejecting attachment 372364 [details] from commit-queue.

[e-mail] 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.

[Oliver Hunt]

(In reply to ews-feeder from comment #6)
> Comment on attachment 372364 [details]
> Patch
> 
> Rejecting attachment 372364 [details] from review queue.
> 
> [e-mail] 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.

[Alexey Shvayka]

(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.
> > 
> > [e-mail] 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.

[Ross Kirsling]

Comment on attachment 372364 [details]
Patch

Just noticed this now. r=me but I think you'll need to rebase expectations.yaml.

[Alexey Shvayka]

Created attachment 378688 [details]
Patch

Rebase patch and simplify std::isfinite check.

[Alexey Shvayka]

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.

[Ross Kirsling]

(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.

[Mark Lam]

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?

[Mark Lam]

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.

[Alexey Shvayka]

Created attachment 378761 [details]
Patch

Set reviewer and add descriptive ChangeLog.

[Mark Lam]

Comment on attachment 378761 [details]
Patch

Thanks.

[WebKit Commit Bot]

Comment on attachment 378761 [details]
Patch

Clearing flags on attachment: 378761

Committed r249861: <https://trac.webkit.org/changeset/249861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55358552>

[Alexey Shvayka]

(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.