Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 427302e

Browse files
gaaclarkegaaclarke
authored
Speculative fix for memory issues related to retrying image decompression (#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<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
1 parent 772142e commit 427302e

File tree

4 files changed

+178
-13
lines changed

4 files changed

+178
-13
lines changed

‎impeller/renderer/backend/metal/context_mtl.mm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ new ContextMTL(device, command_queue,
383383
const fml::closure& failure) {
384384
tasks_awaiting_gpu_.push_back(PendingTasks{task, failure});
385385
while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) {
386-
PendingTasks front = std::move(tasks_awaiting_gpu_.front());
386+
const PendingTasks& front = tasks_awaiting_gpu_.front();
387387
if (front.failure) {
388388
front.failure();
389389
}

‎lib/ui/fixtures/ui_test.dart‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,45 @@ Future<void> toByteDataRetries() async {
394394
}
395395
}
396396

397+
@pragma('vm:entry-point')
398+
Future<void> toByteDataRetryOverflows() async {
399+
final PictureRecorder pictureRecorder = PictureRecorder();
400+
final Canvas canvas = Canvas(pictureRecorder);
401+
final Paint paint = Paint()
402+
..color = Color.fromRGBO(255, 255, 255, 1.0)
403+
..style = PaintingStyle.fill;
404+
final Offset c = Offset(50.0, 50.0);
405+
canvas.drawCircle(c, 25.0, paint);
406+
final Picture picture = pictureRecorder.endRecording();
407+
List<Image> images = [];
408+
// This number must be bigger than impeller::Context::kMaxTasksAwaitingGPU.
409+
int numJobs = 100;
410+
for (int i = 0; i < numJobs; ++i) {
411+
images.add(await picture.toImage(100, 100));
412+
}
413+
List<Future<ByteData?>> dataFutures = [];
414+
_turnOffGPU(true);
415+
for (Image image in images) {
416+
dataFutures.add(image.toByteData());
417+
}
418+
Future<void>.delayed(Duration(milliseconds: 10), () {
419+
_turnOffGPU(false);
420+
});
421+
422+
ByteData? result;
423+
for (Future<ByteData?> future in dataFutures) {
424+
try {
425+
ByteData? byteData = await future;
426+
if (byteData != null) {
427+
result = byteData;
428+
}
429+
} catch (_) {
430+
// Ignore errors from unavailable gpu.
431+
}
432+
}
433+
_validateNotNull(result);
434+
}
435+
397436
@pragma('vm:entry-point')
398437
Future<void> toImageRetries() async {
399438
final PictureRecorder pictureRecorder = PictureRecorder();
@@ -416,6 +455,40 @@ Future<void> toImageRetries() async {
416455
}
417456
}
418457

458+
@pragma('vm:entry-point')
459+
Future<void> toImageRetryOverflows() async {
460+
final PictureRecorder pictureRecorder = PictureRecorder();
461+
final Canvas canvas = Canvas(pictureRecorder);
462+
final Paint paint = Paint()
463+
..color = Color.fromRGBO(255, 255, 255, 1.0)
464+
..style = PaintingStyle.fill;
465+
final Offset c = Offset(50.0, 50.0);
466+
canvas.drawCircle(c, 25.0, paint);
467+
final Picture picture = pictureRecorder.endRecording();
468+
_turnOffGPU(true);
469+
List<Future<Image>> imageFutures = [];
470+
// This number must be bigger than impeller::Context::kMaxTasksAwaitingGPU.
471+
int numJobs = 100;
472+
for (int i = 0; i < numJobs; i++) {
473+
imageFutures.add(picture.toImage(100, 100));
474+
}
475+
Future<void>.delayed(Duration(milliseconds: 10), () {
476+
_turnOffGPU(false);
477+
});
478+
late Image result;
479+
bool didSeeImage = false;
480+
for (Future<Image> future in imageFutures) {
481+
try {
482+
Image image = await future;
483+
result = image;
484+
didSeeImage = true;
485+
} catch (_) {
486+
// Ignore gpu not available errors.
487+
}
488+
}
489+
_validateNotNull(didSeeImage ? result : null);
490+
}
491+
419492
@pragma('vm:entry-point')
420493
Future<void> pumpImage() async {
421494
const int width = 60;

‎lib/ui/painting/image_decoder_impeller.cc‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -418,22 +418,19 @@ void ImageDecoderImpeller::UploadTextureToPrivate(
418418
result(image, decode_error);
419419
})
420420
.SetIfTrue([&result, context, buffer, image_info, resize_info] {
421-
// The `result` function must be copied in the capture list for each
422-
// closure or the stack allocated callback will be cleared by the
423-
// time to closure is executed later.
421+
auto result_ptr = std::make_shared<ImageResult>(std::move(result));
424422
context->StoreTaskForGPU(
425-
[result, context, buffer, image_info, resize_info]() {
423+
[result_ptr, context, buffer, image_info, resize_info]() {
426424
sk_sp<DlImage> image;
427425
std::string decode_error;
428-
std::tie(image, decode_error) =
429-
std::tie(image, decode_error) =
430-
UnsafeUploadTextureToPrivate(context, buffer,
431-
image_info, resize_info);
432-
result(image, decode_error);
426+
std::tie(image, decode_error) = UnsafeUploadTextureToPrivate(
427+
context, buffer, image_info, resize_info);
428+
(*result_ptr)(image, decode_error);
433429
},
434-
[result]() {
435-
result(nullptr,
436-
"Image upload failed due to loss of GPU access.");
430+
[result_ptr]() {
431+
(*result_ptr)(
432+
nullptr,
433+
"Image upload failed due to loss of GPU access.");
437434
});
438435
}));
439436
}

‎lib/ui/painting/image_encoding_unittests.cc‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,54 @@ TEST_F(ShellTest, EncodeImageRetries) {
280280
DestroyShell(std::move(shell), task_runners);
281281
}
282282

283+
TEST_F(ShellTest, EncodeImageRetryOverflows) {
284+
#ifndef FML_OS_MACOSX
285+
GTEST_SKIP() << "Only works on macos currently.";
286+
#endif
287+
Settings settings = CreateSettingsForFixture();
288+
settings.enable_impeller = true;
289+
TaskRunners task_runners("test", // label
290+
GetCurrentTaskRunner(), // platform
291+
CreateNewThread(), // raster
292+
CreateNewThread(), // ui
293+
CreateNewThread() // io
294+
);
295+
296+
std::unique_ptr<Shell> shell = CreateShell({
297+
.settings = settings,
298+
.task_runners = task_runners,
299+
});
300+
301+
auto turn_off_gpu = [&](Dart_NativeArguments args) {
302+
auto handle = Dart_GetNativeArgument(args, 0);
303+
bool value = true;
304+
ASSERT_TRUE(Dart_IsBoolean(handle));
305+
Dart_BooleanValue(handle, &value);
306+
TurnOffGPU(shell.get(), value);
307+
};
308+
309+
AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu));
310+
311+
auto validate_not_null = [&](Dart_NativeArguments args) {
312+
auto handle = Dart_GetNativeArgument(args, 0);
313+
EXPECT_FALSE(Dart_IsNull(handle));
314+
message_latch.Signal();
315+
};
316+
317+
AddNativeCallback("ValidateNotNull", CREATE_NATIVE_ENTRY(validate_not_null));
318+
319+
ASSERT_TRUE(shell->IsSetup());
320+
auto configuration = RunConfiguration::InferFromSettings(settings);
321+
configuration.SetEntrypoint("toByteDataRetryOverflows");
322+
323+
shell->RunEngine(std::move(configuration), [&](auto result) {
324+
ASSERT_EQ(result, Engine::RunStatus::Success);
325+
});
326+
327+
message_latch.Wait();
328+
DestroyShell(std::move(shell), task_runners);
329+
}
330+
283331
TEST_F(ShellTest, ToImageRetries) {
284332
#ifndef FML_OS_MACOSX
285333
GTEST_SKIP() << "Only works on macos currently.";
@@ -327,6 +375,53 @@ TEST_F(ShellTest, ToImageRetries) {
327375
message_latch.Wait();
328376
DestroyShell(std::move(shell), task_runners);
329377
}
378+
TEST_F(ShellTest, ToImageRetryOverflow) {
379+
#ifndef FML_OS_MACOSX
380+
GTEST_SKIP() << "Only works on macos currently.";
381+
#endif
382+
Settings settings = CreateSettingsForFixture();
383+
settings.enable_impeller = true;
384+
TaskRunners task_runners("test", // label
385+
GetCurrentTaskRunner(), // platform
386+
CreateNewThread(), // raster
387+
CreateNewThread(), // ui
388+
CreateNewThread() // io
389+
);
390+
391+
std::unique_ptr<Shell> shell = CreateShell({
392+
.settings = settings,
393+
.task_runners = task_runners,
394+
});
395+
396+
auto turn_off_gpu = [&](Dart_NativeArguments args) {
397+
auto handle = Dart_GetNativeArgument(args, 0);
398+
bool value = true;
399+
ASSERT_TRUE(Dart_IsBoolean(handle));
400+
Dart_BooleanValue(handle, &value);
401+
TurnOffGPU(shell.get(), value);
402+
};
403+
404+
AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu));
405+
406+
auto validate_not_null = [&](Dart_NativeArguments args) {
407+
auto handle = Dart_GetNativeArgument(args, 0);
408+
EXPECT_FALSE(Dart_IsNull(handle));
409+
message_latch.Signal();
410+
};
411+
412+
AddNativeCallback("ValidateNotNull", CREATE_NATIVE_ENTRY(validate_not_null));
413+
414+
ASSERT_TRUE(shell->IsSetup());
415+
auto configuration = RunConfiguration::InferFromSettings(settings);
416+
configuration.SetEntrypoint("toImageRetryOverflows");
417+
418+
shell->RunEngine(std::move(configuration), [&](auto result) {
419+
ASSERT_EQ(result, Engine::RunStatus::Success);
420+
});
421+
422+
message_latch.Wait();
423+
DestroyShell(std::move(shell), task_runners);
424+
}
330425

331426
TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) {
332427
#ifndef FML_OS_MACOSX

0 commit comments

Comments
 (0)