Bug 141115 – new Date(NaN).toJSON() must return null instead of throwing a TypeError

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Claude Pache]

When the toJSON method is applied to an invalid date, it must return null, and not throwing a TypeError.

Note that this behaviour is different from the toISOString method.

For reference, see [ES5.1 Date.prototype.toJSON], steps 2 and 3.

[ES5.1 Date.prototype.toJSON]: http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.5.44

[Jordan Harband]

It'd be nice to fix this one - Safari/WebKit seem to be skipping step 3.

[Jordan Harband]

Created attachment 257066 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 257066 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 257066 [details]
Patch

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

> Source/JavaScriptCore/runtime/DatePrototype.cpp:1076
> +    JSValue timeValue = object->toPrimitive(exec, PreferNumber);

This patch seems wrong.

If "this" has a valueOf function, the code will call that function, but will ignore the value the function returns except for checking it to see if it’s infinity or NaN. I don’t think that’s correct behavior.

Is that really the behavior the specification calls for? If so, why?

[Jordan Harband]

Absolutely it is. It's specifically to catch the case that WebKit is buggy here on: when toISOString() would throw, toJSON should NOT throw, but should return `null`.

[Jordan Harband]

Put another way, serializing an object to JSON should ideally never cause it to throw, but you can't serialize an invalid date to JSON in WebKit - because it throws (incorrectly).

[Darin Adler]

OK, that sounds wrong.

This will work with the built in toISOString function.

But note that we intentionally call whatever toISOString function is present, which could be any arbitrary function. How can we predict what toISOString will do without calling it? It could even be a function that returns different values when you call it multiple times.

I’m pretty sure the right way to handle this is to detect that toISOString tried to throw an exception, and then clear the exception and return null. Not to try to preflight it, which can’t be done reliably.

[Yusuke Suzuki]

Comment on attachment 257066 [details]
Patch

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

>> Source/JavaScriptCore/runtime/DatePrototype.cpp:1076
>> +    JSValue timeValue = object->toPrimitive(exec, PreferNumber);
> 
> This patch seems wrong.
> 
> If "this" has a valueOf function, the code will call that function, but will ignore the value the function returns except for checking it to see if it’s infinity or NaN. I don’t think that’s correct behavior.
> 
> Is that really the behavior the specification calls for? If so, why?

This behavior is actually specified here.
http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.5.44

1. Let O be the result of calling ToObject, giving it the this value as its argument.
2. Let tv be ToPrimitive(O, hint Number).
3. If tv is a Number and is not finite, return null.
4. Let toISO be the result of calling the [[Get]] internal method of O with argument "toISOString".
5. If IsCallable(toISO) is false, throw a TypeError exception.
6. Return the result of calling the [[Call]] internal method of toISO with O as the this value and an empty argument list.

And it is the same in the ES6 spec.
http://ecma-international.org/ecma-262/6.0/#sec-date.prototype.tojso

tv is just used to check that the toPrimitive result value is finite or not.

I think this is becacuse Date#toJSON is defined as the generic function. The spec notes that

> NOTE 2 The toJSON function is intentionally generic; it does not require that its this value be a Date object. Therefore, it can be transferred to other kinds of objects for use as a method. However, it does require that any such object have a toISOString method.

So to check the held value, instead of extracting the value of Date's private field, it calls `toPrimitive` to extract it from the possibly date object.

[Darin Adler]

OK, that is a strange way to specify it, but I accept that it’s what the standard calls for.

[WebKit Commit Bot]

Comment on attachment 257066 [details]
Patch

Clearing flags on attachment: 257066

Committed r187016: <http://trac.webkit.org/changeset/187016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/22468732>