Skip to content

Commit

Permalink
Avoid a deadlock in the flutter_tester process when deleting the Impe…
Browse files Browse the repository at this point in the history 
…ller Vulkan context during shutdown (flutter#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.
  • Loading branch information
@jason-simmons
jason-simmons authored Oct 16, 2023
1 parent 505c720 commit 6cc678c
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 64 deletions.
102 changes: 54 additions & 48 deletions shell/testing/tester_main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,57 @@ static std::vector<std::shared_ptr<fml::Mapping>> ShaderLibraryMappings() {
}

struct ImpellerVulkanContextHolder {
ImpellerVulkanContextHolder() = default;
ImpellerVulkanContextHolder(ImpellerVulkanContextHolder&&) = default;
fml::RefPtr<vulkan::VulkanProcTable> vulkan_proc_table;
std::shared_ptr<impeller::ContextVK> context;
std::shared_ptr<impeller::SurfaceContextVK> surface_context;

bool Initialize(bool enable_validation);
};

bool ImpellerVulkanContextHolder::Initialize(bool enable_validation) {
vulkan_proc_table =
fml::MakeRefCounted<vulkan::VulkanProcTable>(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
Expand Down Expand Up @@ -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)) {}

Expand Down Expand Up @@ -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::VulkanProcTable>(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<PlatformView> 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<TesterPlatformView>(
shell, shell.GetTaskRunners(), impeller_context_holder);
};
shell, shell.GetTaskRunners(), std::move(impeller_context_holder));
});

Shell::CreateCallback<Rasterizer> on_create_rasterizer = [](Shell& shell) {
return std::make_unique<Rasterizer>(
Expand Down
34 changes: 18 additions & 16 deletions testing/run_tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,29 +867,31 @@ 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
) not in 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):
Expand Down

0 comments on commit 6cc678c

Please sign in to comment.