android: fix base::Time::FromLocalExploded() crash.

android: fix base::Time::FromLocalExploded() crash.

This patch does the following:

- Provide a work-around for an Android platform bug that
  happens on older Android releases (e.g. 4.1.2), but fixed
  on later ones (e.g. 4.3), where mktime() / mktime64()
  would return -1 even when passed proper time values.

- Improve the code to properly deal with the fact that
  SysTime is actually int64 on Android, unlike other
  platforms, allowing us to remove the CHECK() that
  was triggered by the platform bug.

- Add a new unit test to verify that the new code
  doesn't crash on Android 4.1.2 anymore, and returns
  the correct values.

BUG=287821
R=jar@chromium.org,mark@chromium.org,brettw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229567

[jar (doing other things), 2013-10-16 21:36:34]

https://codereview.chromium.org/27472003/diff/1/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/27472003/diff/1/base/time/time_posix.cc#newco...
base/time/time_posix.cc:211: timestruct.tm_isdst  = -1;    // attempt to figure
it out
Did you consider using a definite isdst setting of 0 or 1?

For some time periods, you should fail, and this will happen at times other than
just after midnight.

The problem is that some exploded times have *no* corresponding
seconds-since-epoch, since they are "skipped."  

If you have a specific dst setting, then I *think* you won't fail...

[digit1, 2013-10-17 14:00:26]

https://codereview.chromium.org/27472003/diff/1/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/27472003/diff/1/base/time/time_posix.cc#newco...
base/time/time_posix.cc:211: timestruct.tm_isdst  = -1;    // attempt to figure
it out
That's an excellent idea. I've performed some more extensive testing, and on
Android, mktime() can also return -1 depending on the timezone.

I've solved by doing something like:

  mktime() as usual.
  if (result == -1):
      s0 = mktime() with tm_isdst == 0
      s1 = mktime() with tm_isdst == 1
      return the smallest of s0 and s1 that is not -1

This seems to take care of the problem in a more generic way.

https://codereview.chromium.org/27472003/diff/1/base/time/time_posix.cc#newco...
base/time/time_posix.cc:265: : std::numeric_limits<int32_t>::min();
I'm still not sure that using INT32_MAX here is sufficient, but apparently this
code is very cautious because it assumes that the client may perform many
computations with the result, so directly returning INT64_MIN/MAX here might not
be a good idea. What do you think about this?

[digit1, 2013-10-17 21:29:56]

ping?

[jar (doing other things), 2013-10-18 02:14:17]

The patch looks good now... but it should probably be for all of posix, as this
is really standard library return of -1 being handled.  I like your plan now.

https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
File base/time/time_posix.cc (right):

https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
base/time/time_posix.cc:221: #if defined(OS_ANDROID)
This probably isn't an android only problem.  It really relates to proper use of
mktime.

[digit1, 2013-10-18 08:54:59]

https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
File base/time/time_posix.cc (right):

https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
base/time/time_posix.cc:221: #if defined(OS_ANDROID)
Thanks, I've remove the #if defined(OS_ANDROID) now.

As will all things related to timezones, I hope this won't generate new
interesting "surprises" on the desktop ;-)

[kerz_google, 2013-10-18 16:34:22]

On 2013/10/18 08:54:59, digit1 wrote:
>
https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
> File base/time/time_posix.cc (right):
> 
>
https://chromiumcodereview.appspot.com/27472003/diff/7001/base/time/time_posi...
> base/time/time_posix.cc:221: #if defined(OS_ANDROID)
> Thanks, I've remove the #if defined(OS_ANDROID) now.
> 
> As will all things related to timezones, I hope this won't generate new
> interesting "surprises" on the desktop ;-)

I think for the purposes of merging this to the branch, let's keep it behind an
Android ifdef, and we can remove it on the trunk where we'll get more legs.

[jar (doing other things), 2013-10-18 21:05:09]

Patch set 3 LGTM

[commit-bot: I haz the power, 2013-10-19 08:17:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/27472003/34001

[commit-bot: I haz the power, 2013-10-19 15:38:56]

Change committed as 229567

[jar (doing other things), 2013-10-21 18:15:20]

https://codereview.chromium.org/27472003/diff/34001/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/27472003/diff/34001/base/time/time_unittest.c...
base/time/time_unittest.cc:547: TEST_F(TimeTest,
FromLocalExplodedCrashOnAndroid) {
nit: IMO, you should have "similar" tests on all platforms.

You should try to deliberately pick times that don't exist in "common" time
zones, like here in the USA.  Most critically, you should test both "ambiguous"
date/time where there are two possible answers (because the clock was turned
back), and also where there is "no generic answer" because the clock was pushed
forward.

It was interesting that we had problems in Santiago, but currently, Santiago
does not have a time change on October 13th.... so this test seems especially
weak.

Suggestion (that should work cross platform?): Validate that 3 calls result in
integers that are nice and sequential.  The middle call should be the
"troublesome time," while the bracketing calls should be something like one day
earlier, and one day later.

This should be done for both a clock jumping forward, and a clock jumping
backwards chosen as the "troublesome" date.  In addition, you should verify that
the gap between the two bracketing times is either 49 hours (jump forward), or
47 hours (fall back), so that you know you are probably testing the
"troublesome" date.