Bug 26978 - valueOf called in wrong order in atan2 and date constructors.
Summary: valueOf called in wrong order in atan2 and date constructors.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: EasyFix
Depends on:
Blocks:
 
Reported: 2009-07-05 07:54 PDT by Erik Corry
Modified: 2010-09-26 14:38 PDT (History)
5 users (show)

See Also:


Attachments
Test case standalone js script (803 bytes, text/plain)
2009-07-05 07:56 PDT, Erik Corry
no flags Details
Fix for DateConstructor (6.66 KB, patch)
2010-09-25 21:48 PDT, Mark Hahnenberg
oliver: review-
Details | Formatted Diff | Diff
Fix for atan2 (2.03 KB, patch)
2010-09-25 21:48 PDT, Mark Hahnenberg
oliver: review-
Details | Formatted Diff | Diff
Change to ChangeLog (1.04 KB, patch)
2010-09-25 21:49 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix for DateConstructor v2 (8.47 KB, patch)
2010-09-26 09:38 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix for atan2 v2 (3.42 KB, patch)
2010-09-26 09:39 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Corry 2009-07-05 07:54:53 PDT
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.
Comment 1 Erik Corry 2009-07-05 07:56:07 PDT
Created attachment 32274 [details]
Test case standalone js script

Attaching test case failed.  Trying again.
Comment 2 Mark Hahnenberg 2010-09-25 21:47:01 PDT
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.
Comment 3 Mark Hahnenberg 2010-09-25 21:48:13 PDT
Created attachment 68841 [details]
Fix for DateConstructor
Comment 4 Mark Hahnenberg 2010-09-25 21:48:43 PDT
Created attachment 68842 [details]
Fix for atan2
Comment 5 Mark Hahnenberg 2010-09-25 21:49:21 PDT
Created attachment 68843 [details]
Change to ChangeLog
Comment 6 WebKit Review Bot 2010-09-25 21:49:56 PDT
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.
Comment 7 Early Warning System Bot 2010-09-25 21:54:48 PDT
Attachment 68841 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4054142
Comment 8 Oliver Hunt 2010-09-25 22:08:36 PDT
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
Comment 9 Oliver Hunt 2010-09-25 22:15:46 PDT
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
Comment 10 Mark Hahnenberg 2010-09-26 09:38:20 PDT
Created attachment 68849 [details]
Fix for DateConstructor v2

Fixed issues with the first patch.
Comment 11 Mark Hahnenberg 2010-09-26 09:39:11 PDT
Created attachment 68850 [details]
Fix for atan2 v2

Fixed issues with the first atan2 patch.
Comment 12 Oliver Hunt 2010-09-26 13:54:51 PDT
Comment on attachment 68849 [details]
Fix for DateConstructor v2

r=me, thanks!
Comment 13 Oliver Hunt 2010-09-26 13:55:16 PDT
Comment on attachment 68850 [details]
Fix for atan2 v2

r=me
Comment 14 WebKit Commit Bot 2010-09-26 14:26:32 PDT
Comment on attachment 68849 [details]
Fix for DateConstructor v2

Clearing flags on attachment: 68849

Committed r68347: <http://trac.webkit.org/changeset/68347>
Comment 15 WebKit Commit Bot 2010-09-26 14:38:40 PDT
Comment on attachment 68850 [details]
Fix for atan2 v2

Clearing flags on attachment: 68850

Committed r68348: <http://trac.webkit.org/changeset/68348>
Comment 16 WebKit Commit Bot 2010-09-26 14:38:46 PDT
All reviewed patches have been landed.  Closing bug.