From 97eb094aac415bb378dfb2963e98bd70e30712aa Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 28 Jun 2019 15:46:26 -0700 Subject: [PATCH] [flutter] Re-enable unhandled error reporting This re-enables unhandled Dart error handling in Flutter applications, which was removed in a76b958. The error handling as originally landed was unsafe. Specifically, in the case where the unhandled error handler was triggered during shutdown, there was a race condition which could cause a crash in the following scenario: 1. Runner::OnApplicationTerminate() is triggered, which posts a task to the application's platform thread will free the Application instance and terminate the platform thread. 2. Before that task is serviced, the unhandled error handler is called (by hooks.dart -> window.cc -> ui_dart_state.cc) on the UI thread. 3. The kill task is serviced and the Application dtor and Thread::Quit() are called, terminating the platform thread. 4. The unhandled error handler attempts to post a task to the platform thread, whose thread was killed in step 3. This triggers a crash. Fixing this requires a mechanism for the message loop to know that the associated thread has been terminated out from under it. This patch adds mitigation for this scenario, but remains non-threadsafe/racy. We pass the unhandled error handler a weak pointer to the Application and check it before posting a task to the platform thread. This has two issues: 1. WeakPtr isn't threadsafe, and assumes that all operations occur on a single thread. We're checking its value (which is mutated on the platform thread) on the UI thread without synchronization. 2. Even with a guarantee that the WeakPtr state were synchronized, there's a window between when we check the weak pointer and when we post to the platform thread in which application shutdown and thread destruction may occur. This unsafe mitigation is being landed in order to unblock a high priority bug (FL-256) on a short schedule, and a proper refactoring will be required to make this properly threadsafe. Change-Id: If60d1d3ca5799d82597f8a3acc4ddd3871058972 Ported from Topaz tree. --- shell/platform/fuchsia/flutter/component.cc | 42 ++++++++++++++++----- shell/platform/fuchsia/flutter/component.h | 1 + 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/shell/platform/fuchsia/flutter/component.cc b/shell/platform/fuchsia/flutter/component.cc index b117733d3ed07..bce5056064827 100644 --- a/shell/platform/fuchsia/flutter/component.cc +++ b/shell/platform/fuchsia/flutter/component.cc @@ -29,6 +29,7 @@ #include "service_provider_dir.h" #include "task_observers.h" +#include "task_runner_adapter.h" #include "thread.h" namespace flutter_runner { @@ -80,7 +81,8 @@ Application::Application( debug_label_(DebugLabelForURL(startup_info.launch_info.url)), application_controller_(this), outgoing_dir_(new vfs::PseudoDir()), - runner_incoming_services_(runner_incoming_services) { + runner_incoming_services_(runner_incoming_services), + weak_factory_(this) { application_controller_.set_error_handler( [this](zx_status_t status) { Kill(); }); @@ -308,17 +310,37 @@ Application::Application( settings_.dart_flags.push_back("--profile_period=10000"); #endif // defined(__aarch64__) - auto dispatcher = async_get_default_dispatcher(); - FML_CHECK(dispatcher); + auto weak_application = weak_factory_.GetWeakPtr(); + auto platform_task_runner = + CreateFMLTaskRunner(async_get_default_dispatcher()); const std::string component_url = package.resolved_url; settings_.unhandled_exception_callback = - [dispatcher, runner_incoming_services, component_url]( - const std::string& error, const std::string& stack_trace) { - async::PostTask(dispatcher, [runner_incoming_services, component_url, - error, stack_trace]() { - dart_utils::HandleException(runner_incoming_services, component_url, - error, stack_trace); - }); + [weak_application, platform_task_runner, runner_incoming_services, + component_url](const std::string& error, + const std::string& stack_trace) { + if (weak_application) { + // TODO(cbracken): unsafe. The above check and the PostTask below are + // happening on the UI thread. If the Application dtor and thread + // termination happen (on the platform thread) between the previous + // line and the next line, a crash will occur since we'll be posting + // to a dead thread. See Runner::OnApplicationTerminate() in + // runner.cc. + platform_task_runner->PostTask([weak_application, + runner_incoming_services, + component_url, error, stack_trace]() { + if (weak_application) { + dart_utils::HandleException(runner_incoming_services, + component_url, error, stack_trace); + } else { + FML_LOG(ERROR) + << "Unhandled exception after application shutdown: " + << error; + } + }); + } else { + FML_LOG(ERROR) << "Unhandled exception after application shutdown: " + << error; + } // Ideally we would return whether HandleException returned ZX_OK, but // short of knowing if the exception was correctly handled, we return // false to have the error and stack trace printed in the logs. diff --git a/shell/platform/fuchsia/flutter/component.h b/shell/platform/fuchsia/flutter/component.h index 44b8d75a1f182..c9dfc87ba938d 100644 --- a/shell/platform/fuchsia/flutter/component.h +++ b/shell/platform/fuchsia/flutter/component.h @@ -79,6 +79,7 @@ class Application final : public Engine::Delegate, fml::RefPtr shared_snapshot_; std::set> shell_holders_; std::pair last_return_code_; + fml::WeakPtrFactory weak_factory_; Application( TerminationCallback termination_callback,