From 9019beb02b0ea1351a36d6bc018d81d5efe78a2d Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 16 Oct 2023 07:37:18 -0700 Subject: [PATCH] Avoid a deadlock in the flutter_tester process when deleting the Impeller Vulkan context during shutdown (flutter/engine#46860) The Impeller ContextVK contains a ConcurrentMessageLoop whose threads may invoke Dart timeline APIs. The Dart APIs will create a thread-local object that will be deleted during thread shutdown. Therefore, these threads should not outlive the engine/Shell and Dart VM. Previously, RunTester held the ImpellerVulkanContextHolder on the stack, and its reference to the ContextVK would be dropped while exiting the function after the Shell is destructed. This PR moves the contents of the tester's ImpellerVulkanContextHolder out of the instance on the stack and into a lambda owned by the Shell. It also reenables the flutter_tester Impeller tests in the run_tests script. --- .../src/flutter/shell/testing/tester_main.cc | 102 +++++++++--------- engine/src/flutter/testing/run_tests.py | 34 +++--- 2 files changed, 72 insertions(+), 64 deletions(-) diff --git a/engine/src/flutter/shell/testing/tester_main.cc b/engine/src/flutter/shell/testing/tester_main.cc index 8d59300fc2..607dc6a6a6 100644 --- a/engine/src/flutter/shell/testing/tester_main.cc +++ b/engine/src/flutter/shell/testing/tester_main.cc @@ -64,10 +64,57 @@ static std::vector> ShaderLibraryMappings() { } struct ImpellerVulkanContextHolder { + ImpellerVulkanContextHolder() = default; + ImpellerVulkanContextHolder(ImpellerVulkanContextHolder&&) = default; fml::RefPtr vulkan_proc_table; std::shared_ptr context; std::shared_ptr surface_context; + + bool Initialize(bool enable_validation); }; + +bool ImpellerVulkanContextHolder::Initialize(bool enable_validation) { + vulkan_proc_table = + fml::MakeRefCounted(VULKAN_SO_PATH); + if (!vulkan_proc_table->NativeGetInstanceProcAddr()) { + FML_LOG(ERROR) << "Could not load Swiftshader library."; + return false; + } + impeller::ContextVK::Settings context_settings; + context_settings.proc_address_callback = + vulkan_proc_table->NativeGetInstanceProcAddr(); + context_settings.shader_libraries_data = ShaderLibraryMappings(); + context_settings.cache_directory = fml::paths::GetCachesDirectory(); + context_settings.enable_validation = enable_validation; + + context = impeller::ContextVK::Create(std::move(context_settings)); + if (!context || !context->IsValid()) { + VALIDATION_LOG << "Could not create Vulkan context."; + return false; + } + + impeller::vk::SurfaceKHR vk_surface; + impeller::vk::HeadlessSurfaceCreateInfoEXT surface_create_info; + auto res = context->GetInstance().createHeadlessSurfaceEXT( + &surface_create_info, // surface create info + nullptr, // allocator + &vk_surface // surface + ); + if (res != impeller::vk::Result::eSuccess) { + VALIDATION_LOG << "Could not create surface for tester " + << impeller::vk::to_string(res); + return false; + } + + impeller::vk::UniqueSurfaceKHR surface{vk_surface, context->GetInstance()}; + surface_context = context->CreateSurfaceContext(); + if (!surface_context->SetWindowSurface(std::move(surface))) { + VALIDATION_LOG << "Could not set up surface for context."; + return false; + } + return true; +} + #else struct ImpellerVulkanContextHolder {}; #endif // IMPELLER_SUPPORTS_RENDERING @@ -126,7 +173,7 @@ class TesterPlatformView : public PlatformView, public: TesterPlatformView(Delegate& delegate, const TaskRunners& task_runners, - ImpellerVulkanContextHolder impeller_context_holder) + ImpellerVulkanContextHolder&& impeller_context_holder) : PlatformView(delegate, task_runners), impeller_context_holder_(std::move(impeller_context_holder)) {} @@ -313,60 +360,19 @@ int RunTester(const flutter::Settings& settings, #if ALLOW_IMPELLER if (settings.enable_impeller) { - impeller_context_holder.vulkan_proc_table = - fml::MakeRefCounted(VULKAN_SO_PATH); - if (!impeller_context_holder.vulkan_proc_table - ->NativeGetInstanceProcAddr()) { - FML_LOG(ERROR) << "Could not load Swiftshader library."; - return EXIT_FAILURE; - } - impeller::ContextVK::Settings context_settings; - context_settings.proc_address_callback = - impeller_context_holder.vulkan_proc_table->NativeGetInstanceProcAddr(); - context_settings.shader_libraries_data = ShaderLibraryMappings(); - context_settings.cache_directory = fml::paths::GetCachesDirectory(); - context_settings.enable_validation = settings.enable_vulkan_validation; - - impeller_context_holder.context = - impeller::ContextVK::Create(std::move(context_settings)); - if (!impeller_context_holder.context || - !impeller_context_holder.context->IsValid()) { - VALIDATION_LOG << "Could not create Vulkan context."; - return EXIT_FAILURE; - } - - impeller::vk::SurfaceKHR vk_surface; - impeller::vk::HeadlessSurfaceCreateInfoEXT surface_create_info; - auto res = - impeller_context_holder.context->GetInstance().createHeadlessSurfaceEXT( - &surface_create_info, // surface create info - nullptr, // allocator - &vk_surface // surface - ); - if (res != impeller::vk::Result::eSuccess) { - VALIDATION_LOG << "Could not create surface for tester " - << impeller::vk::to_string(res); - return EXIT_FAILURE; - } - - impeller::vk::UniqueSurfaceKHR surface{ - vk_surface, impeller_context_holder.context->GetInstance()}; - impeller_context_holder.surface_context = - impeller_context_holder.context->CreateSurfaceContext(); - if (!impeller_context_holder.surface_context->SetWindowSurface( - std::move(surface))) { - VALIDATION_LOG << "Could not set up surface for context."; + if (!impeller_context_holder.Initialize( + settings.enable_vulkan_validation)) { return EXIT_FAILURE; } } #endif // ALLOW_IMPELLER Shell::CreateCallback on_create_platform_view = - [impeller_context_holder = - std::move(impeller_context_holder)](Shell& shell) { + fml::MakeCopyable([impeller_context_holder = std::move( + impeller_context_holder)](Shell& shell) mutable { return std::make_unique( - shell, shell.GetTaskRunners(), impeller_context_holder); - }; + shell, shell.GetTaskRunners(), std::move(impeller_context_holder)); + }); Shell::CreateCallback on_create_rasterizer = [](Shell& shell) { return std::make_unique( diff --git a/engine/src/flutter/testing/run_tests.py b/engine/src/flutter/testing/run_tests.py index 42488b83c2..7f196402c9 100755 --- a/engine/src/flutter/testing/run_tests.py +++ b/engine/src/flutter/testing/run_tests.py @@ -867,15 +867,16 @@ def gather_dart_tests(build_dir, test_filter): logger.info( "Gathering dart test '%s' with observatory enabled", dart_test_file ) - for multithreaded, enable_impeller in [(True, False), (False, False)]: - yield gather_dart_test( - build_dir, dart_test_file, - FlutterTesterOptions( - multithreaded=multithreaded, - enable_impeller=enable_impeller, - enable_observatory=True - ) - ) + for multithreaded in [False, True]: + for enable_impeller in [False, True]: + yield gather_dart_test( + build_dir, dart_test_file, + FlutterTesterOptions( + multithreaded=multithreaded, + enable_impeller=enable_impeller, + enable_observatory=True + ) + ) for dart_test_file in dart_tests: if test_filter is not None and os.path.basename(dart_test_file @@ -883,13 +884,14 @@ def gather_dart_tests(build_dir, test_filter): logger.info("Skipping '%s' due to filter.", dart_test_file) else: logger.info("Gathering dart test '%s'", dart_test_file) - for multithreaded, enable_impeller in [(True, False), (False, False)]: - yield gather_dart_test( - build_dir, dart_test_file, - FlutterTesterOptions( - multithreaded=multithreaded, enable_impeller=enable_impeller - ) - ) + for multithreaded in [False, True]: + for enable_impeller in [False, True]: + yield gather_dart_test( + build_dir, dart_test_file, + FlutterTesterOptions( + multithreaded=multithreaded, enable_impeller=enable_impeller + ) + ) def gather_dart_smoke_test(build_dir, test_filter):