Skip to content

Commit 16f09f2

Browse files
filmilcommit-bot@chromium.org
authored andcommitted
[vm] Replaces fuchsia.deprecatedtimezone
(prior attempt was rolled back as it caused downstream tests to time out. See prior attempt at: See: https://dart-review.googlesource.com/c/sdk/+/149206) The FIDL library fuchsia.deprecatedtimezone is going away. There are different and better ways to obtain the same functionality. This change removes the dependency on fuchsia.deprecatedtimezone from the Dart SDK. Adds inspect metrics that allow whitebox testing of the runners. Here's a sample `fx iquery` excerpt from a running device, showing both a dart and a flutter runner exposing the same OS diagnostic metrics. ``` /hub/c/dart_jit_runner.cmx/70981/out/diagnostics: /hub/c/dart_jit_runner.cmx/70981/out/diagnostics#os: dst_status = 0 get_profile_status = 0 timezone_content_status = 0 tz_data_close_status = 0 tz_data_status = 0 /hub/c/flutter_jit_runner.cmx/29567/out/diagnostics: /hub/c/flutter_jit_runner.cmx/29567/out/diagnostics#os: dst_status = 0 get_profile_status = 0 timezone_content_status = 0 tz_data_close_status = 0 tz_data_status = 0 ``` Under nominal operation, all of the above values should be equal to 0. Nonzero values indicate an error. This functionality is guarded by Fuchsia integration tests at //src/tests/intl. Tested: (compile locally for Fuchsia and deploy) fx test //src/tests/intl See: - #42245 - #39650 Fixes #39650 Change-Id: I97f6e17e57000f6eec71246aee670bca65b7e1d1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150662 Commit-Queue: Filip Filmar <[email protected]> Reviewed-by: Martin Kustermann <[email protected]>
1 parent 94fcf22 commit 16f09f2

File tree

2 files changed

+103
-28
lines changed

2 files changed

+103
-28
lines changed

runtime/vm/BUILD.gn

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ library_for_all_configs("libdart_vm") {
7474
if (is_fuchsia) {
7575
if (using_fuchsia_gn_sdk) {
7676
extra_deps = [
77-
"$fuchsia_sdk_root/fidl/fuchsia.deprecatedtimezone",
77+
"$fuchsia_sdk_root/fidl/fuchsia.intl",
7878
"$fuchsia_sdk_root/pkg/inspect",
7979
"$fuchsia_sdk_root/pkg/inspect_service_cpp",
8080
"$fuchsia_sdk_root/pkg/sys_cpp",
@@ -83,7 +83,7 @@ library_for_all_configs("libdart_vm") {
8383
]
8484
} else if (using_fuchsia_sdk) {
8585
extra_deps = [
86-
"$fuchsia_sdk_root/fidl:fuchsia.deprecatedtimezone",
86+
"$fuchsia_sdk_root/fidl:fuchsia.intl",
8787
"$fuchsia_sdk_root/pkg:inspect",
8888
"$fuchsia_sdk_root/pkg:inspect_service_cpp",
8989
"$fuchsia_sdk_root/pkg:sys_cpp",
@@ -92,9 +92,7 @@ library_for_all_configs("libdart_vm") {
9292
]
9393
} else {
9494
extra_deps = [
95-
# TODO(US-399): Remove time_service specific code when it is no longer
96-
# necessary.
97-
"//sdk/fidl/fuchsia.deprecatedtimezone",
95+
"//sdk/fidl/fuchsia.intl",
9896
"//sdk/lib/sys/cpp",
9997
"//sdk/lib/sys/inspect/cpp",
10098
"//zircon/public/lib/fbl",

runtime/vm/os_fuchsia.cc

Lines changed: 100 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#include <fcntl.h>
1212
#include <stdint.h>
1313

14-
#include <fuchsia/deprecatedtimezone/cpp/fidl.h>
14+
#include <fuchsia/intl/cpp/fidl.h>
1515
#include <lib/inspect/cpp/inspect.h>
1616
#include <lib/sys/cpp/component_context.h>
1717
#include <lib/sys/cpp/service_directory.h>
@@ -21,17 +21,31 @@
2121
#include <zircon/syscalls/object.h>
2222
#include <zircon/types.h>
2323

24+
#include "third_party/icu/source/common/unicode/errorcode.h"
25+
#include "third_party/icu/source/i18n/unicode/timezone.h"
26+
2427
#include "platform/assert.h"
28+
#include "platform/syslog.h"
2529
#include "platform/utils.h"
2630
#include "vm/zone.h"
2731

2832
namespace {
2933

34+
static constexpr int32_t kMsPerSec = 1000;
35+
3036
// The data directory containing ICU timezone data files.
3137
static constexpr char kICUTZDataDir[] = "/config/data/tzdata/icu/44/le";
3238

39+
// This is the general OK status.
40+
static constexpr int32_t kOk = 0;
41+
42+
// This status means that the error code is not initialized yet ("set" was not
43+
// yet called). Error codes are usually either 0 (kOk), or negative.
44+
static constexpr int32_t kUninitialized = 1;
45+
3346
// The status codes for tzdata file open and read.
3447
enum class TZDataStatus {
48+
// The operation completed without error.
3549
OK = 0,
3650
// The open call for the tzdata file did not succeed.
3751
COULD_NOT_OPEN = -1,
@@ -41,16 +55,23 @@ enum class TZDataStatus {
4155

4256
// Adds a facility for introspecting timezone data errors. Allows insight into
4357
// the internal state of the VM even if error reporting facilities fail.
58+
//
59+
// Under normal operation, all metric values below should be zero.
4460
class InspectMetrics {
4561
public:
4662
// Does not take ownership of inspector.
4763
explicit InspectMetrics(inspect::Inspector* inspector)
4864
: inspector_(inspector),
4965
root_(inspector_->GetRoot()),
5066
metrics_(root_.CreateChild("os")),
51-
dst_status_(metrics_.CreateInt("dst_status", 0)),
52-
tz_data_status_(metrics_.CreateInt("tz_data_status", 0)),
53-
tz_data_close_status_(metrics_.CreateInt("tz_data_close_status", 0)) {}
67+
dst_status_(metrics_.CreateInt("dst_status", kUninitialized)),
68+
tz_data_status_(metrics_.CreateInt("tz_data_status", kUninitialized)),
69+
tz_data_close_status_(
70+
metrics_.CreateInt("tz_data_close_status", kUninitialized)),
71+
get_profile_status_(
72+
metrics_.CreateInt("get_profile_status", kUninitialized)),
73+
profiles_timezone_content_status_(
74+
metrics_.CreateInt("timezone_content_status", kOk)) {}
5475

5576
// Sets the last status code for DST offset calls.
5677
void SetDSTOffsetStatus(zx_status_t status) {
@@ -64,6 +85,17 @@ class InspectMetrics {
6485
tz_data_close_status_.Set(status);
6586
}
6687

88+
// Sets the last status code for the call to PropertyProvider::GetProfile.
89+
void SetProfileStatus(zx_status_t status) {
90+
get_profile_status_.Set(static_cast<int32_t>(status));
91+
}
92+
93+
// Sets the last status seen while examining timezones returned from
94+
// PropertyProvider::GetProfile.
95+
void SetTimeZoneContentStatus(zx_status_t status) {
96+
profiles_timezone_content_status_.Set(static_cast<int32_t>(status));
97+
}
98+
6799
private:
68100
// The inspector that all metrics are being reported into.
69101
inspect::Inspector* inspector_;
@@ -82,6 +114,15 @@ class InspectMetrics {
82114

83115
// The return code for the close() call for tzdata files.
84116
inspect::IntProperty tz_data_close_status_;
117+
118+
// The return code of the GetProfile call in GetTimeZoneName. If this is
119+
// nonzero, then os_fuchsia.cc reported a default timezone as a fallback.
120+
inspect::IntProperty get_profile_status_;
121+
122+
// U_ILLEGAL_ARGUMENT_ERROR(=1) if timezones read from ProfileProvider were
123+
// incorrect. Otherwise 0. If this metric reports U_ILLEGAL_ARGUMENT_ERROR,
124+
// the os_fuchsia.cc module reported a default timezone as a fallback.
125+
inspect::IntProperty profiles_timezone_content_status_;
85126
};
86127

87128
// Initialized on OS:Init(), deinitialized on OS::Cleanup.
@@ -132,50 +173,85 @@ intptr_t OS::ProcessId() {
132173
return static_cast<intptr_t>(getpid());
133174
}
134175

176+
// This is the default timezone returned if it could not be obtained. For
177+
// Fuchsia, the default device timezone is always UTC.
178+
static const char kDefaultTimezone[] = "UTC";
179+
135180
// TODO(FL-98): Change this to talk to fuchsia.dart to get timezone service to
136181
// directly get timezone.
137182
//
138183
// Putting this hack right now due to CP-120 as I need to remove
139184
// component:ConnectToEnvironmentServices and this is the only thing that is
140185
// blocking it and FL-98 will take time.
141-
static fuchsia::deprecatedtimezone::TimezoneSyncPtr tz;
186+
static fuchsia::intl::PropertyProviderSyncPtr property_provider;
142187

143188
static zx_status_t GetLocalAndDstOffsetInSeconds(int64_t seconds_since_epoch,
144189
int32_t* local_offset,
145190
int32_t* dst_offset) {
146-
zx_status_t status = tz->GetTimezoneOffsetMinutes(seconds_since_epoch * 1000,
147-
local_offset, dst_offset);
148-
metrics->SetDSTOffsetStatus(status);
149-
if (status != ZX_OK) {
150-
return status;
191+
const char* timezone_id = OS::GetTimeZoneName(seconds_since_epoch);
192+
std::unique_ptr<icu::TimeZone> timezone(
193+
icu::TimeZone::createTimeZone(timezone_id));
194+
UErrorCode error = U_ZERO_ERROR;
195+
const auto ms_since_epoch =
196+
static_cast<UDate>(kMsPerSec * seconds_since_epoch);
197+
// The units of time that local_offset and dst_offset are returned from this
198+
// function is, usefully, not documented, but it seems that the units are
199+
// milliseconds. Add these variables here for clarity.
200+
int32_t local_offset_ms = 0;
201+
int32_t dst_offset_ms = 0;
202+
timezone->getOffset(ms_since_epoch, /*local_time=*/false, local_offset_ms,
203+
dst_offset_ms, error);
204+
metrics->SetDSTOffsetStatus(error);
205+
if (error != U_ZERO_ERROR) {
206+
icu::ErrorCode icu_error;
207+
icu_error.set(error);
208+
Syslog::PrintErr("could not get DST offset: %s\n", icu_error.errorName());
209+
return ZX_ERR_INTERNAL;
151210
}
152-
*local_offset *= 60;
153-
*dst_offset *= 60;
211+
// We must return offset in seconds, so convert.
212+
*local_offset = local_offset_ms / kMsPerSec;
213+
*dst_offset = dst_offset_ms / kMsPerSec;
154214
return ZX_OK;
155215
}
156216

157217
const char* OS::GetTimeZoneName(int64_t seconds_since_epoch) {
158218
// TODO(abarth): Handle time zone changes.
159-
static const auto* tz_name = new std::string([] {
160-
std::string result;
161-
tz->GetTimezoneId(&result);
162-
return result;
163-
}());
219+
static const std::unique_ptr<std::string> tz_name =
220+
std::make_unique<std::string>([]() -> std::string {
221+
fuchsia::intl::Profile profile;
222+
const zx_status_t status = property_provider->GetProfile(&profile);
223+
metrics->SetProfileStatus(status);
224+
if (status != ZX_OK) {
225+
return kDefaultTimezone;
226+
}
227+
const std::vector<fuchsia::intl::TimeZoneId>& timezones =
228+
profile.time_zones();
229+
if (timezones.empty()) {
230+
metrics->SetTimeZoneContentStatus(U_ILLEGAL_ARGUMENT_ERROR);
231+
// Empty timezone array is not up to fuchsia::intl spec. The serving
232+
// endpoint is broken and should be fixed.
233+
Syslog::PrintErr("got empty timezone value\n");
234+
return kDefaultTimezone;
235+
}
236+
return timezones[0].id;
237+
}());
164238
return tz_name->c_str();
165239
}
166240

167241
int OS::GetTimeZoneOffsetInSeconds(int64_t seconds_since_epoch) {
168-
int32_t local_offset, dst_offset;
169-
zx_status_t status = GetLocalAndDstOffsetInSeconds(
242+
int32_t local_offset = 0;
243+
int32_t dst_offset = 0;
244+
const zx_status_t status = GetLocalAndDstOffsetInSeconds(
170245
seconds_since_epoch, &local_offset, &dst_offset);
171246
return status == ZX_OK ? local_offset + dst_offset : 0;
172247
}
173248

174249
int OS::GetLocalTimeZoneAdjustmentInSeconds() {
175-
int32_t local_offset, dst_offset;
176250
zx_time_t now = 0;
177251
zx_clock_get(ZX_CLOCK_UTC, &now);
178-
zx_status_t status = GetLocalAndDstOffsetInSeconds(
252+
int32_t local_offset = 0;
253+
int32_t dst_offset = 0;
254+
const zx_status_t status = GetLocalAndDstOffsetInSeconds(
179255
now / ZX_SEC(1), &local_offset, &dst_offset);
180256
return status == ZX_OK ? local_offset : 0;
181257
}
@@ -199,7 +275,7 @@ int64_t OS::GetCurrentMonotonicFrequency() {
199275
}
200276

201277
int64_t OS::GetCurrentMonotonicMicros() {
202-
int64_t ticks = GetCurrentMonotonicTicks();
278+
const int64_t ticks = GetCurrentMonotonicTicks();
203279
ASSERT(GetCurrentMonotonicFrequency() == kNanosecondsPerSecond);
204280
return ticks / kNanosecondsPerMicrosecond;
205281
}
@@ -347,7 +423,8 @@ void OS::Init() {
347423
metrics = std::make_unique<InspectMetrics>(component_inspector->inspector());
348424

349425
InitializeTZData();
350-
context->svc()->Connect(tz.NewRequest());
426+
auto services = sys::ServiceDirectory::CreateFromNamespace();
427+
services->Connect(property_provider.NewRequest());
351428
}
352429

353430
void OS::Cleanup() {

0 commit comments

Comments
 (0)