Switch from wcsftime to GetDateFormat to avoid crash in setup....

Switch from wcsftime to GetDateFormat to avoid crash in setup.

TEST=Fix for crash in setup.  See bug for details.
BUG=73212
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75305

[robertshield, 2011-02-17 01:47:30]

On 2011/02/16 22:44:50, tommi wrote:

LGTM

Change itself looks good to me, but the bug doesn't seem to contain information
on what the cause of the crash with the old implementation was.

[tommi (sloooow) - chröme, 2011-02-17 02:56:08]

The exception that the crt is throwing is because of invalid input.
This suggests that either time(NULL) or localtime_s() was doing something
bad.
It's not clear to me from the dump exactly what it was that triggered the
error but
I figured since there's a simple single method offered by the os and does
not raise
this kind of exception, I could just take the crt out of the equation and as
a
bonus add a test.

On Wed, Feb 16, 2011 at 8:47 PM, <robertshield@chromium.org> wrote:

> On 2011/02/16 22:44:50, tommi wrote:
>
> LGTM
>
> Change itself looks good to me, but the bug doesn't seem to contain
> information
> on what the cause of the crash with the old implementation was.
>
>
> http://codereview.chromium.org/6533009/
>

[grt (UTC plus 2), 2011-02-17 04:04:10]

drive-by, not that you asked...

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:268: wchar_t date_str[10] = {0};
Why 10?

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:269: const wchar_t kDateFormat[] =
L"yyyyMMdd";
nit: static const?

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:270: size_t ret =
GetDateFormatW(LOCALE_USER_DEFAULT, 0, NULL, kDateFormat,
Use LOCALE_INVARIANT.  Otherwise, you could potentially write something like
43440216 into the registry (this is year 4344 in the/a Korean calendar), which
isn't what the "Programs and Features" control panel expects.  It expects
Gregorian in the registry, which it appears to parse then format according to
the user's display region.

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:270: size_t ret =
GetDateFormatW(LOCALE_USER_DEFAULT, 0, NULL, kDateFormat,
MSDN says GetDateFormat returns an int.  If you chose size_t for the sake of the
DCHECK, consider switching to int if you take my advice below.

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:272: DCHECK_EQ(arraysize(kDateFormat),
ret);
MSDN says that 'yyyy' can yield a 5-digit year, which would cause this DCHECK to
fail.  Given the LOCAL_INVARIANT comment above, that shouldn't happen until
Y10k.  In either case, I don't see that this DCHECK is helpful.  How about
instead DPLOG_IF(DFATAL, ret == 0) so that we see the last-error code in logs?

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:273: return date_str;
I'm a huge fan of std::wstring(date_str, ret), which avoids a needless strlen
since you know the length.

[tommi (sloooow) - chröme, 2011-02-17 04:51:51]

Thanks Greg.

That's very useful info and I'll go over these in the morning.
I was actually going to ask you about the below and if there was ever a
chance that we could get output from non-gregorian calendars - hence the
[10] and not [9] :)

On Wed, Feb 16, 2011 at 11:04 PM, <grt@chromium.org> wrote:

> drive-by, not that you asked...
>
you're too quick for me! :)

>
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> File chrome/installer/util/install_util.cc (right):
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:268: wchar_t date_str[10] = {0};
> Why 10?
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:269: const wchar_t kDateFormat[] =
> L"yyyyMMdd";
> nit: static const?
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:270: size_t ret =
>
> GetDateFormatW(LOCALE_USER_DEFAULT, 0, NULL, kDateFormat,
> Use LOCALE_INVARIANT.  Otherwise, you could potentially write something
> like 43440216 into the registry (this is year 4344 in the/a Korean
> calendar), which isn't what the "Programs and Features" control panel
> expects.  It expects Gregorian in the registry, which it appears to
> parse then format according to the user's display region.
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:270: size_t ret =
>
> GetDateFormatW(LOCALE_USER_DEFAULT, 0, NULL, kDateFormat,
> MSDN says GetDateFormat returns an int.  If you chose size_t for the
> sake of the DCHECK, consider switching to int if you take my advice
> below.
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:272:
>
> DCHECK_EQ(arraysize(kDateFormat), ret);
> MSDN says that 'yyyy' can yield a 5-digit year, which would cause this
> DCHECK to fail.  Given the LOCAL_INVARIANT comment above, that shouldn't
> happen until Y10k.  In either case, I don't see that this DCHECK is
> helpful.  How about instead DPLOG_IF(DFATAL, ret == 0) so that we see
> the last-error code in logs?
>
>
>
http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
> chrome/installer/util/install_util.cc:273: return date_str;
> I'm a huge fan of std::wstring(date_str, ret), which avoids a needless
> strlen since you know the length.
>
>
> http://codereview.chromium.org/6533009/
>

[tommi (sloooow) - chröme, 2011-02-17 19:40:09]

ptal

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:268: wchar_t date_str[10] = {0};
On 2011/02/17 04:04:10, grt wrote:
> Why 10?

nervousness around yyyyy

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:269: const wchar_t kDateFormat[] =
L"yyyyMMdd";
On 2011/02/17 04:04:10, grt wrote:
> nit: static const?

Done.

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:270: size_t ret =
GetDateFormatW(LOCALE_USER_DEFAULT, 0, NULL, kDateFormat,
On 2011/02/17 04:04:10, grt wrote:
> Use LOCALE_INVARIANT.  Otherwise, you could potentially write something like
> 43440216 into the registry (this is year 4344 in the/a Korean calendar), which
> isn't what the "Programs and Features" control panel expects.  It expects
> Gregorian in the registry, which it appears to parse then format according to
> the user's display region.

Done.

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:272: DCHECK_EQ(arraysize(kDateFormat),
ret);
On 2011/02/17 04:04:10, grt wrote:
> MSDN says that 'yyyy' can yield a 5-digit year, which would cause this DCHECK
to
> fail.  Given the LOCAL_INVARIANT comment above, that shouldn't happen until
> Y10k.  In either case, I don't see that this DCHECK is helpful.  How about
> instead DPLOG_IF(DFATAL, ret == 0) so that we see the last-error code in logs?

Done.

http://codereview.chromium.org/6533009/diff/1/chrome/installer/util/install_u...
chrome/installer/util/install_util.cc:273: return date_str;
On 2011/02/17 04:04:10, grt wrote:
> I'm a huge fan of std::wstring(date_str, ret), which avoids a needless strlen
> since you know the length.

Done.

[grt (UTC plus 2), 2011-02-17 20:02:50]

LGTM