Bug 188378 – Date.UTC should not return NaN with only Year param

• Component: JavaScriptCore
• Status: RESOLVED FIXED

Hi everyone,
there is a inconsistency on JSC when we try to get Date.UTC passing only the year param.
According to ES6 specs (https://www.ecma-international.org/ecma-262/8.0/index.html#sec-date.utc) the year param is required, the others params are optional.

JSC build: 234555
OS: Ubuntu 16.04 x64

Steps to reproduce:
d = Date.UTC(2015);
print(isNaN(d));

d = Date.UTC(2000 + 15, 0);
print(d == 1420070400000);


Actual results:
true
true

Expected results:
false
true

Chakra, V8 and SpiderMonkey works as expected.

cinfuzz

[Yusuke Suzuki]

Created attachment 346904 [details]
Patch

[EWS Watchlist]

Attachment 346904 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 346905 [details]
Patch

[Yusuke Suzuki]

Created attachment 346906 [details]
Patch

[EWS Watchlist]

Comment on attachment 346906 [details]
Patch

Attachment 346906 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8819741

New failing tests:
ChakraCore.yaml/ChakraCore/test/Date/dateutc.js.default
apiTests

[Yusuke Suzuki]

Created attachment 346908 [details]
Patch

[Keith Miller]

Comment on attachment 346908 [details]
Patch

r=me. Does this fix any test262 tests?

[Yusuke Suzuki]

(In reply to Keith Miller from comment #7)
> Comment on attachment 346908 [details]
> Patch
> 
> r=me. Does this fix any test262 tests?

test/built-ins/Date/UTC/return-value.js has passed! I've added it :D

[Yusuke Suzuki]

Committed r234763: <https://trac.webkit.org/changeset/234763>

[Radar WebKit Bug Importer]

<rdar://problem/43147473>

[Darin Adler]

Comment on attachment 346908 [details]
Patch

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

> Source/JavaScriptCore/runtime/DateConstructor.cpp:94
> +    double doubleArguments[7] {
> +        0, 0, 1, 0, 0, 0, 0
> +    };

Reads nicely on a single line.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:95
> +    unsigned numberOfUsedArguments = std::max(std::min<unsigned>(7U, args.size()), 1U);

I like to write clamping expressions in sequence like this:

    unsigned numberOfUsedArguments = std::max(1U, std::min<unsigned>(args.size(), 7U));

> Source/JavaScriptCore/runtime/DateConstructor.cpp:101
> +        if (!std::isfinite(doubleArguments[i]) || (doubleArguments[i] > INT_MAX) || (doubleArguments[i] < INT_MIN))

This can be done more efficiently without an explicit isfinite check:

    if (!(doubleArguments[i] >= std::numeric_limits<int>::min() && doubleArguments[i] <= std::numeric_limits<int>::max()))

By doing the checks in this fashion, NaN and both infinities are handled naturally without any explicit check.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:106
>      int year = JSC::toInt32(doubleArguments[0]);

Not new to this patch: I don’t think it is correct to call toInt32 after checking if the value is in range.

I believe this means the range check doesn’t take the rounding done by toInt32 into account and I suspect we will have incorrect results for values right on the edge of the acceptable range. For example, if the value is the maximum integer + 0.1 or the minimum integer - 0.1 it will be treated as NaN, but should not be. Someone should add test cases to cover these edges and then change this to use the correct idiom.

I also suspect that there is a more efficient way to do the JSC::toInt32 operation when we know we don’t want to convert any values outside the 32-bit integer range. For example, maybe std::lround already does the right thing efficiently for that case?

Being more efficient here is not critical, but it would be nice to handle the edges 100% correctly.