Bug 26978 – valueOf called in wrong order in atan2 and date constructors.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Erik Corry]

The attached js file should produce

onetwo
1234567
1234567

instead it produces

twoone
12345671234567
12345671234567

Replace 'print' with 'alert' to use it in a browser instead of standalone jsc.

[Erik Corry]

Created attachment 32274 [details]
Test case standalone js script

Attaching test case failed.  Trying again.

[Mark Hahnenberg]

I think these are actually two distinct bugs.

I believe the issue with atan2 is due to the fact that the order of evaluation of the arguments to a function call is unspecified in C++.  I think the compiler is actually reordering the calls to the toNumber() function in mathProtoFuncATan2().  When I ran it in gdb, argument(1) was evaluated before argument(0), which seems to confirm this hypothesis.

As for the Date constructors, the issue was that the arguments were being evaluated to numeric representations twice, once by toNumber() and once by toInt32(),  therefore the valueOf function was getting called twice.

I've created three separate patches, one for each issue and one for the overall changelog.

[Mark Hahnenberg]

Created attachment 68841 [details]
Fix for DateConstructor

[Mark Hahnenberg]

Created attachment 68842 [details]
Fix for atan2

[Mark Hahnenberg]

Created attachment 68843 [details]
Change to ChangeLog

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/DateConstructor.cpp:93:  date_fields is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/DateConstructor.cpp:164:  date_fields is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


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

[Early Warning System Bot]

Attachment 68841 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4054142

[Oliver Hunt]

Comment on attachment 68842 [details]
Fix for atan2

code is good, but you need to make a changelog -- "prepare-Changelog --bug 26978" will get all the correct formatting done so you can just fill in the details

[Oliver Hunt]

Comment on attachment 68841 [details]
Fix for DateConstructor

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

This needs a changelog as well

> JavaScriptCore/runtime/DateConstructor.cpp:100
> +        double date_fields[7];
> +        if (isnan((date_fields[0] = args.at(0).toNumber(exec)))
> +                || isnan((date_fields[1] = args.at(1).toNumber(exec)))
> +                || (numArgs >= 3 && isnan((date_fields[2] = args.at(2).toNumber(exec))))
> +                || (numArgs >= 4 && isnan((date_fields[3] = args.at(3).toNumber(exec))))
> +                || (numArgs >= 5 && isnan((date_fields[4] = args.at(4).toNumber(exec))))
> +                || (numArgs >= 6 && isnan((date_fields[5] = args.at(5).toNumber(exec))))
> +                || (numArgs >= 7 && isnan((date_fields[6] = args.at(6).toNumber(exec)))))

I think this be better done as 
double doubleArguments[7] = {args.at(0).toNumber(exec), args.at(1).toNumber(exec), ...};
then
if (isnan(doubleArguments[0]) || ...)

> JavaScriptCore/runtime/DateConstructor.cpp:172
> +    if (isnan((date_fields[0] = exec->argument(0).toNumber(exec)))
> +            || isnan((date_fields[1] = exec->argument(1).toNumber(exec)))
> +            || (n >= 3 && isnan((date_fields[2] = exec->argument(2).toNumber(exec))))
> +            || (n >= 4 && isnan((date_fields[3] = exec->argument(3).toNumber(exec))))
> +            || (n >= 5 && isnan((date_fields[4] = exec->argument(4).toNumber(exec))))
> +            || (n >= 6 && isnan((date_fields[5] = exec->argument(5).toNumber(exec))))
> +            || (n >= 7 && isnan((date_fields[6] = exec->argument(6).toNumber(exec)))))

Similar change to what i recommended above

[Mark Hahnenberg]

Created attachment 68849 [details]
Fix for DateConstructor v2

Fixed issues with the first patch.

[Mark Hahnenberg]

Created attachment 68850 [details]
Fix for atan2 v2

Fixed issues with the first atan2 patch.

[Oliver Hunt]

Comment on attachment 68849 [details]
Fix for DateConstructor v2

r=me, thanks!

[Oliver Hunt]

Comment on attachment 68850 [details]
Fix for atan2 v2

r=me

[WebKit Commit Bot]

Comment on attachment 68849 [details]
Fix for DateConstructor v2

Clearing flags on attachment: 68849

Committed r68347: <http://trac.webkit.org/changeset/68347>

[WebKit Commit Bot]

Comment on attachment 68850 [details]
Fix for atan2 v2

Clearing flags on attachment: 68850

Committed r68348: <http://trac.webkit.org/changeset/68348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.