Skip to content

Commit

Permalink
Switch Mac to use time_exploded_posix.cc.
Browse files Browse the repository at this point in the history
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: I2cf030fd4d3ed8da87e41bc8ba3dc3b1b99e8f6d
Reviewed-on: https://chromium-review.googlesource.com/754079
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515062}
  • Loading branch information
asvitkine-chromium authored and Commit Bot committed Nov 9, 2017
1 parent 6c4f0cf commit dcb7897
Show file tree
Hide file tree
Showing 9 changed files with 93 additions and 62 deletions.
1 change: 1 addition & 0 deletions base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1515,6 +1515,7 @@ 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
25 changes: 25 additions & 0 deletions base/time/time.h
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,31 @@ 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_MACOSX) && !defined(OS_IOS)
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();
#elif defined(OS_POSIX) && !defined(__LP64__) && !defined(OS_IOS)
static constexpr int kExplodedMinYear = 1970;
static constexpr int kExplodedMaxYear = 2037;
#else
static constexpr int kExplodedMinYear = std::numeric_limits<int>::min();
static constexpr int kExplodedMaxYear = 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: 1 addition & 3 deletions base/time/time_exploded_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,8 @@
#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)
#error "This implementation is for POSIX platforms other than Mac."
static_assert(sizeof(time_t) >= 8, "Y2038 problem!");
#endif

namespace {
Expand Down
8 changes: 8 additions & 0 deletions base/time/time_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,13 @@ 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 @@ -258,6 +265,7 @@ void Time::Explode(bool is_local, Exploded* exploded) const {
(microsecond - kMicrosecondsPerMillisecond + 1) /
kMicrosecondsPerMillisecond;
}
#endif // OS_IOS

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

Expand Down
35 changes: 35 additions & 0 deletions base/time/time_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,41 @@ 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;
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: 13 additions & 19 deletions net/cert/x509_certificate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,25 @@ 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;

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

// 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;
}
// TODO(mattm): consider consolidating this with
// SaturatedTimeFromUTCExploded from cookie_util.cc
if (generalized.year > base::Time::kExplodedMaxYear) {
*result = base::Time::Max();
return true;
}
if (generalized.year < base::Time::kExplodedMinYear) {
*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: 2 additions & 22 deletions net/cookies/cookie_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,36 +54,16 @@ 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;
}

// 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) {
if (exploded.year > base::Time::kExplodedMaxYear) {
*out = base::Time::Max();
return true;
}
#endif // defined(OS_POSIX) && !defined(OS_MACOSX)

#if defined(OS_WIN)
// Allow dates prior to Windows epoch.
if (exploded.year < 1601) {
if (exploded.year < base::Time::kExplodedMinYear) {
*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[] = {
// 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",
// 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",
};

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

// 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());
EXPECT_LE(parsed_time, base::Time::UnixEpoch()) << test;
}
}

Expand Down
10 changes: 0 additions & 10 deletions net/socket/ssl_client_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1346,17 +1346,7 @@ 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 dcb7897

Please sign in to comment.