diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index d0182f4580ab0..6056147ddc95f 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -306,6 +306,8 @@ FILE: ../../../flutter/lib/ui/painting/image.cc FILE: ../../../flutter/lib/ui/painting/image.h FILE: ../../../flutter/lib/ui/painting/image_decoder.cc FILE: ../../../flutter/lib/ui/painting/image_decoder.h +FILE: ../../../flutter/lib/ui/painting/image_decoder_test.cc +FILE: ../../../flutter/lib/ui/painting/image_decoder_test.h FILE: ../../../flutter/lib/ui/painting/image_decoder_unittests.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.h diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index c904141b338e8..235c1acdefdf9 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -162,7 +162,11 @@ if (current_toolchain == host_toolchain) { executable("ui_unittests") { testonly = true + configs += [ "//flutter:export_dynamic_symbols" ] + sources = [ + "painting/image_decoder_test.cc", + "painting/image_decoder_test.h", "painting/image_decoder_unittests.cc", "window/pointer_data_packet_converter_unittests.cc", ] @@ -171,8 +175,14 @@ if (current_toolchain == host_toolchain) { ":ui", ":ui_unittests_fixtures", "//flutter/common", + "//flutter/fml", + "//flutter/lib/snapshot", + "//flutter/runtime", + "//flutter/shell/common", "//flutter/testing:dart", "//flutter/testing:opengl", + "//flutter/third_party/tonic", + "//third_party/dart/runtime/bin:elf_loader", ] } } diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 6d604e30543bf..dfd68d7416c54 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -3,4 +3,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:ui'; + void main() {} + +@pragma('vm:entry-point') +void frameCallback(FrameInfo info) { + print('called back'); +} diff --git a/lib/ui/painting/image_decoder_test.cc b/lib/ui/painting/image_decoder_test.cc new file mode 100644 index 0000000000000..6075856e7f38e --- /dev/null +++ b/lib/ui/painting/image_decoder_test.cc @@ -0,0 +1,53 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/lib/ui/painting/image_decoder_test.h" + +namespace flutter { +namespace testing { + +ImageDecoderFixtureTest::ImageDecoderFixtureTest() + : native_resolver_(std::make_shared()), + assets_dir_(fml::OpenDirectory(GetFixturesPath(), + false, + fml::FilePermission::kRead)), + aot_symbols_(LoadELFSymbolFromFixturesIfNeccessary()) {} + +Settings ImageDecoderFixtureTest::CreateSettingsForFixture() { + Settings settings; + settings.leak_vm = false; + settings.task_observer_add = [](intptr_t, fml::closure) {}; + settings.task_observer_remove = [](intptr_t) {}; + settings.isolate_create_callback = [this]() { + native_resolver_->SetNativeResolverForIsolate(); + }; + settings.enable_observatory = false; + SetSnapshotsAndAssets(settings); + return settings; +} + +void ImageDecoderFixtureTest::SetSnapshotsAndAssets(Settings& settings) { + if (!assets_dir_.is_valid()) { + return; + } + + settings.assets_dir = assets_dir_.get(); + + // In JIT execution, all snapshots are present within the binary itself and + // don't need to be explicitly supplied by the embedder. In AOT, these + // snapshots will be present in the application AOT dylib. + if (DartVM::IsRunningPrecompiledCode()) { + FML_CHECK(PrepareSettingsForAOTWithSymbols(settings, aot_symbols_)); + } else { + settings.application_kernels = [this]() { + std::vector> kernel_mappings; + kernel_mappings.emplace_back( + fml::FileMapping::CreateReadOnly(assets_dir_, "kernel_blob.bin")); + return kernel_mappings; + }; + } +} + +} // namespace testing +} // namespace flutter diff --git a/lib/ui/painting/image_decoder_test.h b/lib/ui/painting/image_decoder_test.h new file mode 100644 index 0000000000000..5b7c58f49953e --- /dev/null +++ b/lib/ui/painting/image_decoder_test.h @@ -0,0 +1,39 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_LIB_UI_PAINTING_IMAGE_DECODER_TEST_H_ +#define FLUTTER_LIB_UI_PAINTING_IMAGE_DECODER_TEST_H_ + +#include + +#include "flutter/common/settings.h" +#include "flutter/runtime/dart_vm.h" +#include "flutter/testing/elf_loader.h" +#include "flutter/testing/test_dart_native_resolver.h" +#include "flutter/testing/testing.h" +#include "flutter/testing/thread_test.h" + +namespace flutter { +namespace testing { + +class ImageDecoderFixtureTest : public ThreadTest { + public: + ImageDecoderFixtureTest(); + + Settings CreateSettingsForFixture(); + + private: + std::shared_ptr native_resolver_; + fml::UniqueFD assets_dir_; + ELFAOTSymbols aot_symbols_; + + void SetSnapshotsAndAssets(Settings& settings); + + FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoderFixtureTest); +}; + +} // namespace testing +} // namespace flutter + +#endif // FLUTTER_LIB_UI_PAINTING_IMAGE_DECODER_TEST_H_ diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 288eb2abda1b0..94c008e2959d2 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -6,6 +6,13 @@ #include "flutter/fml/mapping.h" #include "flutter/fml/synchronization/waitable_event.h" #include "flutter/lib/ui/painting/image_decoder.h" +#include "flutter/lib/ui/painting/image_decoder_test.h" +#include "flutter/lib/ui/painting/multi_frame_codec.h" +#include "flutter/runtime/dart_vm.h" +#include "flutter/runtime/dart_vm_lifecycle.h" +#include "flutter/testing/dart_isolate_runner.h" +#include "flutter/testing/elf_loader.h" +#include "flutter/testing/test_dart_native_resolver.h" #include "flutter/testing/test_gl_surface.h" #include "flutter/testing/testing.h" #include "flutter/testing/thread_test.h" @@ -14,8 +21,6 @@ namespace flutter { namespace testing { -using ImageDecoderFixtureTest = ThreadTest; - class TestIOManager final : public IOManager { public: TestIOManager(fml::RefPtr task_runner, @@ -557,5 +562,96 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { assert_image(decode({}, 100)); } +TEST_F(ImageDecoderFixtureTest, + MultiFrameCodecCanBeCollectedBeforeIOTasksFinish) { + // This test verifies that the MultiFrameCodec safely shares state between + // tasks on the IO and UI runners, and does not allow unsafe memory access if + // the UI object is collected while the IO thread still has pending decode + // work. This could happen in a real application if the engine is collected + // while a multi-frame image is decoding. To exercise this, the test: + // - Starts a Dart VM + // - Latches the IO task runner + // - Create a MultiFrameCodec for an animated gif pointed to a callback + // in the Dart fixture + // - Calls getNextFrame on the UI task runner + // - Collects the MultiFrameCodec object before unlatching the IO task + // runner. + // - Unlatches the IO task runner + auto settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(settings); + auto vm_data = vm_ref.GetVMData(); + + auto gif_mapping = OpenFixtureAsSkData("hello_loop_2.gif"); + + ASSERT_TRUE(gif_mapping); + + auto gif_codec = SkCodec::MakeFromData(gif_mapping); + ASSERT_TRUE(gif_codec); + + TaskRunners runners(GetCurrentTestName(), // label + CreateNewThread("platform"), // platform + CreateNewThread("gpu"), // gpu + CreateNewThread("ui"), // ui + CreateNewThread("io") // io + ); + + fml::AutoResetWaitableEvent latch; + fml::AutoResetWaitableEvent io_latch; + std::unique_ptr io_manager; + + // Setup the IO manager. + runners.GetIOTaskRunner()->PostTask([&]() { + io_manager = std::make_unique(runners.GetIOTaskRunner()); + latch.Signal(); + }); + latch.Wait(); + + auto isolate = + RunDartCodeInIsolate(vm_ref, settings, runners, "main", {}, + GetFixturesPath(), io_manager->GetWeakIOManager()); + + // Latch the IO task runner. + runners.GetIOTaskRunner()->PostTask([&]() { io_latch.Wait(); }); + + runners.GetUITaskRunner()->PostTask([&]() { + fml::AutoResetWaitableEvent isolate_latch; + fml::RefPtr codec; + EXPECT_TRUE(isolate->RunInIsolateScope([&]() -> bool { + Dart_Handle library = Dart_RootLibrary(); + if (Dart_IsError(library)) { + isolate_latch.Signal(); + return false; + } + Dart_Handle closure = + Dart_GetField(library, Dart_NewStringFromCString("frameCallback")); + if (Dart_IsError(closure) || !Dart_IsClosure(closure)) { + isolate_latch.Signal(); + return false; + } + + codec = fml::MakeRefCounted(std::move(gif_codec)); + codec->getNextFrame(closure); + codec = nullptr; + isolate_latch.Signal(); + return true; + })); + isolate_latch.Wait(); + + EXPECT_FALSE(codec); + + io_latch.Signal(); + + latch.Signal(); + }); + latch.Wait(); + + // Destroy the IO manager + runners.GetIOTaskRunner()->PostTask([&]() { + io_manager.reset(); + latch.Signal(); + }); + latch.Wait(); +} + } // namespace testing } // namespace flutter diff --git a/lib/ui/painting/multi_frame_codec.cc b/lib/ui/painting/multi_frame_codec.cc index f650e25a97e61..ae2b4bdb83f02 100644 --- a/lib/ui/painting/multi_frame_codec.cc +++ b/lib/ui/painting/multi_frame_codec.cc @@ -12,13 +12,16 @@ namespace flutter { MultiFrameCodec::MultiFrameCodec(std::unique_ptr codec) + : state_(new State(std::move(codec))) {} + +MultiFrameCodec::~MultiFrameCodec() = default; + +MultiFrameCodec::State::State(std::unique_ptr codec) : codec_(std::move(codec)), frameCount_(codec_->getFrameCount()), repetitionCount_(codec_->getRepetitionCount()), nextFrameIndex_(0) {} -MultiFrameCodec::~MultiFrameCodec() = default; - static void InvokeNextFrameCallback( fml::RefPtr frameInfo, std::unique_ptr callback, @@ -70,7 +73,7 @@ static bool CopyToBitmap(SkBitmap* dst, return true; } -sk_sp MultiFrameCodec::GetNextFrameImage( +sk_sp MultiFrameCodec::State::GetNextFrameImage( fml::WeakPtr resourceContext) { SkBitmap bitmap = SkBitmap(); SkImageInfo info = codec_->getInfo().makeColorType(kN32_SkColorType); @@ -128,7 +131,7 @@ sk_sp MultiFrameCodec::GetNextFrameImage( } } -void MultiFrameCodec::GetNextFrameAndInvokeCallback( +void MultiFrameCodec::State::GetNextFrameAndInvokeCallback( std::unique_ptr callback, fml::RefPtr ui_task_runner, fml::WeakPtr resourceContext, @@ -167,9 +170,16 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) { task_runners.GetIOTaskRunner()->PostTask(fml::MakeCopyable( [callback = std::make_unique( tonic::DartState::Current(), callback_handle), - this, trace_id, ui_task_runner = task_runners.GetUITaskRunner(), + weak_state = std::weak_ptr(state_), trace_id, + ui_task_runner = task_runners.GetUITaskRunner(), io_manager = dart_state->GetIOManager()]() mutable { - GetNextFrameAndInvokeCallback( + auto state = weak_state.lock(); + if (!state) { + ui_task_runner->PostTask(fml::MakeCopyable( + [callback = std::move(callback)]() { callback->Clear(); })); + return; + } + state->GetNextFrameAndInvokeCallback( std::move(callback), std::move(ui_task_runner), io_manager->GetResourceContext(), io_manager->GetSkiaUnrefQueue(), trace_id); @@ -179,11 +189,11 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) { } int MultiFrameCodec::frameCount() const { - return frameCount_; + return state_->frameCount_; } int MultiFrameCodec::repetitionCount() const { - return repetitionCount_; + return state_->repetitionCount_; } } // namespace flutter diff --git a/lib/ui/painting/multi_frame_codec.h b/lib/ui/painting/multi_frame_codec.h index 0be63c2a57582..3ca317cbacadb 100644 --- a/lib/ui/painting/multi_frame_codec.h +++ b/lib/ui/painting/multi_frame_codec.h @@ -26,24 +26,44 @@ class MultiFrameCodec : public Codec { Dart_Handle getNextFrame(Dart_Handle args) override; private: - const std::unique_ptr codec_; - const int frameCount_; - const int repetitionCount_; - int nextFrameIndex_; - - // The last decoded frame that's required to decode any subsequent frames. - std::unique_ptr lastRequiredFrame_; - // The index of the last decoded required frame. - int lastRequiredFrameIndex_ = -1; - - sk_sp GetNextFrameImage(fml::WeakPtr resourceContext); - - void GetNextFrameAndInvokeCallback( - std::unique_ptr callback, - fml::RefPtr ui_task_runner, - fml::WeakPtr resourceContext, - fml::RefPtr unref_queue, - size_t trace_id); + // Captures the state shared between the IO and UI task runners. + // + // The state is initialized on the UI task runner when the Dart object is + // created. Decoding occurs on the IO task runner. Since it is possible for + // the UI object to be collected independently of the IO task runner work, + // it is not safe for this state to live directly on the MultiFrameCodec. + // Instead, the MultiFrameCodec creates this object when it is constructed, + // shares it with the IO task runner's decoding work, and sets the live_ + // member to false when it is destructed. + struct State { + State(std::unique_ptr codec); + + const std::unique_ptr codec_; + const int frameCount_; + const int repetitionCount_; + + // The non-const members and functions below here are only read or written + // to on the IO thread. They are not safe to access or write on the UI + // thread. + int nextFrameIndex_; + // The last decoded frame that's required to decode any subsequent frames. + std::unique_ptr lastRequiredFrame_; + + // The index of the last decoded required frame. + int lastRequiredFrameIndex_ = -1; + + sk_sp GetNextFrameImage(fml::WeakPtr resourceContext); + + void GetNextFrameAndInvokeCallback( + std::unique_ptr callback, + fml::RefPtr ui_task_runner, + fml::WeakPtr resourceContext, + fml::RefPtr unref_queue, + size_t trace_id); + }; + + // Shared across the UI and IO task runners. + std::shared_ptr state_; FML_FRIEND_MAKE_REF_COUNTED(MultiFrameCodec); FML_FRIEND_REF_COUNTED_THREAD_SAFE(MultiFrameCodec);