Bug 24674 - Crashes in !PLATFORM(MAC)'s formatLocaleDate, in very specific situations
Summary: Crashes in !PLATFORM(MAC)'s formatLocaleDate, in very specific situations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-18 10:10 PDT by Gustavo Noronha (kov)
Modified: 2009-03-23 08:14 PDT (History)
1 user (show)

See Also:


Attachments
proposed fix (2.92 KB, patch)
2009-03-18 10:16 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
backtrace of the crash (6.80 KB, text/plain)
2009-03-18 10:18 PDT, Gustavo Noronha (kov)
no flags Details
proposed fix (2.94 KB, patch)
2009-03-18 13:24 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
proposed fix (1.67 KB, patch)
2009-03-18 15:23 PDT, Thadeu Lima de Souza Cascardo
no flags Details | Formatted Diff | Diff
proposed fix (2.76 KB, patch)
2009-03-18 15:53 PDT, Gustavo Noronha (kov)
mrowe: review-
Details | Formatted Diff | Diff
second try (4.64 KB, patch)
2009-03-19 08:08 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
simplified second try (4.01 KB, patch)
2009-03-22 21:58 PDT, Gustavo Noronha (kov)
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-03-18 10:10:33 PDT
While testing JavaScriptCore for the GTK+ port, I found that test ecma_3/Date/15.9.5.6.js was crashing. It was being run with LC_ALL=C and TZ=PST+8. The problem is that JavaScriptCore assumes strftime's %#x format will always generate >= 4-digits years, and the call it does to strstr() fails when 2-year digits are returned.
Comment 1 Gustavo Noronha (kov) 2009-03-18 10:16:27 PDT
Created attachment 28722 [details]
proposed fix

Would be good to see if this is a GNU/Linux specific problem, or if other libcs would also return 2-digits years in those circunstances.
Comment 2 Gustavo Noronha (kov) 2009-03-18 10:18:19 PDT
Created attachment 28723 [details]
backtrace of the crash
Comment 3 Darin Adler 2009-03-18 10:45:06 PDT
Comment on attachment 28722 [details]
proposed fix

> +#include <langinfo.h>

Is langinfo.h available on all the platforms we need to support, such as Windows? If not, then this needs to be inside an #if.

> +        char* yPos = strstr(formatString, "y");

Should be strchr, not strstr.

Is this really the simplest possible solution?
Comment 4 Gustavo Noronha (kov) 2009-03-18 12:42:18 PDT
(In reply to comment #3)
> (From update of attachment 28722 [details] [review])
> > +#include <langinfo.h>
> 
> Is langinfo.h available on all the platforms we need to support, such as
> Windows? If not, then this needs to be inside an #if.

According to http://www.cygwin.com/cygwin-api/compatibility.html#std-susv3, it should be available for windows. Since it's POSIX.1-2001, I'm pretty sure all the unixy platforms will have it. Is there a canonical list of platforms we need to support, by the way?

> Is this really the simplest possible solution?

I have put some thought on the problem, and couldn't find a solution that would be simple, not impact the performance of the common case, and still work for cases where the year needs to be offset. I'm open for ideas to try out, though.
Comment 5 Gustavo Noronha (kov) 2009-03-18 13:24:27 PDT
Created attachment 28729 [details]
proposed fix

Use strchr and correctly const_cast when attributing from formatStrings to formatString.
Comment 6 Darin Adler 2009-03-18 13:33:27 PDT
(In reply to comment #4)
> Since it's POSIX.1-2001, I'm pretty sure all
> the unixy platforms will have it. Is there a canonical list of platforms we
> need to support, by the way?

There's not.
Comment 7 Thadeu Lima de Souza Cascardo 2009-03-18 15:23:35 PDT
Created attachment 28733 [details]
proposed fix

This is more simple and does not present the problems with parseDate either. kov should present some performance tests he made with it.
Comment 8 Gustavo Noronha (kov) 2009-03-18 15:35:11 PDT
While discussing this with cascardo we found out that never allowing strftime to return 2-digit years has no apparent performance impact (I tried sunspider runs and made a small test js script to run toLocaleDate() on a date object with current time, so I'd agree with going that route. The parseDate problem he mentions is because we'd need extra checks for years between 1900 and 1950.
Comment 9 Gustavo Noronha (kov) 2009-03-18 15:53:07 PDT
Created attachment 28735 [details]
proposed fix

Here's a properly formatted patch, including a changelog entry, with the simplified approach. I tested that it fixes the crash and the test passes.
Comment 10 Darin Adler 2009-03-18 16:01:58 PDT
Comment on attachment 28735 [details]
proposed fix

r=me
Comment 11 Gustavo Noronha (kov) 2009-03-18 16:22:38 PDT
Landed as r41818.
Comment 12 Ada Chan 2009-03-18 21:48:55 PDT
r41818 broke the windows build.  The change has been rolled out.
Comment 13 Mark Rowe (bdash) 2009-03-19 03:32:01 PDT
Comment on attachment 28735 [details]
proposed fix

Clearing review flag due to the build breakage this caused.
Comment 14 Gustavo Noronha (kov) 2009-03-19 08:08:24 PDT
Created attachment 28752 [details]
second try

I am not sure why langinfo.h is not found on windows, since cygwin's documentation says it's supported; it may be some build setup, and I'd be interested in having someone from the windows port work with me on this. In the meanwhile, I am proposing the change again, this time wrapped in a check HAVE(LANGINFO_H).
Comment 15 Adam Roben (:aroben) 2009-03-19 10:03:16 PDT
(In reply to comment #14)
> I am not sure why langinfo.h is not found on windows, since cygwin's
> documentation says it's supported;

Apple's Windows port doesn't use Cygwin headers in its build.
Comment 16 Gustavo Noronha (kov) 2009-03-19 10:35:50 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > I am not sure why langinfo.h is not found on windows, since cygwin's
> > documentation says it's supported;
> 
> Apple's Windows port doesn't use Cygwin headers in its build.

Yeah, Ada told me that. I was under this impression because the page about building Webkit mentions cygwin: http://webkit.org/building/build.html. As does the tools page: http://webkit.org/building/tools.html. I think the additional condition is the way to go, in this case.
Comment 17 Ada Chan 2009-03-19 12:39:29 PDT
Tested the latest patch on windows build environment and it built successfully.
Comment 18 Gustavo Noronha (kov) 2009-03-22 21:58:54 PDT
Created attachment 28844 [details]
simplified second try

I modified the patch so that it doesn't duplicate code. I set up a windows build environment and tested that the patch builds, too. Also tested with Qt. The test I originally set out to fix is fixed, too =).
Comment 19 Adam Roben (:aroben) 2009-03-23 06:59:00 PDT
Comment on attachment 28844 [details]
simplified second try

r=me

Bonus points for making sure it works on the other platforms!
Comment 20 Gustavo Noronha (kov) 2009-03-23 08:14:37 PDT
Landed as r41909, now let me fetch some pop corn to watch the bot =).