From b43efd60b9c4a5f3011287437f8361a2cf025cb6 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 7 Jan 2021 13:14:22 -0800 Subject: [PATCH] During image decoding, avoid using smart pointers for DartWrappables that cross thread-boundaries. In https://github.com/flutter/engine/pull/19843, a regression was introduced where an image descriptor object that has a Dart peer could be collected on a concurrent task runner thread. Collection of such object needs to enter isolate scope which might still be active on the UI thread. [A comment in the original commit explicitly mentions][1] this and attempts to achieve the collection of the Dart wrappable on the UI task runner. However, the author missed that the object was present in the captures of a copyable closure. These captures may be released on any of the participating threads. To avoid the the indeterminism of the smart pointer being dropped on the concurrent task runner thread, this patch resorts to manually reference counting the descriptor. Fixes https://github.com/flutter/flutter/issues/72144 [1]: https://github.com/flutter/engine/pull/19843/files#diff-a3034d4a06133381b6b636d841ec98cb7cfce640f316dda9e71eda4d9e7872f6R227 --- lib/ui/painting/image_decoder.cc | 77 +++++++++++++--------- lib/ui/painting/image_decoder.h | 2 +- lib/ui/painting/image_decoder_unittests.cc | 12 ++-- 3 files changed, 54 insertions(+), 37 deletions(-) diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc index e679e4d296bb9..3d497f9aea82c 100644 --- a/lib/ui/painting/image_decoder.cc +++ b/lib/ui/painting/image_decoder.cc @@ -75,7 +75,7 @@ static sk_sp ResizeRasterImage(sk_sp image, } static sk_sp ImageFromDecompressedData( - fml::RefPtr descriptor, + ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow) { @@ -98,7 +98,7 @@ static sk_sp ImageFromDecompressedData( SkISize::Make(target_width, target_height), flow); } -sk_sp ImageFromCompressedData(fml::RefPtr descriptor, +sk_sp ImageFromCompressedData(ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow) { @@ -216,38 +216,55 @@ static SkiaGPUObject UploadRasterImage( return result; } -void ImageDecoder::Decode(fml::RefPtr descriptor, +void ImageDecoder::Decode(fml::RefPtr descriptor_ref_ptr, uint32_t target_width, uint32_t target_height, const ImageResult& callback) { TRACE_EVENT0("flutter", __FUNCTION__); fml::tracing::TraceFlow flow(__FUNCTION__); + // ImageDescriptors have Dart peers that must be collected on the UI thread. + // However, closures in MakeCopyable below capture the descriptor. The + // captures of copyable closures may be collected on any of the thread + // participating in task execution. + // + // To avoid this issue, we resort to manually reference counting the + // descriptor. Since all task flows invoke the `result` callback, the raw + // descriptor is retained in the beginning and released in the `result` + // callback. + // + // `ImageDecoder::Decode` itself is invoked on the UI thread, so the + // collection of the smart pointer from which we obtained the raw descriptor + // is fine in this scope. + auto raw_descriptor = descriptor_ref_ptr.get(); + raw_descriptor->AddRef(); + FML_DCHECK(callback); FML_DCHECK(runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); // Always service the callback (and cleanup the descriptor) on the UI thread. - auto result = [callback, descriptor, ui_runner = runners_.GetUITaskRunner()]( - SkiaGPUObject image, - fml::tracing::TraceFlow flow) { - ui_runner->PostTask( - fml::MakeCopyable([callback, descriptor, image = std::move(image), - flow = std::move(flow)]() mutable { - // We are going to terminate the trace flow here. Flows cannot - // terminate without a base trace. Add one explicitly. - TRACE_EVENT0("flutter", "ImageDecodeCallback"); - flow.End(); - callback(std::move(image)); - })); - }; - - if (!descriptor->data() || descriptor->data()->size() == 0) { + auto result = + [callback, raw_descriptor, ui_runner = runners_.GetUITaskRunner()]( + SkiaGPUObject image, fml::tracing::TraceFlow flow) { + ui_runner->PostTask(fml::MakeCopyable( + [callback, raw_descriptor, image = std::move(image), + flow = std::move(flow)]() mutable { + // We are going to terminate the trace flow here. Flows cannot + // terminate without a base trace. Add one explicitly. + TRACE_EVENT0("flutter", "ImageDecodeCallback"); + flow.End(); + callback(std::move(image)); + raw_descriptor->Release(); + })); + }; + + if (!raw_descriptor->data() || raw_descriptor->data()->size() == 0) { result({}, std::move(flow)); return; } concurrent_task_runner_->PostTask( - fml::MakeCopyable([descriptor, // + fml::MakeCopyable([raw_descriptor, // io_manager = io_manager_, // io_runner = runners_.GetIOTaskRunner(), // result, // @@ -258,16 +275,15 @@ void ImageDecoder::Decode(fml::RefPtr descriptor, // Step 1: Decompress the image. // On Worker. - auto decompressed = - descriptor->is_compressed() - ? ImageFromCompressedData(std::move(descriptor), // - target_width, // - target_height, // - flow) - : ImageFromDecompressedData(std::move(descriptor), // - target_width, // - target_height, // - flow); + auto decompressed = raw_descriptor->is_compressed() + ? ImageFromCompressedData(raw_descriptor, // + target_width, // + target_height, // + flow) + : ImageFromDecompressedData(raw_descriptor, // + target_width, // + target_height, // + flow); if (!decompressed) { FML_LOG(ERROR) << "Could not decompress image."; @@ -283,7 +299,8 @@ void ImageDecoder::Decode(fml::RefPtr descriptor, std::move(flow)]() mutable { if (!io_manager) { FML_LOG(ERROR) << "Could not acquire IO manager."; - return result({}, std::move(flow)); + result({}, std::move(flow)); + return; } // If the IO manager does not have a resource context, the caller diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h index dbcf4701d04ad..e5f2fba6df1ad 100644 --- a/lib/ui/painting/image_decoder.h +++ b/lib/ui/painting/image_decoder.h @@ -61,7 +61,7 @@ class ImageDecoder { FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoder); }; -sk_sp ImageFromCompressedData(fml::RefPtr descriptor, +sk_sp ImageFromCompressedData(ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow); diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 0a6dfbaed4dea..2e37af243f254 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -556,10 +556,10 @@ TEST(ImageDecoderTest, VerifySimpleDecoding) { auto descriptor = fml::MakeRefCounted(std::move(data), std::move(codec)); - ASSERT_EQ( - ImageFromCompressedData(descriptor, 6, 2, fml::tracing::TraceFlow("")) - ->dimensions(), - SkISize::Make(6, 2)); + ASSERT_EQ(ImageFromCompressedData(descriptor.get(), 6, 2, + fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(6, 2)); } TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { @@ -574,8 +574,8 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); auto decode = [descriptor](uint32_t target_width, uint32_t target_height) { - return ImageFromCompressedData(descriptor, target_width, target_height, - fml::tracing::TraceFlow("")); + return ImageFromCompressedData(descriptor.get(), target_width, + target_height, fml::tracing::TraceFlow("")); }; auto expected_data = OpenFixtureAsSkData("Horizontal.png");