From cdc18f9d6176597420ca7fa84fb08946c99dcb88 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 8 Nov 2024 12:07:11 -0800 Subject: [PATCH] Add FlutterTaskRunnerDescription.destruction_callback (flutter/engine#56445) Adds a `destruction_callback` member to `FlutterTaskRunnerDescription` that provides embedder developers a means of performing any cleanup of resources tied to the lifetime of the task runner. In the case of the macOS embedder, the task runner holds a reference to the engine itself that is used in the `post_task_callback` and whose lifetime needs to match/exceed that of the task runner. This refactoring allows us to centralise all related retain count manipulation around task runner setup. This is a refactor of the lifecycle bits of https://github.com/flutter/engine/pull/13300. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../macos/framework/Source/FlutterEngine.mm | 16 ++++++++----- .../shell/platform/embedder/embedder.h | 2 ++ .../platform/embedder/embedder_task_runner.cc | 5 +++- .../platform/embedder/embedder_task_runner.h | 4 ++++ .../platform/embedder/embedder_thread_host.cc | 24 +++++++++++++------ .../embedder/tests/embedder_unittests.cc | 6 +++++ .../embedder/tests/embedder_unittests_util.h | 5 ++++ 7 files changed, 48 insertions(+), 14 deletions(-) diff --git a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm index 2c79ee079d4..1a87c663860 100644 --- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm +++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm @@ -645,16 +645,23 @@ static void SetThreadPriority(FlutterThreadPriority priority) { static size_t sTaskRunnerIdentifiers = 0; const FlutterTaskRunnerDescription cocoa_task_runner_description = { .struct_size = sizeof(FlutterTaskRunnerDescription), - .user_data = (void*)CFBridgingRetain(self), + // Retain for use in post_task_callback. Released in destruction_callback. + .user_data = (__bridge_retained void*)self, .runs_task_on_current_thread_callback = [](void* user_data) -> bool { return [[NSThread currentThread] isMainThread]; }, .post_task_callback = [](FlutterTask task, uint64_t target_time_nanos, void* user_data) -> void { - [((__bridge FlutterEngine*)(user_data)) postMainThreadTask:task - targetTimeInNanoseconds:target_time_nanos]; + FlutterEngine* engine = (__bridge FlutterEngine*)user_data; + [engine postMainThreadTask:task targetTimeInNanoseconds:target_time_nanos]; }, .identifier = ++sTaskRunnerIdentifiers, + .destruction_callback = + [](void* user_data) { + // Balancing release for the retain when setting user_data above. + FlutterEngine* engine = (__bridge_transfer FlutterEngine*)user_data; + engine = nil; + }, }; const FlutterCustomTaskRunners custom_task_runners = { .struct_size = sizeof(FlutterCustomTaskRunners), @@ -1144,9 +1151,6 @@ static void SetThreadPriority(FlutterThreadPriority priority) { NSLog(@"Could not de-initialize the Flutter engine: error %d", result); } - // Balancing release for the retain in the task runner dispatch table. - CFRelease((CFTypeRef)self); - result = _embedderAPI.Shutdown(_engine); if (result != kSuccess) { NSLog(@"Failed to shut down Flutter engine: error %d", result); diff --git a/engine/src/flutter/shell/platform/embedder/embedder.h b/engine/src/flutter/shell/platform/embedder/embedder.h index 2873e188925..f0df9973f49 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder.h +++ b/engine/src/flutter/shell/platform/embedder/embedder.h @@ -1651,6 +1651,8 @@ typedef struct { /// A unique identifier for the task runner. If multiple task runners service /// tasks on the same thread, their identifiers must match. size_t identifier; + /// The callback invoked when the task runner is destroyed. + VoidCallback destruction_callback; } FlutterTaskRunnerDescription; typedef struct { diff --git a/engine/src/flutter/shell/platform/embedder/embedder_task_runner.cc b/engine/src/flutter/shell/platform/embedder/embedder_task_runner.cc index abbc340db0f..061baa9a5e9 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder_task_runner.cc +++ b/engine/src/flutter/shell/platform/embedder/embedder_task_runner.cc @@ -18,9 +18,12 @@ EmbedderTaskRunner::EmbedderTaskRunner(DispatchTable table, fml::MessageLoopTaskQueues::GetInstance()->CreateTaskQueue()) { FML_DCHECK(dispatch_table_.post_task_callback); FML_DCHECK(dispatch_table_.runs_task_on_current_thread_callback); + FML_DCHECK(dispatch_table_.destruction_callback); } -EmbedderTaskRunner::~EmbedderTaskRunner() = default; +EmbedderTaskRunner::~EmbedderTaskRunner() { + dispatch_table_.destruction_callback(); +} size_t EmbedderTaskRunner::GetEmbedderIdentifier() const { return embedder_identifier_; diff --git a/engine/src/flutter/shell/platform/embedder/embedder_task_runner.h b/engine/src/flutter/shell/platform/embedder/embedder_task_runner.h index 5c0e1e845d4..4f7b47bd76e 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder_task_runner.h +++ b/engine/src/flutter/shell/platform/embedder/embedder_task_runner.h @@ -39,6 +39,10 @@ class EmbedderTaskRunner final : public fml::TaskRunner { /// thread. /// std::function runs_task_on_current_thread_callback; + + //-------------------------------------------------------------------------- + /// Performs user-designated cleanup on destruction. + std::function destruction_callback; }; //---------------------------------------------------------------------------- diff --git a/engine/src/flutter/shell/platform/embedder/embedder_thread_host.cc b/engine/src/flutter/shell/platform/embedder/embedder_thread_host.cc index 7cca136462e..78ad599fbb0 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder_thread_host.cc +++ b/engine/src/flutter/shell/platform/embedder/embedder_thread_host.cc @@ -55,11 +55,16 @@ CreateEmbedderTaskRunner(const FlutterTaskRunnerDescription* description) { auto runs_task_on_current_thread_callback_c = description->runs_task_on_current_thread_callback; + VoidCallback destruction_callback_c = [](void* user_data) {}; + if (SAFE_ACCESS(description, destruction_callback, nullptr) != nullptr) { + destruction_callback_c = description->destruction_callback; + } + EmbedderTaskRunner::DispatchTable task_runner_dispatch_table = { - // .post_task_callback - [post_task_callback_c, user_data](EmbedderTaskRunner* task_runner, - uint64_t task_baton, - fml::TimePoint target_time) -> void { + .post_task_callback = [post_task_callback_c, user_data]( + EmbedderTaskRunner* task_runner, + uint64_t task_baton, + fml::TimePoint target_time) -> void { FlutterTask task = { // runner reinterpret_cast(task_runner), @@ -69,10 +74,15 @@ CreateEmbedderTaskRunner(const FlutterTaskRunnerDescription* description) { post_task_callback_c(task, target_time.ToEpochDelta().ToNanoseconds(), user_data); }, - // runs_task_on_current_thread_callback - [runs_task_on_current_thread_callback_c, user_data]() -> bool { + .runs_task_on_current_thread_callback = + [runs_task_on_current_thread_callback_c, user_data]() -> bool { return runs_task_on_current_thread_callback_c(user_data); - }}; + }, + .destruction_callback = + [destruction_callback_c, user_data]() { + destruction_callback_c(user_data); + }, + }; return {true, fml::MakeRefCounted( task_runner_dispatch_table, diff --git a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc index 22cdb54cdcf..656865ad0e1 100644 --- a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc +++ b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc @@ -210,6 +210,7 @@ TEST_F(EmbedderTest, CanSpecifyCustomPlatformTaskRunner) { auto platform_task_runner = CreateNewThread("test_platform_thread"); static std::mutex engine_mutex; static bool signaled_once = false; + static std::atomic destruction_callback_called = false; UniqueEngine engine; EmbedderTestTaskRunner test_task_runner( @@ -230,6 +231,8 @@ TEST_F(EmbedderTest, CanSpecifyCustomPlatformTaskRunner) { ASSERT_EQ(FlutterEngineRunTask(engine.get(), &task), kSuccess); latch.Signal(); }); + test_task_runner.SetDestructionCallback( + [](void* user_data) { destruction_callback_called = true; }); platform_task_runner->PostTask([&]() { EmbedderConfigBuilder builder(context); @@ -263,6 +266,9 @@ TEST_F(EmbedderTest, CanSpecifyCustomPlatformTaskRunner) { ASSERT_TRUE(signaled_once); signaled_once = false; + + ASSERT_TRUE(destruction_callback_called); + destruction_callback_called = false; } TEST(EmbedderTestNoFixture, CanGetCurrentTimeInNanoseconds) { diff --git a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests_util.h b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests_util.h index 4e1cae51b1d..1336b3ef343 100644 --- a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests_util.h +++ b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests_util.h @@ -130,9 +130,14 @@ class EmbedderTestTaskRunner { real_task_runner->PostTaskForTime(invoke_task, target_time); }; + task_runner_description_.destruction_callback = [](void* user_data) {}; task_runner_description_.identifier = identifier_; } + void SetDestructionCallback(VoidCallback callback) { + task_runner_description_.destruction_callback = callback; + } + const FlutterTaskRunnerDescription& GetFlutterTaskRunnerDescription() { return task_runner_description_; }