Skip to content

Commit

Permalink
Revert "Re-land "Switch Mac to use time_exploded_posix.cc.""
Browse files Browse the repository at this point in the history
This reverts commit af2ab97.

Reason for revert:

This breaks the "ios-simulator" trybot on my CL. See https://chromium-review.googlesource.com/c/chromium/src/+/590273 and failed trybots runs at

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337162
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337212

which has TimeTest.TimeT/0 failing:
TimeTest.TimeT:
../../base/time/time_unittest.cc:134: Failure
      Expected: tms.tm_hour
      Which is: 14
To be equal to: exploded.hour
      Which is: 22


Original change's description:
> Re-land "Switch Mac to use time_exploded_posix.cc."
> 
> Original review:
> https://chromium-review.googlesource.com/c/chromium/src/+/754079
> Reverted here:
> https://chromium-review.googlesource.com/c/chromium/src/+/760116
> 
> Previously, we would use a CoreFoundation-based version of these
> functions implemented in time_mac.cc because time_t on Mac was
> capped at year 2038. However, with 64-bit that is no longer the
> case and we can just use time_exploded_posix.cc.
> 
> From UMA sampling profiler data (see bug), we know that the
> CoreFoundation versions of these functions are slow and currently
> account for around 80ms cost during startup. After this change
> is submitted and makes it to Canary, we can see how the POSIX
> versions of these functions perform - hopefully much faster than
> the CoreFoundation ones.
> 
> Some code in net/ had assumptions around the min and max years
> that are supported on different platforms by these functions. To
> make that logic a bit cleaner, this CL introduces two constants
> in time.h specifying the platform-specific limits - so that
> downstream code like net can use them instead of baking their
> own assumptions. Some net error codes are changing as a result
> of this clean up - which can be seen in the updated net tests.
> 
> BUG=781601
> 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
> Change-Id: I5d10a06dd5b0073bfd943f58af08f67980227d4a
> Reviewed-on: https://chromium-review.googlesource.com/760443
> Reviewed-by: Matt Mueller <mattm@chromium.org>
> Reviewed-by: Yuri Wiitala <miu@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#515370}

TBR=miu@chromium.org,mattm@chromium.org,asvitkine@chromium.org,mmenke@chromium.org,mark@chromium.org

Change-Id: I4a582e11d4fd8490cf22cc37d4d746b4c3731957
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 781601
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/764868
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515727}
  • Loading branch information
Avi Drissman authored and Commit Bot committed Nov 10, 2017
1 parent adda6e1 commit ac4c8a2
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 101 deletions.
1 change: 0 additions & 1 deletion base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,6 @@ jumbo_component("base") {
"mac/scoped_typeref.h",
"power_monitor/power_monitor_device_source_mac.mm",
"time/time_conversion_posix.cc",
"time/time_exploded_posix.cc",
"time/time_mac.cc",
]

Expand Down
27 changes: 0 additions & 27 deletions base/time/time.h
Original file line number Diff line number Diff line change
Expand Up @@ -460,33 +460,6 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
static constexpr int64_t kQPCOverflowThreshold = INT64_C(0x8637BD05AF7);
#endif

// kExplodedMinYear and kExplodedMaxYear define the platform-specific limits
// for values passed to FromUTCExploded() and FromLocalExploded(). Those
// functions will return false if passed values outside these limits. The limits
// are inclusive, meaning that the API should support all dates within a given
// limit year.
#if defined(OS_WIN)
static constexpr int kExplodedMinYear = 1601;
static constexpr int kExplodedMaxYear = 30827;
#elif defined(OS_IOS)
static constexpr int kExplodedMinYear = std::numeric_limits<int>::min();
static constexpr int kExplodedMaxYear = std::numeric_limits<int>::max();
#elif defined(OS_MACOSX)
static constexpr int kExplodedMinYear = 1902;
static constexpr int kExplodedMaxYear = std::numeric_limits<int>::max();
#elif defined(OS_ANDROID)
// Though we use 64-bit time APIs on both 32 and 64 bit Android, some OS
// versions like KitKat (ARM but not x86 emulator) can't handle some early
// dates (e.g. before 1170). So we set min conservatively here.
static constexpr int kExplodedMinYear = 1902;
static constexpr int kExplodedMaxYear = std::numeric_limits<int>::max();
#else
static constexpr int kExplodedMinYear =
(sizeof(time_t) == 4 ? 1902 : std::numeric_limits<int>::min());
static constexpr int kExplodedMaxYear =
(sizeof(time_t) == 4 ? 2037 : std::numeric_limits<int>::max());
#endif

