From df4e79d73108be0d7f02d82c17923df6118dc409 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 2 Feb 2021 09:12:55 -0800 Subject: [PATCH] Avoid calling Dart timeline APIs during Dart_Cleanup (#24007) The Dart timeline is not thread safe if an engine thread that is not controlled by Dart calls Dart_TimelineEvent while another thread is calling Dart_Cleanup. Fixes https://github.com/flutter/flutter/issues/74134 --- ci/licenses_golden/licenses_flutter | 2 + fml/trace_event.cc | 13 ++++-- fml/trace_event.h | 12 +++++ runtime/BUILD.gn | 2 + runtime/dart_vm.cc | 14 ++---- runtime/dart_vm_initializer.cc | 71 +++++++++++++++++++++++++++++ runtime/dart_vm_initializer.h | 26 +++++++++++ runtime/dart_vm_lifecycle.h | 1 + 8 files changed, 127 insertions(+), 14 deletions(-) create mode 100644 runtime/dart_vm_initializer.cc create mode 100644 runtime/dart_vm_initializer.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 12d0d31cbf6..e36f03db176 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -597,6 +597,8 @@ FILE: ../../../flutter/runtime/dart_vm.cc FILE: ../../../flutter/runtime/dart_vm.h FILE: ../../../flutter/runtime/dart_vm_data.cc FILE: ../../../flutter/runtime/dart_vm_data.h +FILE: ../../../flutter/runtime/dart_vm_initializer.cc +FILE: ../../../flutter/runtime/dart_vm_initializer.h FILE: ../../../flutter/runtime/dart_vm_lifecycle.cc FILE: ../../../flutter/runtime/dart_vm_lifecycle.h FILE: ../../../flutter/runtime/dart_vm_unittests.cc diff --git a/fml/trace_event.cc b/fml/trace_event.cc index bb2becbd036..8ee1bed5905 100644 --- a/fml/trace_event.cc +++ b/fml/trace_event.cc @@ -19,6 +19,7 @@ namespace tracing { namespace { AsciiTrie gAllowlist; +TimelineEventHandler gTimelineEventHandler; inline void FlutterTimelineEvent(const char* label, int64_t timestamp0, @@ -27,9 +28,9 @@ inline void FlutterTimelineEvent(const char* label, intptr_t argument_count, const char** argument_names, const char** argument_values) { - if (gAllowlist.Query(label)) { - Dart_TimelineEvent(label, timestamp0, timestamp1_or_async_id, type, - argument_count, argument_names, argument_values); + if (gTimelineEventHandler && gAllowlist.Query(label)) { + gTimelineEventHandler(label, timestamp0, timestamp1_or_async_id, type, + argument_count, argument_names, argument_values); } } } // namespace @@ -38,6 +39,10 @@ void TraceSetAllowlist(const std::vector& allowlist) { gAllowlist.Fill(allowlist); } +void TraceSetTimelineEventHandler(TimelineEventHandler handler) { + gTimelineEventHandler = handler; +} + size_t TraceNonce() { static std::atomic_size_t gLastItem; return ++gLastItem; @@ -288,6 +293,8 @@ void TraceEventFlowEnd0(TraceArg category_group, TraceArg name, TraceIDArg id) { void TraceSetAllowlist(const std::vector& allowlist) {} +void TraceSetTimelineEventHandler(TimelineEventHandler handler) {} + size_t TraceNonce() { return 0; } diff --git a/fml/trace_event.h b/fml/trace_event.h index 3928d68c7d2..a030cce117e 100644 --- a/fml/trace_event.h +++ b/fml/trace_event.h @@ -5,6 +5,8 @@ #ifndef FLUTTER_FML_TRACE_EVENT_H_ #define FLUTTER_FML_TRACE_EVENT_H_ +#include + #include "flutter/fml/build_config.h" #if defined(OS_FUCHSIA) @@ -145,6 +147,16 @@ using TraceIDArg = int64_t; void TraceSetAllowlist(const std::vector& allowlist); +using TimelineEventHandler = std::function; + +void TraceSetTimelineEventHandler(TimelineEventHandler handler); + void TraceTimelineEvent(TraceArg category_group, TraceArg name, int64_t timestamp_micros, diff --git a/runtime/BUILD.gn b/runtime/BUILD.gn index 9788794f7da..074c040a721 100644 --- a/runtime/BUILD.gn +++ b/runtime/BUILD.gn @@ -49,6 +49,8 @@ source_set("runtime") { "dart_vm.h", "dart_vm_data.cc", "dart_vm_data.h", + "dart_vm_initializer.cc", + "dart_vm_initializer.h", "dart_vm_lifecycle.cc", "dart_vm_lifecycle.h", "embedder_resources.cc", diff --git a/runtime/dart_vm.cc b/runtime/dart_vm.cc index 331dfb82e2a..ba6dc9dad9e 100644 --- a/runtime/dart_vm.cc +++ b/runtime/dart_vm.cc @@ -24,6 +24,7 @@ #include "flutter/lib/ui/dart_ui.h" #include "flutter/runtime/dart_isolate.h" #include "flutter/runtime/dart_service_isolate.h" +#include "flutter/runtime/dart_vm_initializer.h" #include "flutter/runtime/ptrace_check.h" #include "third_party/dart/runtime/include/bin/dart_io_api.h" #include "third_party/skia/include/core/SkExecutor.h" @@ -437,11 +438,7 @@ DartVM::DartVM(std::shared_ptr vm_data, params.thread_exit = ThreadExitCallback; params.get_service_assets = GetVMServiceAssetsArchiveCallback; params.entropy_source = dart::bin::GetEntropy; - char* init_error = Dart_Initialize(¶ms); - if (init_error) { - FML_LOG(FATAL) << "Error while initializing the Dart VM: " << init_error; - ::free(init_error); - } + DartVMInitializer::Initialize(¶ms); // Send the earliest available timestamp in the application lifecycle to // timeline. The difference between this timestamp and the time we render // the very first frame gives us a good idea about Flutter's startup time. @@ -486,14 +483,9 @@ DartVM::~DartVM() { Dart_ExitIsolate(); } - char* result = Dart_Cleanup(); + DartVMInitializer::Cleanup(); dart::bin::CleanupDartIo(); - - FML_CHECK(result == nullptr) - << "Could not cleanly shut down the Dart VM. Error: \"" << result - << "\"."; - free(result); } std::shared_ptr DartVM::GetVMData() const { diff --git a/runtime/dart_vm_initializer.cc b/runtime/dart_vm_initializer.cc new file mode 100644 index 00000000000..5ee62aa2302 --- /dev/null +++ b/runtime/dart_vm_initializer.cc @@ -0,0 +1,71 @@ +// Copyright 2013 The Flutter 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 "flutter/runtime/dart_vm_initializer.h" + +#include + +#include "flutter/fml/logging.h" +#include "flutter/fml/synchronization/shared_mutex.h" +#include "flutter/fml/trace_event.h" + +// Tracks whether Dart has been initialized and if it is safe to call Dart +// APIs. +static std::atomic gDartInitialized; + +void DartVMInitializer::Initialize(Dart_InitializeParams* params) { + FML_DCHECK(!gDartInitialized); + + char* error = Dart_Initialize(params); + if (error) { + FML_LOG(FATAL) << "Error while initializing the Dart VM: " << error; + ::free(error); + } else { + gDartInitialized = true; + } + + fml::tracing::TraceSetTimelineEventHandler(LogDartTimelineEvent); +} + +void DartVMInitializer::Cleanup() { + FML_DCHECK(gDartInitialized); + + // Dart_TimelineEvent is unsafe during a concurrent call to Dart_Cleanup + // because Dart_Cleanup will destroy the timeline recorder. Clear the + // initialized flag so that future calls to LogDartTimelineEvent will not + // call Dart_TimelineEvent. + // + // Note that this is inherently racy. If a thread sees that gDartInitialized + // is set and proceeds to call Dart_TimelineEvent shortly before another + // thread calls Dart_Cleanup, then the Dart_TimelineEvent call may crash + // if Dart_Cleanup deletes the timeline before Dart_TimelineEvent completes. + // In practice this is unlikely because Dart_Cleanup does significant other + // work before deleting the timeline. + // + // The engine can not safely guard Dart_Cleanup and LogDartTimelineEvent with + // a lock due to the risk of deadlocks. Dart_Cleanup waits for various + // Dart-owned threads to shut down. If one of those threads invokes an engine + // callback that calls LogDartTimelineEvent while the Dart_Cleanup thread owns + // the lock, then Dart_Cleanup would deadlock. + gDartInitialized = false; + + char* error = Dart_Cleanup(); + if (error) { + FML_LOG(FATAL) << "Error while cleaning up the Dart VM: " << error; + ::free(error); + } +} + +void DartVMInitializer::LogDartTimelineEvent(const char* label, + int64_t timestamp0, + int64_t timestamp1_or_async_id, + Dart_Timeline_Event_Type type, + intptr_t argument_count, + const char** argument_names, + const char** argument_values) { + if (gDartInitialized) { + Dart_TimelineEvent(label, timestamp0, timestamp1_or_async_id, type, + argument_count, argument_names, argument_values); + } +} diff --git a/runtime/dart_vm_initializer.h b/runtime/dart_vm_initializer.h new file mode 100644 index 00000000000..93608fcfa73 --- /dev/null +++ b/runtime/dart_vm_initializer.h @@ -0,0 +1,26 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_RUNTIME_DART_VM_INITIALIZER_H_ +#define FLUTTER_RUNTIME_DART_VM_INITIALIZER_H_ + +#include "third_party/dart/runtime/include/dart_api.h" +#include "third_party/dart/runtime/include/dart_tools_api.h" + +class DartVMInitializer { + public: + static void Initialize(Dart_InitializeParams* params); + static void Cleanup(); + + private: + static void LogDartTimelineEvent(const char* label, + int64_t timestamp0, + int64_t timestamp1_or_async_id, + Dart_Timeline_Event_Type type, + intptr_t argument_count, + const char** argument_names, + const char** argument_values); +}; + +#endif // FLUTTER_RUNTIME_DART_VM_INITIALIZER_H_ diff --git a/runtime/dart_vm_lifecycle.h b/runtime/dart_vm_lifecycle.h index 87ad135e0a0..9a4fbfda45c 100644 --- a/runtime/dart_vm_lifecycle.h +++ b/runtime/dart_vm_lifecycle.h @@ -11,6 +11,7 @@ #include "flutter/lib/ui/isolate_name_server/isolate_name_server.h" #include "flutter/runtime/dart_vm.h" #include "flutter/runtime/service_protocol.h" +#include "third_party/dart/runtime/include/dart_tools_api.h" namespace flutter {