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.
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.
Created attachment 28723 [details] backtrace of the crash
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?
(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.
Created attachment 28729 [details] proposed fix Use strchr and correctly const_cast when attributing from formatStrings to formatString.
(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.
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.
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.
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 on attachment 28735 [details] proposed fix r=me
Landed as r41818.
r41818 broke the windows build. The change has been rolled out.
Comment on attachment 28735 [details] proposed fix Clearing review flag due to the build breakage this caused.
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).
(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.
(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.
Tested the latest patch on windows build environment and it built successfully.
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 on attachment 28844 [details] simplified second try r=me Bonus points for making sure it works on the other platforms!
Landed as r41909, now let me fetch some pop corn to watch the bot =).