Add: check exploded time is properly converted

base/time/time_mac.cc

Index: base/time/time_mac.cc
diff --git a/base/time/time_mac.cc b/base/time/time_mac.cc
index c23c4917e757f492ab7a6cc151158a55901878ef..82318ab36f6ee96022843bd3efed0d4857f7814a 100644
--- a/base/time/time_mac.cc
+++ b/base/time/time_mac.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -184,8 +184,24 @@ Time Time::FromExploded(bool is_local, const Exploded& exploded) {
       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 {