// Represents an exploded time that can be formatted nicely. This is kind of
// like the Win32 SYSTEMTIME structure or the Unix "struct tm" with a few
// additions and changes to prevent errors.
Expand Down
4 changes: 3 additions & 1 deletion base/time/time_exploded_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
#include "base/os_compat_nacl.h"
#endif

// Ensure the Mac build does not include this module. Instead, non-POSIX
// implementation is used to support Time::Exploded.
#if defined(OS_MACOSX)
static_assert(sizeof(time_t) >= 8, "Y2038 problem!");
#error "This implementation is for POSIX platforms other than Mac."
#endif

namespace {
Expand Down
8 changes: 0 additions & 8 deletions base/time/time_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,6 @@ Time Time::NowFromSystemTime() {
return Now();
}

// Note: These implementations of Time::FromExploded() and Time::Explode() are
// only used on iOS now. Since Mac is now always 64-bit, we can use the POSIX
// versions of these functions as time_t is not capped at year 2038 on 64-bit
// builds. The POSIX functions are preferred since they don't suffer from some
// performance problems that are present in these implementations.
// See crbug.com/781601 for more details.
#if defined(OS_IOS)
// static
bool Time::FromExploded(bool is_local, const Exploded& exploded, Time* time) {
base::ScopedCFTypeRef<CFTimeZoneRef> time_zone(
Expand Down Expand Up @@ -265,7 +258,6 @@ void Time::Explode(bool is_local, Exploded* exploded) const {
(microsecond - kMicrosecondsPerMillisecond + 1) /
kMicrosecondsPerMillisecond;
}
#endif // OS_IOS

// TimeTicks ------------------------------------------------------------------

Expand Down
41 changes: 0 additions & 41 deletions base/time/time_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,47 +631,6 @@ TEST_F(TimeTest, FromLocalExplodedCrashOnAndroid) {
}
#endif // OS_ANDROID

TEST_F(TimeTest, FromExploded_MinMax) {
Time::Exploded exploded = {0};
exploded.month = 1;
exploded.day_of_month = 1;

Time parsed_time;

if (Time::kExplodedMinYear != std::numeric_limits<int>::min()) {
exploded.year = Time::kExplodedMinYear;
EXPECT_TRUE(Time::FromUTCExploded(exploded, &parsed_time));
#if !defined(OS_WIN)
// On Windows, January 1, 1601 00:00:00 is actually the null time.
EXPECT_FALSE(parsed_time.is_null());
#endif

#if !defined(OS_ANDROID)
// The dates earlier than |kExplodedMinYear| that don't work are OS version
// dependent on Android.
exploded.year--;
EXPECT_FALSE(Time::FromUTCExploded(exploded, &parsed_time));
EXPECT_TRUE(parsed_time.is_null());
#endif
}

if (Time::kExplodedMaxYear != std::numeric_limits<int>::max()) {
exploded.year = Time::kExplodedMaxYear;
exploded.month = 12;
exploded.day_of_month = 31;
exploded.hour = 23;
exploded.minute = 59;
exploded.second = 59;
exploded.millisecond = 999;
EXPECT_TRUE(Time::FromUTCExploded(exploded, &parsed_time));
EXPECT_FALSE(parsed_time.is_null());

exploded.year++;
EXPECT_FALSE(Time::FromUTCExploded(exploded, &parsed_time));
EXPECT_TRUE(parsed_time.is_null());
}
}

TEST(TimeTicks, Deltas) {
for (int index = 0; index < 50; index++) {
TimeTicks ticks_start = TimeTicks::Now();
Expand Down
32 changes: 19 additions & 13 deletions net/cert/x509_certificate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,25 +87,31 @@ bool GeneralizedTimeToBaseTime(const der::GeneralizedTime& generalized,
exploded.hour = generalized.hours;
exploded.minute = generalized.minutes;
exploded.second = generalized.seconds;

#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
if (base::Time::FromUTCExploded(exploded, result))
return true;

// Fail on obviously bad dates.
if (!exploded.HasValidValues())
return false;
if (sizeof(time_t) == 4) {
// Fail on obviously bad dates before trying the 32-bit hacks.
if (!exploded.HasValidValues())
return false;

// TODO(mattm): consider consolidating this with
// SaturatedTimeFromUTCExploded from cookie_util.cc
if (static_cast<int>(generalized.year) > base::Time::kExplodedMaxYear) {
*result = base::Time::Max();
return true;
}
if (static_cast<int>(generalized.year) < base::Time::kExplodedMinYear) {
*result = base::Time::Min();
return true;
// Hack to handle dates that can't be converted on 32-bit systems.
// TODO(mattm): consider consolidating this with
// SaturatedTimeFromUTCExploded from cookie_util.cc
if (generalized.year >= 2038) {
*result = base::Time::Max();
return true;
}
if (generalized.year < 1970) {
*result = base::Time::Min();
return true;
}
}
return false;
#else
return base::Time::FromUTCExploded(exploded, result);
#endif
}

// Sets |value| to the Value from a DER Sequence Tag-Length-Value and return
Expand Down
24 changes: 22 additions & 2 deletions net/cookies/cookie_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,36 @@ bool SaturatedTimeFromUTCExploded(const base::Time::Exploded& exploded,
// some invalid calendar dates in the out-of-range case.
if (!exploded.HasValidValues())
return false;
#if defined(OS_POSIX) && !defined(OS_MACOSX)
// Allow dates prior to unix epoch (which fail on non-Mac/iOS POSIX).
if (exploded.year < 1970) {
*out = MinNonNullTime();
return true;
}

if (exploded.year > base::Time::kExplodedMaxYear) {
// On 32-bit non-Mac/iOS POSIX systems, the time_t value that FromExploded()
// returns overflows in the middle of year 2038. In that case, return
// Time::Max().
if (sizeof(time_t) == 4u && exploded.year >= 2038) {
*out = base::Time::Max();
return true;
}
if (exploded.year < base::Time::kExplodedMinYear) {
#endif // defined(OS_POSIX) && !defined(OS_MACOSX)

#if defined(OS_WIN)
// Allow dates prior to Windows epoch.
if (exploded.year < 1601) {
*out = MinNonNullTime();
return true;
}

// Allow dates after the Windows epoch.
if (exploded.year >= 30827) {
*out = base::Time::Max();
return true;
}
#endif // defined(OS_WIN)

return false;
}

Expand Down
16 changes: 8 additions & 8 deletions net/cookies/cookie_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -171,21 +171,21 @@ TEST(CookieUtilTest, ParseCookieExpirationTimeBeyond2038) {
// succeed anyway (and return a minimal base::Time).
TEST(CookieUtilTest, ParseCookieExpirationTimeBefore1970) {
const char* kTests[] = {
// Times around the Unix epoch.
"1970 Jan 1 00:00:00", "1969 March 3 21:01:22",
// Times around the Windows epoch.
"1601 Jan 1 00:00:00", "1600 April 15 21:01:22",
// Times around kExplodedMinYear on Mac.
"1902 Jan 1 00:00:00", "1901 Jan 1 00:00:00",
// The unix epoch.
"1970 Jan 1 00:00:00",
// The windows epoch.
"1601 Jan 1 00:00:00",
// Other dates.
"1969 March 3 21:01:22", "1600 April 15 21:01:22",
};

for (auto* test : kTests) {
base::Time parsed_time = cookie_util::ParseCookieExpirationTime(test);
EXPECT_FALSE(parsed_time.is_null()) << test;
EXPECT_FALSE(parsed_time.is_null());

// It should either have an exact value, or should be base::Time(1)
// For simplicity just check that it is less than the unix epoch.
EXPECT_LE(parsed_time, base::Time::UnixEpoch()) << test;
EXPECT_LE(parsed_time, base::Time::UnixEpoch());
}
}

Expand Down
10 changes: 10 additions & 0 deletions net/socket/ssl_client_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1346,7 +1346,17 @@ TEST_F(SSLClientSocketTest, ConnectBadValidity) {
SSLConfig ssl_config;
int rv;
ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));

#if defined(OS_WIN)
EXPECT_THAT(rv, IsError(ERR_SSL_SERVER_CERT_BAD_FORMAT));
EXPECT_FALSE(IsCertificateError(rv));
#elif defined(OS_ANDROID)
// Android date handling behavior can vary depending on the platform.
EXPECT_THAT(rv, AnyOf(IsError(ERR_SSL_SERVER_CERT_BAD_FORMAT),
IsError(ERR_CERT_DATE_INVALID)));
#else // !(defined(OS_WIN) || defined(OS_ANDROID))
EXPECT_THAT(rv, IsError(ERR_CERT_DATE_INVALID));
#endif
}

// Attempt to connect to a page which requests a client certificate. It should
Expand Down

0 comments on commit ac4c8a2

Please sign in to comment.