Bug 5280 - Date.setMonth fails with negative values
Summary: Date.setMonth fails with negative values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL: http://www.rte.ie/news/nuacht.html
Keywords:
: 5154 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-10-05 21:29 PDT by Maciej Stachowiak
Modified: 2005-10-16 02:00 PDT (History)
2 users (show)

See Also:


Attachments
testcase (296 bytes, text/html)
2005-10-08 02:01 PDT, mitz
no flags Details
handle negative month values in kjs::maketime (899 bytes, patch)
2005-10-08 02:51 PDT, mitz
no flags Details | Formatted Diff | Diff
regression test (1.67 KB, patch)
2005-10-08 03:02 PDT, mitz
no flags Details | Formatted Diff | Diff
remove code to use CFDate, use NaN instead of LONG_MIN for bad dates (17.80 KB, patch)
2005-10-08 23:11 PDT, Darin Adler
mjs: review-
Details | Formatted Diff | Diff
alternate version of patch that uses _r versions of functions too (either one seems fine to me) (21.38 KB, patch)
2005-10-08 23:37 PDT, Darin Adler
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2005-10-05 21:29:42 PDT
Steps to reproduce:

1) Go to http://www.rte.ie/news/nuacht.html

Results:

hang
Comment 1 Maciej Stachowiak 2005-10-06 02:03:09 PDT
Also filed as <rdar://problem/4092064> hanging loading page; rte.ie (works in IE and Firefox)
Comment 2 mitz 2005-10-08 02:01:27 PDT
Created attachment 4252 [details]
testcase

It's got something to do with dates, but is it a bug in JSC's date
implementation? Investigating...
Comment 3 mitz 2005-10-08 02:51:06 PDT
Created attachment 4254 [details]
handle negative month values in kjs::maketime
Comment 4 mitz 2005-10-08 03:02:26 PDT
Created attachment 4255 [details]
regression test
Comment 5 Darin Adler 2005-10-08 11:11:13 PDT
Comment on attachment 4254 [details]
handle negative month values in kjs::maketime

There really doesn't need to be a second branch. One case for negative numbers
and another for >= 0 would have been fine.

That having been said, this does look fine as-is. r=me
Comment 6 Darin Adler 2005-10-08 23:01:40 PDT
I think a better fix is to not use CoreFoundation-based date routines. I have a patch in the works to do 
that.
Comment 7 Darin Adler 2005-10-08 23:11:35 PDT
Created attachment 4262 [details]
remove code to use CFDate, use NaN instead of LONG_MIN for bad dates

This patch has a few changes, but I think it's still fine to land it all at
once:

    1) use the real gmtime, localtime, mktime, and time
    2) some tweaks to styleFromArgString and formatLocaleDate (mainly to use
arg values intead of arg counts)
    3) added a FIXME to some code that looks broken for the non-__APPLE__ case
    4) took out unnecessary defined use (#if !X does work fine if X is not
defined at all!)
    5) changed invalidDate to NaN now that it's used in a context where we have
a double rather than a time_t
    6) removed some code that's now unnecessary if invalidDate is NaN
    7) put the code to use CoreFoundation to do locale-specific dates inside
__APPLE__ rather than APPLE_CHANGES
Comment 8 Darin Adler 2005-10-08 23:13:23 PDT
Forgot to mention: I ran all the tests and everything passes, including the new regression test that Mitz 
attached to this bug.
Comment 9 Darin Adler 2005-10-08 23:37:25 PDT
Created attachment 4263 [details]
alternate version of patch that uses _r versions of functions too (either one seems fine to me)

I think it's fine to use the _r versions of the functions even if they aren't
universally available. It's easy to provide "adapter" implementations for
platforms that lack them even if they aren't threadsafe.
Comment 10 Darin Adler 2005-10-08 23:38:49 PDT
These patches also address bug 5154.
Comment 11 Maciej Stachowiak 2005-10-09 13:33:06 PDT
Comment on attachment 4263 [details]
alternate version of patch that uses _r versions of functions too (either one seems fine to me)

Using the _r versions is better. For platforms that don't have them, we can
solve it on a case-by-case basis.
Comment 12 Maciej Stachowiak 2005-10-09 13:34:50 PDT
Comment on attachment 4262 [details]
remove code to use CFDate, use NaN instead of LONG_MIN for bad dates

Marking this alternate patch r- so it goes away from the review queue.
Comment 13 Geoffrey Garen 2005-10-09 14:08:54 PDT
*** Bug 5154 has been marked as a duplicate of this bug. ***