Skip to content

Commit

Permalink
[flutter] Re-enable unhandled error reporting
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cbracken committed Jun 29, 2019
1 parent a48c21a commit 97eb094
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
42 changes: 32 additions & 10 deletions shell/platform/fuchsia/flutter/component.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "service_provider_dir.h"
#include "task_observers.h"
#include "task_runner_adapter.h"
#include "thread.h"

namespace flutter_runner {
Expand Down Expand Up @@ -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(); });

Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions shell/platform/fuchsia/flutter/component.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class Application final : public Engine::Delegate,
fml::RefPtr<flutter::DartSnapshot> shared_snapshot_;
std::set<std::unique_ptr<Engine>> shell_holders_;
std::pair<bool, uint32_t> last_return_code_;
fml::WeakPtrFactory<Application> weak_factory_;

Application(
TerminationCallback termination_callback,
Expand Down

0 comments on commit 97eb094

Please sign in to comment.