Add: check exploded time is properly converted

base/time/time_mac.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/time/time.h"
 
 #include <CoreFoundation/CFDate.h>
 #include <CoreFoundation/CFTimeZone.h>
 #include <mach/mach.h>
 #include <mach/mach_time.h>
 #include <stddef.h>
@@ skipping 165 matching lines @@
       kCFAllocatorDefault, kCFGregorianCalendar));
   CFCalendarSetTimeZone(gregorian, time_zone);
   CFAbsoluteTime absolute_time;
   // 'S' is not defined in componentDesc in Apple documentation, but can be
   // found at http://www.opensource.apple.com/source/CF/CF-855.17/CFCalendar.c
   CFCalendarComposeAbsoluteTime(
       gregorian, &absolute_time, "yMdHmsS", exploded.year, exploded.month,
       exploded.day_of_month, exploded.hour, exploded.minute, exploded.second,
       exploded.millisecond);
   CFAbsoluteTime seconds = absolute_time + kCFAbsoluteTimeIntervalSince1970;
-  return Time(static_cast<int64_t>(seconds * kMicrosecondsPerSecond) +
-              kWindowsEpochDeltaMicroseconds);
+
+  base::Time t_time =
+      Time(static_cast<int64_t>(seconds * kMicrosecondsPerSecond) +
+           kWindowsEpochDeltaMicroseconds);
+
+  // If |exploded.day_of_month| is set to 31
+  // on a 28-30 day month, it will return the first day of the next month.

[mmenke, 2016/05/18 17:32:00]

We should figure out what platforms this happens on, and only have this extra
logic on those platforms.  How do we figure that out?  Simplest thing to do
would be for you to upload a CL with your unit test, but without these fixes,
and I'll run it through the trybots, and we can see which ones fail.

[eroman, 2016/05/18 19:35:17]

In the case of our _posix implementation, the specification for mktime() is
surprisingly ambiguous on this issue.

In practice though you can pass out of range values and it will do the
arithmetic (for instance Feb -1 becomes Jan 31).

This is a notable difference from the Windows implementation.

So yeah, matching the lowest-common denominator here is good.

[mmenke, 2016/05/18 20:11:32]

That's a good point...I was mostly thinking (hoping?) OSX and Windows would at
least either consistently be good or consistently be bad...but that may be
hoping for too much.

Having duplicate code on each platform to check the time seems ugly, though not
sure it's worth the effort of avoiding.

[maksims (do not use this acc), 2016/05/19 09:57:20]

What's then? Can you run trybots to see what environments fail to execute
FromExplodedOutOfBoundsTimeOld?

+  // Thus round-trip the time and compare the initial |exploded| with
+  // |utc_to_exploded| time.
+  // If they are not same, return Time(0).
+  // Windows and Posix implementations return Time(0) on failure.

[mmenke, 2016/05/18 17:32:00]

These last two sentences seem not to be needed.

[maksims (do not use this acc), 2016/05/19 09:57:20]

Done.

+  base::Time::Exploded to_exploded = {0};

[Mark Mentovai, 2016/05/18 17:42:49]

No need to waste time {0}-initializing.

[maksims (do not use this acc), 2016/05/19 09:57:20]

Done.

+  if (!is_local)
+    t_time.UTCExplode(&to_exploded);
+  else
+    t_time.LocalExplode(&to_exploded);
+
+  return to_exploded != exploded ? Time(0) : t_time;
 }
 
 void Time::Explode(bool is_local, Exploded* exploded) const {
   // Avoid rounding issues, by only putting the integral number of seconds
   // (rounded towards -infinity) into a |CFAbsoluteTime| (which is a |double|).
   int64_t microsecond = us_ % kMicrosecondsPerSecond;
   if (microsecond < 0)
     microsecond += kMicrosecondsPerSecond;
   CFAbsoluteTime seconds = ((us_ - microsecond) / kMicrosecondsPerSecond) -
                            kWindowsEpochDeltaSeconds -
@@ skipping 47 matching lines @@
   return Clock::MAC_MACH_ABSOLUTE_TIME;
 #endif  // defined(OS_IOS)
 }
 
 // static
 ThreadTicks ThreadTicks::Now() {
   return ThreadTicks(ComputeThreadTicks());
 }
 
 }  // namespace base