From 9294d02a9ede4b16b600bdc59b6d6537cf2a21d6 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Thu, 22 Feb 2024 21:26:07 +0000 Subject: [PATCH 1/3] [Impeller] Fix a race that can abort the process if the Vulkan context is destroyed while pipeline creation tasks are pending The Vulkan pipeline library queues pipeline creation tasks to a ConcurrentTaskRunner. If the ContextVK is destroyed before these tasks execute, then the ConcurrentMessageLoop destructor will delete the pending tasks. Each task lambda holds a reference to a promise that will be completed with the pipeline. If the task was never run and its promise was never completed, then the promise destructor will complete it with an exception. This will cause a std::abort because Flutter is built without exception support. This PR wraps the promise in an object that completes it with a default value during destruction if the promise was never given a value. --- impeller/base/base_unittests.cc | 17 ++++++++++ impeller/base/promise.h | 32 +++++++++++++++++++ .../backend/vulkan/pipeline_library_vk.cc | 2 +- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/impeller/base/base_unittests.cc b/impeller/base/base_unittests.cc index a7c40d868df55..6908627395fb8 100644 --- a/impeller/base/base_unittests.cc +++ b/impeller/base/base_unittests.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "flutter/testing/testing.h" +#include "impeller/base/promise.h" #include "impeller/base/strings.h" #include "impeller/base/thread.h" @@ -233,5 +234,21 @@ TEST(ConditionVariableTest, TestsCriticalSectionAfterWait) { ASSERT_EQ(sum, kThreadCount); } +TEST(BaseTest, PromiseDestructWrapperValue) { + PromiseDestructWrapper wrapper; + std::future future = wrapper.get_future(); + wrapper.set_value(123); + ASSERT_EQ(future.get(), 123); +} + +TEST(BaseTest, PromiseDestructWrapperEmpty) { + auto wrapper = std::make_shared>(); + std::future future = wrapper->get_future(); + + // Destroy the empty promise with the future still pending. Verify that the + // process does not abort while destructing the promise. + wrapper.reset(); +} + } // namespace testing } // namespace impeller diff --git a/impeller/base/promise.h b/impeller/base/promise.h index 30d45ae4d3f5f..3b7629a950814 100644 --- a/impeller/base/promise.h +++ b/impeller/base/promise.h @@ -6,6 +6,8 @@ #define FLUTTER_IMPELLER_BASE_PROMISE_H_ #include +#include +#include namespace impeller { @@ -17,6 +19,36 @@ std::future RealizedFuture(T t) { return future; } +// Wraps a std::promise and completes the promise with a value during +// destruction if the promise does not already have a value. +// +// By default the std::promise destructor will complete an empty promise with an +// exception. This will fail because Flutter is built without exception support. +template +class PromiseDestructWrapper { + public: + PromiseDestructWrapper() : promise_(std::in_place) {} + + ~PromiseDestructWrapper() { + if (promise_) { + promise_->set_value({}); + } + } + + std::future get_future() { + FML_DCHECK(promise_); + return promise_->get_future(); + } + + void set_value(const T& value) { + promise_->set_value(value); + promise_.reset(); + } + + private: + std::optional> promise_; +}; + } // namespace impeller #endif // FLUTTER_IMPELLER_BASE_PROMISE_H_ diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index 600ef830450d5..493a4f857e367 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -170,7 +170,7 @@ PipelineFuture PipelineLibraryVK::GetPipeline( } auto promise = std::make_shared< - std::promise>>>(); + PromiseDestructWrapper>>>(); auto pipeline_future = PipelineFuture{descriptor, promise->get_future()}; pipelines_[descriptor] = pipeline_future; From f4e14b90f48d0ad8e929a7e65406c30b6754ec3c Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 23 Feb 2024 02:37:48 +0000 Subject: [PATCH 2/3] remove use of optional --- impeller/base/promise.h | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/impeller/base/promise.h b/impeller/base/promise.h index 3b7629a950814..9998ae6e62f63 100644 --- a/impeller/base/promise.h +++ b/impeller/base/promise.h @@ -6,8 +6,6 @@ #define FLUTTER_IMPELLER_BASE_PROMISE_H_ #include -#include -#include namespace impeller { @@ -27,26 +25,24 @@ std::future RealizedFuture(T t) { template class PromiseDestructWrapper { public: - PromiseDestructWrapper() : promise_(std::in_place) {} + PromiseDestructWrapper() = default; ~PromiseDestructWrapper() { - if (promise_) { - promise_->set_value({}); + if (!value_set_) { + promise_.set_value({}); } } - std::future get_future() { - FML_DCHECK(promise_); - return promise_->get_future(); - } + std::future get_future() { return promise_.get_future(); } void set_value(const T& value) { - promise_->set_value(value); - promise_.reset(); + promise_.set_value(value); + value_set_ = true; } private: - std::optional> promise_; + std::promise promise_; + bool value_set_ = false; }; } // namespace impeller From d25e0b61abb0cfb795193eb8c67102b3fb15c634 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 23 Feb 2024 17:31:09 +0000 Subject: [PATCH 3/3] rename to NoExceptionPromise --- impeller/base/base_unittests.cc | 8 ++++---- impeller/base/promise.h | 6 +++--- impeller/renderer/backend/vulkan/pipeline_library_vk.cc | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/impeller/base/base_unittests.cc b/impeller/base/base_unittests.cc index 6908627395fb8..db2c97852be49 100644 --- a/impeller/base/base_unittests.cc +++ b/impeller/base/base_unittests.cc @@ -234,15 +234,15 @@ TEST(ConditionVariableTest, TestsCriticalSectionAfterWait) { ASSERT_EQ(sum, kThreadCount); } -TEST(BaseTest, PromiseDestructWrapperValue) { - PromiseDestructWrapper wrapper; +TEST(BaseTest, NoExceptionPromiseValue) { + NoExceptionPromise wrapper; std::future future = wrapper.get_future(); wrapper.set_value(123); ASSERT_EQ(future.get(), 123); } -TEST(BaseTest, PromiseDestructWrapperEmpty) { - auto wrapper = std::make_shared>(); +TEST(BaseTest, NoExceptionPromiseEmpty) { + auto wrapper = std::make_shared>(); std::future future = wrapper->get_future(); // Destroy the empty promise with the future still pending. Verify that the diff --git a/impeller/base/promise.h b/impeller/base/promise.h index 9998ae6e62f63..9f7205eaab60b 100644 --- a/impeller/base/promise.h +++ b/impeller/base/promise.h @@ -23,11 +23,11 @@ std::future RealizedFuture(T t) { // By default the std::promise destructor will complete an empty promise with an // exception. This will fail because Flutter is built without exception support. template -class PromiseDestructWrapper { +class NoExceptionPromise { public: - PromiseDestructWrapper() = default; + NoExceptionPromise() = default; - ~PromiseDestructWrapper() { + ~NoExceptionPromise() { if (!value_set_) { promise_.set_value({}); } diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index 493a4f857e367..130cabd36ae98 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -170,7 +170,7 @@ PipelineFuture PipelineLibraryVK::GetPipeline( } auto promise = std::make_shared< - PromiseDestructWrapper>>>(); + NoExceptionPromise>>>(); auto pipeline_future = PipelineFuture{descriptor, promise->get_future()}; pipelines_[descriptor] = pipeline_future;