firka/flutter
firka/flutter
4
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Speculative fix for memory issues related to retrying image decompression (flutter/engine#55704)

Browse Source 
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<impeller::ContextMTL::PendingTasks>::destroy[abi:v15000](impeller::ContextMTL::PendingTasks*)
0x000000010a48b9fc(Flutter -allocator_traits.h:309)impeller::ContextMTL::StoreTaskForGPU(std::_fl::function<void ()> const&, std::_fl::function<void ()> const&)
0x000000010a48b9fc(Flutter -allocator_traits.h:309)impeller::ContextMTL::StoreTaskForGPU(std::_fl::function<void ()> const&, std::_fl::function<void ()> const&)
0x000000010a4d49e4(Flutter -image_decoder_impeller.cc:422)std::_fl::__function::__func<flutter::ImageDecoderImpeller::UploadTextureToPrivate(std::_fl::function<void (sk_sp<flutter::DlImage>, std::_fl::basic_string<char, std::_fl::char_traits<char>, std::_fl::allocator<char>>)>, std::_fl::shared_ptr<impeller::Context> const&, std::_fl::shared_ptr<impeller::DeviceBuffer> const&, SkImageInfo const&, std::_fl::shared_ptr<SkBitmap> const&, std::_fl::optional<SkImageInfo> const&, std::_fl::shared_ptr<fml::SyncSwitch> const&)::$_1, std::_fl::allocator<flutter::ImageDecoderImpeller::UploadTextureToPrivate(std::_fl::function<void (sk_sp<flutter::DlImage>, std::_fl::basic_string<char, std::_fl::char_traits<char>, std::_fl::allocator<char>>)>, std::_fl::shared_ptr<impeller::Context> const&, std::_fl::shared_ptr<impeller::DeviceBuffer> const&, SkImageInfo const&, std::_fl::shared_ptr<SkBitmap> const&, std::_fl::optional<SkImageInfo> const&, std::_fl::shared_ptr<fml::SyncSwitch> 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<void (sk_sp<flutter::DlImage>, std::_fl::basic_string<char, std::_fl::char_traits<char>, std::_fl::allocator<char>>)>, std::_fl::shared_ptr<impeller::Context> const&, std::_fl::shared_ptr<impeller::DeviceBuffer> const&, SkImageInfo const&, std::_fl::shared_ptr<SkBitmap> const&, std::_fl::optional<SkImageInfo> const&, std::_fl::shared_ptr<fml::SyncSwitch> const&)
0x000000010a4d3838(Flutter -image_decoder_impeller.cc:538)std::_fl::__function::__func<flutter::ImageDecoderImpeller::Decode(fml::RefPtr<flutter::ImageDescriptor>, unsigned int, unsigned int, std::_fl::function<void (sk_sp<flutter::DlImage>, std::_fl::basic_string<char, std::_fl::char_traits<char>, std::_fl::allocator<char>>)> const&)::$_1, std::_fl::allocator<flutter::ImageDecoderImpeller::Decode(fml::RefPtr<flutter::ImageDescriptor>, unsigned int, unsigned int, std::_fl::function<void (sk_sp<flutter::DlImage>, std::_fl::basic_string<char, std::_fl::char_traits<char>, std::_fl::allocator<char>>)> const&)::$_1>, void ()>::operator()()
0x000000010a06dd1c(Flutter -function.h:512)fml::ConcurrentMessageLoopDarwin::ExecuteTask(std::_fl::function<void ()> const&)
0x000000010a06737c(Flutter -concurrent_message_loop.cc:101)void* std::_fl::__thread_proxy[abi:v15000]<std::_fl::tuple<std::_fl::unique_ptr<std::_fl::__thread_struct, std::_fl::default_delete<std::_fl::__thread_struct>>, 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
This commit is contained in:
gaaclarke 2024-10-07 13:46:16 -07:00 committed by GitHub
parent 3f50f56235
commit 96b0bf0ce3
4 changed files with 178 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm
View File

@ -383,7 +383,7 @@ void ContextMTL::StoreTaskForGPU(const fml::closure& task,
const fml::closure& failure) { const fml::closure& failure) {
tasks_awaiting_gpu_.push_back(PendingTasks{task, failure}); tasks_awaiting_gpu_.push_back(PendingTasks{task, failure});
while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) { while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) {
PendingTasks front = std::move(tasks_awaiting_gpu_.front()); const PendingTasks& front = tasks_awaiting_gpu_.front();
if (front.failure) { if (front.failure) {
front.failure(); front.failure();
} }

73
engine/src/flutter/lib/ui/fixtures/ui_test.dart
View File

@ -394,6 +394,45 @@ Future<void> toByteDataRetries() async {
} }
} }
@pragma('vm:entry-point')
Future<void> 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<Image> 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<Future<ByteData?>> dataFutures = [];
_turnOffGPU(true);
for (Image image in images) {
dataFutures.add(image.toByteData());
}
Future<void>.delayed(Duration(milliseconds: 10), () {
_turnOffGPU(false);
});
ByteData? result;
for (Future<ByteData?> 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') @pragma('vm:entry-point')
Future<void> toImageRetries() async { Future<void> toImageRetries() async {
final PictureRecorder pictureRecorder = PictureRecorder(); final PictureRecorder pictureRecorder = PictureRecorder();
@ -416,6 +455,40 @@ Future<void> toImageRetries() async {
} }
} }
@pragma('vm:entry-point')
Future<void> 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<Future<Image>> 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<void>.delayed(Duration(milliseconds: 10), () {
_turnOffGPU(false);
});
late Image result;
bool didSeeImage = false;
for (Future<Image> 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') @pragma('vm:entry-point')
Future<void> pumpImage() async { Future<void> pumpImage() async {
const int width = 60; const int width = 60;

21
engine/src/flutter/lib/ui/painting/image_decoder_impeller.cc
View File

@ -418,22 +418,19 @@ void ImageDecoderImpeller::UploadTextureToPrivate(
result(image, decode_error); result(image, decode_error);
}) })
.SetIfTrue([&result, context, buffer, image_info, resize_info] { .SetIfTrue([&result, context, buffer, image_info, resize_info] {
// The `result` function must be copied in the capture list for each auto result_ptr = std::make_shared<ImageResult>(std::move(result));
// closure or the stack allocated callback will be cleared by the
// time to closure is executed later.
context->StoreTaskForGPU( context->StoreTaskForGPU(
[result, context, buffer, image_info, resize_info]() { [result_ptr, context, buffer, image_info, resize_info]() {
sk_sp<DlImage> image; sk_sp<DlImage> image;
std::string decode_error; std::string decode_error;
std::tie(image, decode_error) = std::tie(image, decode_error) = UnsafeUploadTextureToPrivate(
std::tie(image, decode_error) = context, buffer, image_info, resize_info);
UnsafeUploadTextureToPrivate(context, buffer, (*result_ptr)(image, decode_error);
image_info, resize_info);
result(image, decode_error);
}, },
[result]() { [result_ptr]() {
result(nullptr, (*result_ptr)(
"Image upload failed due to loss of GPU access."); nullptr,
"Image upload failed due to loss of GPU access.");
}); });
})); }));
} }

95
engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc
View File

@ -280,6 +280,54 @@ TEST_F(ShellTest, EncodeImageRetries) {
DestroyShell(std::move(shell), task_runners); 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> 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) { TEST_F(ShellTest, ToImageRetries) {
#ifndef FML_OS_MACOSX #ifndef FML_OS_MACOSX
GTEST_SKIP() << "Only works on macos currently."; GTEST_SKIP() << "Only works on macos currently.";
@ -327,6 +375,53 @@ TEST_F(ShellTest, ToImageRetries) {
message_latch.Wait(); message_latch.Wait();
DestroyShell(std::move(shell), task_runners); 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> 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) { TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) {
#ifndef FML_OS_MACOSX #ifndef FML_OS_MACOSX