time overflow

Handle time overflow more gracefully in base/time_posix.cc.  Instead of
asserting when encountering time overflow, use the most distant future or past
time representation possible.  This fixes assertions that occur when handling
cookies that expire well in the future.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=4631

[Mark Mentovai, 2008-10-31 17:06:25]

This is similar, but slightly different, from PR_ImplodeTime in
base/third_party/nspr/prtime.cc.  PR_ImplodeTime seems to be present in our
codebase only to support PR_ParseTimeString, used by Time::FromString.  It might
be a good idea to get Time::FromExploded using PR_ImplodeTime too, although the
impediment is that Time::FromExploded uses an is_local bool argument and can
figure out whether DST is in effect, where PR_ImplodeTime requires that the GMT
and DST offsets be passed in, and will not figure out whether DST is in effect.

[wtc, 2008-10-31 17:39:25]

http://codereview.chromium.org/8771/diff/1/2
File base/time_posix.cc (right):

http://codereview.chromium.org/8771/diff/1/2#newcode56
Line 56: timestruct.tm_zone   = NULL;  // not a POSIX field, so mktime/timegm
ignore
If this function isn't using the tm_gmtoff and tm_zone
fields, it may be better to just zero the whole timestruct
and then assign the necessary members.

http://codereview.chromium.org/8771/diff/1/2#newcode73
Line 73: milliseconds = std::numeric_limits<time_t>::min() *
Excellent use of std::numeric_limits<time_t>.  We should
change prtime.cc to do the same.  But this code has two
problems.

milliseconds is uint64, an unsigned type.  Should we just
set milliseconds to 0 here?  Or should milliseconds be
int64?

http://codereview.chromium.org/8771/diff/1/2#newcode78
Line 78: kMillisecondsPerSecond - 1;
Why do you need + kMillisecondsPerSecond - 1?

If time_t is int64_t, this will overflow, won't it?

[wtc, 2008-10-31 20:52:01]

http://codereview.chromium.org/8771/diff/5/6
File base/time_posix.cc (right):

http://codereview.chromium.org/8771/diff/5/6#newcode73
Line 73: milliseconds = std::numeric_limits<time_t>::min() *
Some points to ponder:

- What if time_t is uint32?  Then the min of time_t is 0.
  I think this is fine for our purposes.
- What if time_t is double?  Is the range of double contained
  in the range of int64?

http://codereview.chromium.org/8771/diff/5/6#newcode78
Line 78: kMillisecondsPerSecond - 1;
The need to add 999 milliseconds is still not clear to me.
If you want to return a number greater than the max of time_t,
why don't you just add 1 millisecond or 1 second?

And why don't you do this for the min case?  Why don't you
subtract 999 milliseconds from the min of time_t?

[Mark Mentovai, 2008-10-31 21:21:01]

http://codereview.chromium.org/8771/diff/5/6
File base/time_posix.cc (right):

http://codereview.chromium.org/8771/diff/5/6#newcode73
Line 73: milliseconds = std::numeric_limits<time_t>::min() *
wtc wrote:
> - What if time_t is uint32?  Then the min of time_t is 0.
>   I think this is fine for our purposes.

Me too.  But we can add this to check our assumption:

COMPILE_ASSERT(std::numeric_limits<time_t>::min() < 0);

> - What if time_t is double?  Is the range of double contained
>   in the range of int64?

No, it's not, but double's precision is worse than 1 unit per step at the
extremities of the range supported by int64, so for us to get into this case,
we'd be dealing with a weird clock that can't accurately represent seconds
anyway.

Do you want to do this?

COMPILE_ASSERT(std::numeric_limits<int64>::min() <=
               std::numeric_limits<time_t>::min() * kMicrosecondPerSecond &&
               std::numeric_limits<int64>::max() >=
               std::numeric_limits<time_t>::max() * kMicrosecondPerSecond);

And possibly even add an extra 999ms into the last clause?

With these assertions, I think the only not-unreasonable assumption made by the
code that's unchecked is whether the system treats the epoch as 1970-01-01
00:00:00 UTC.

http://codereview.chromium.org/8771/diff/5/6#newcode78
Line 78: kMillisecondsPerSecond - 1;
wtc wrote:
> The need to add 999 milliseconds is still not clear to me.
> If you want to return a number greater than the max of time_t,
> why don't you just add 1 millisecond or 1 second?
> 
> And why don't you do this for the min case?  Why don't you
> subtract 999 milliseconds from the min of time_t?

Because in the non-overflow case, we never subtract milliseconds from the result
of mktime/timegm, we only add.  There can never be a time earlier than
1901-12-13 20:45:52.000 UTC, so the correct time to return in the case of
negative overflow must be <= 1901-12-13 20:45:52.000 UTC.  I chose = rather than
<= so that if someone tries to convert it back into a standard time_t by
truncating the fractional portion, it round-trips properly.  That is, if the
time is re-exploded, it can be imploded again without hitting the overflow
condition.  In a perfect world where everyone comes through the same API, we
could overflow on the low end with -1ms (instead of 0) and on the high end with
+1000ms (instead of 999), but realistically, it's probably too tempting for
people to try using native time routines by marshalling things back into a
time_t in non-cross-platform code, and in those cases, -1ms and +1000ms will not
necessarily be interpreted consistently as we intend.  Consider someone dividing
the time by kMicrosecondsPerSecond to feed a time_t into some interface that
requires one, like localtime+strftime or ctime, for example.  If we overflow to
something that will wind up outside of the range of time_t, this will break.

The same applies to my choice of 999ms on top of the maximum range of time_t,
2038-01-19 03:14:07.999 UTC.

To better understand the +999ms, assume a bunch of time samples taken near this
maximum at sub-second intervals.  If you didn't add 999ms, your values would
look like:

2038-01-19 03:14:07.994 UTC
2038-01-19 03:14:07.996 UTC
2038-01-19 03:14:07.998 UTC  // overflow occurs next
2038-01-19 03:14:07.000 UTC  // time went backwards! bad!
2038-01-19 03:14:07.000 UTC
2038-01-19 03:14:07.000 UTC

It's preferable to clamp things at the maximum value:

2038-01-19 03:14:07.994 UTC
2038-01-19 03:14:07.996 UTC
2038-01-19 03:14:07.998 UTC  // overflow occurs next
2038-01-19 03:14:07.999 UTC
2038-01-19 03:14:07.999 UTC
2038-01-19 03:14:07.999 UTC

In the latter case, you can't disambiguate the last three times from each other
as "real" 07.999 or "overflow" 07.999, but I think that's OK given the
round-tripping and interfacing with native time_t described above.

[wtc, 2008-11-03 19:40:27]

LGTM.

It would be nice to add a short comment to explain that
you clamp at the minimally and maximally representable
times for the time_t type and you want to make roundtripping
work.

I didn't realize Standard C doesn't specify the epoch for
time_t.  If some existing unit tests validate our assumption
of the epoch being 1970 and that time_t is an integral type,
we don't need to add static assertions here.