From 96b0bf0ce3b01ab0bc2ce360aca0c024d45843f7 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 7 Oct 2024 13:46:16 -0700 Subject: [PATCH] Speculative fix for memory issues related to retrying image decompression (flutter/engine#55704) b/371512414 This issue had no reproduction but the stacktrace was provided. The theory of this fix is that the `ImageResult` type is capturing something that doesn't handle being copied and deallocated twice so instead of copying it, we std::move it into a shared_ptr so that it will only be deallocated once. Also, I just avoid using a `std::move` when overflowing. The idea here is maybe sometimes deleting the std::move'd item isn't kosher. We know from the stacktrace that it has something to do with overflowing because that would be the only case where something is being deleted. I've added 2 integration tests that exercises the code where the crash appears to be happening. This is good coverage to have but neither of them reproduced the issue, even with MallocScribble enabled. That's the best we can do without a reproduction. ``` Thread 7 (id: 0x0000a103)crashed 0x000000010a48ae8c(Flutter -function.h)std::_fl::allocator::destroy[abi:v15000](impeller::ContextMTL::PendingTasks*) 0x000000010a48b9fc(Flutter -allocator_traits.h:309)impeller::ContextMTL::StoreTaskForGPU(std::_fl::function const&, std::_fl::function const&) 0x000000010a48b9fc(Flutter -allocator_traits.h:309)impeller::ContextMTL::StoreTaskForGPU(std::_fl::function const&, std::_fl::function const&) 0x000000010a4d49e4(Flutter -image_decoder_impeller.cc:422)std::_fl::__function::__func, std::_fl::basic_string, std::_fl::allocator>)>, std::_fl::shared_ptr const&, std::_fl::shared_ptr const&, SkImageInfo const&, std::_fl::shared_ptr const&, std::_fl::optional const&, std::_fl::shared_ptr const&)::$_1, std::_fl::allocator, std::_fl::basic_string, std::_fl::allocator>)>, std::_fl::shared_ptr const&, std::_fl::shared_ptr const&, SkImageInfo const&, std::_fl::shared_ptr const&, std::_fl::optional const&, std::_fl::shared_ptr const&)::$_1>, void ()>::operator()() 0x000000010a06c234(Flutter -function.h:512)fml::SyncSwitch::Execute(fml::SyncSwitch::Handlers const&) const 0x000000010a4d42f8(Flutter -image_decoder_impeller.cc:408)flutter::ImageDecoderImpeller::UploadTextureToPrivate(std::_fl::function, std::_fl::basic_string, std::_fl::allocator>)>, std::_fl::shared_ptr const&, std::_fl::shared_ptr const&, SkImageInfo const&, std::_fl::shared_ptr const&, std::_fl::optional const&, std::_fl::shared_ptr const&) 0x000000010a4d3838(Flutter -image_decoder_impeller.cc:538)std::_fl::__function::__func, unsigned int, unsigned int, std::_fl::function, std::_fl::basic_string, std::_fl::allocator>)> const&)::$_1, std::_fl::allocator, unsigned int, unsigned int, std::_fl::function, std::_fl::basic_string, std::_fl::allocator>)> const&)::$_1>, void ()>::operator()() 0x000000010a06dd1c(Flutter -function.h:512)fml::ConcurrentMessageLoopDarwin::ExecuteTask(std::_fl::function const&) 0x000000010a06737c(Flutter -concurrent_message_loop.cc:101)void* std::_fl::__thread_proxy[abi:v15000]>, fml::ConcurrentMessageLoop::ConcurrentMessageLoop(unsigned long)::$_0>>(void*) 0x000000021d4ff378(libsystem_pthread.dylib + 0x00006378)_pthread_start ``` [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../renderer/backend/metal/context_mtl.mm | 2 +- .../src/flutter/lib/ui/fixtures/ui_test.dart | 73 ++++++++++++++ .../lib/ui/painting/image_decoder_impeller.cc | 21 ++-- .../ui/painting/image_encoding_unittests.cc | 95 +++++++++++++++++++ 4 files changed, 178 insertions(+), 13 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm index e2f5281362a..2d6c432e1b6 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm @@ -383,7 +383,7 @@ void ContextMTL::StoreTaskForGPU(const fml::closure& task, const fml::closure& failure) { tasks_awaiting_gpu_.push_back(PendingTasks{task, failure}); while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) { - PendingTasks front = std::move(tasks_awaiting_gpu_.front()); + const PendingTasks& front = tasks_awaiting_gpu_.front(); if (front.failure) { front.failure(); } diff --git a/engine/src/flutter/lib/ui/fixtures/ui_test.dart b/engine/src/flutter/lib/ui/fixtures/ui_test.dart index 19688d108a6..c7b0967d6a4 100644 --- a/engine/src/flutter/lib/ui/fixtures/ui_test.dart +++ b/engine/src/flutter/lib/ui/fixtures/ui_test.dart @@ -394,6 +394,45 @@ Future toByteDataRetries() async { } } +@pragma('vm:entry-point') +Future toByteDataRetryOverflows() async { + final PictureRecorder pictureRecorder = PictureRecorder(); + final Canvas canvas = Canvas(pictureRecorder); + final Paint paint = Paint() + ..color = Color.fromRGBO(255, 255, 255, 1.0) + ..style = PaintingStyle.fill; + final Offset c = Offset(50.0, 50.0); + canvas.drawCircle(c, 25.0, paint); + final Picture picture = pictureRecorder.endRecording(); + List images = []; + // This number must be bigger than impeller::Context::kMaxTasksAwaitingGPU. + int numJobs = 100; + for (int i = 0; i < numJobs; ++i) { + images.add(await picture.toImage(100, 100)); + } + List> dataFutures = []; + _turnOffGPU(true); + for (Image image in images) { + dataFutures.add(image.toByteData()); + } + Future.delayed(Duration(milliseconds: 10), () { + _turnOffGPU(false); + }); + + ByteData? result; + for (Future future in dataFutures) { + try { + ByteData? byteData = await future; + if (byteData != null) { + result = byteData; + } + } catch (_) { + // Ignore errors from unavailable gpu. + } + } + _validateNotNull(result); +} + @pragma('vm:entry-point') Future toImageRetries() async { final PictureRecorder pictureRecorder = PictureRecorder(); @@ -416,6 +455,40 @@ Future toImageRetries() async { } } +@pragma('vm:entry-point') +Future toImageRetryOverflows() async { + final PictureRecorder pictureRecorder = PictureRecorder(); + final Canvas canvas = Canvas(pictureRecorder); + final Paint paint = Paint() + ..color = Color.fromRGBO(255, 255, 255, 1.0) + ..style = PaintingStyle.fill; + final Offset c = Offset(50.0, 50.0); + canvas.drawCircle(c, 25.0, paint); + final Picture picture = pictureRecorder.endRecording(); + _turnOffGPU(true); + List> imageFutures = []; + // This number must be bigger than impeller::Context::kMaxTasksAwaitingGPU. + int numJobs = 100; + for (int i = 0; i < numJobs; i++) { + imageFutures.add(picture.toImage(100, 100)); + } + Future.delayed(Duration(milliseconds: 10), () { + _turnOffGPU(false); + }); + late Image result; + bool didSeeImage = false; + for (Future future in imageFutures) { + try { + Image image = await future; + result = image; + didSeeImage = true; + } catch (_) { + // Ignore gpu not available errors. + } + } + _validateNotNull(didSeeImage ? result : null); +} + @pragma('vm:entry-point') Future pumpImage() async { const int width = 60; diff --git a/engine/src/flutter/lib/ui/painting/image_decoder_impeller.cc b/engine/src/flutter/lib/ui/painting/image_decoder_impeller.cc index 4fe7f283e15..adb06b435fc 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoder_impeller.cc +++ b/engine/src/flutter/lib/ui/painting/image_decoder_impeller.cc @@ -418,22 +418,19 @@ void ImageDecoderImpeller::UploadTextureToPrivate( result(image, decode_error); }) .SetIfTrue([&result, context, buffer, image_info, resize_info] { - // The `result` function must be copied in the capture list for each - // closure or the stack allocated callback will be cleared by the - // time to closure is executed later. + auto result_ptr = std::make_shared(std::move(result)); context->StoreTaskForGPU( - [result, context, buffer, image_info, resize_info]() { + [result_ptr, context, buffer, image_info, resize_info]() { sk_sp image; std::string decode_error; - std::tie(image, decode_error) = - std::tie(image, decode_error) = - UnsafeUploadTextureToPrivate(context, buffer, - image_info, resize_info); - result(image, decode_error); + std::tie(image, decode_error) = UnsafeUploadTextureToPrivate( + context, buffer, image_info, resize_info); + (*result_ptr)(image, decode_error); }, - [result]() { - result(nullptr, - "Image upload failed due to loss of GPU access."); + [result_ptr]() { + (*result_ptr)( + nullptr, + "Image upload failed due to loss of GPU access."); }); })); } diff --git a/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc b/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc index 380647d43d0..db4afca5f7e 100644 --- a/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc +++ b/engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc @@ -280,6 +280,54 @@ TEST_F(ShellTest, EncodeImageRetries) { DestroyShell(std::move(shell), task_runners); } +TEST_F(ShellTest, EncodeImageRetryOverflows) { +#ifndef FML_OS_MACOSX + GTEST_SKIP() << "Only works on macos currently."; +#endif + Settings settings = CreateSettingsForFixture(); + settings.enable_impeller = true; + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = CreateShell({ + .settings = settings, + .task_runners = task_runners, + }); + + auto turn_off_gpu = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + bool value = true; + ASSERT_TRUE(Dart_IsBoolean(handle)); + Dart_BooleanValue(handle, &value); + TurnOffGPU(shell.get(), value); + }; + + AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu)); + + auto validate_not_null = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + EXPECT_FALSE(Dart_IsNull(handle)); + message_latch.Signal(); + }; + + AddNativeCallback("ValidateNotNull", CREATE_NATIVE_ENTRY(validate_not_null)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("toByteDataRetryOverflows"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch.Wait(); + DestroyShell(std::move(shell), task_runners); +} + TEST_F(ShellTest, ToImageRetries) { #ifndef FML_OS_MACOSX GTEST_SKIP() << "Only works on macos currently."; @@ -327,6 +375,53 @@ TEST_F(ShellTest, ToImageRetries) { message_latch.Wait(); DestroyShell(std::move(shell), task_runners); } +TEST_F(ShellTest, ToImageRetryOverflow) { +#ifndef FML_OS_MACOSX + GTEST_SKIP() << "Only works on macos currently."; +#endif + Settings settings = CreateSettingsForFixture(); + settings.enable_impeller = true; + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = CreateShell({ + .settings = settings, + .task_runners = task_runners, + }); + + auto turn_off_gpu = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + bool value = true; + ASSERT_TRUE(Dart_IsBoolean(handle)); + Dart_BooleanValue(handle, &value); + TurnOffGPU(shell.get(), value); + }; + + AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu)); + + auto validate_not_null = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + EXPECT_FALSE(Dart_IsNull(handle)); + message_latch.Signal(); + }; + + AddNativeCallback("ValidateNotNull", CREATE_NATIVE_ENTRY(validate_not_null)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("toImageRetryOverflows"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch.Wait(); + DestroyShell(std::move(shell), task_runners); +} TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { #ifndef FML_OS_MACOSX