Avoid capturing this unsafely in MultiFrameCodec (flutter#16824)

Author: dnfield

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

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",
     ]
   }
 }

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');
+}

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<TestDartNativeResolver>()),
+      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<std::unique_ptr<const fml::Mapping>> kernel_mappings;
+      kernel_mappings.emplace_back(
+          fml::FileMapping::CreateReadOnly(assets_dir_, "kernel_blob.bin"));
+      return kernel_mappings;
+    };
+  }
+}
+
+}  // namespace testing
+}  // namespace flutter

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 <memory>
+
+#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<TestDartNativeResolver> 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_

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<fml::TaskRunner> 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<TestIOManager> io_manager;
+
+  // Setup the IO manager.
+  runners.GetIOTaskRunner()->PostTask([&]() {
+    io_manager = std::make_unique<TestIOManager>(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<MultiFrameCodec> 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<MultiFrameCodec>(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

lib/ui/painting/multi_frame_codec.cc

@@ -12,13 +12,16 @@
 namespace flutter {
 
 MultiFrameCodec::MultiFrameCodec(std::unique_ptr<SkCodec> codec)
+    : state_(new State(std::move(codec))) {}
+
+MultiFrameCodec::~MultiFrameCodec() = default;
+
+MultiFrameCodec::State::State(std::unique_ptr<SkCodec> codec)
     : codec_(std::move(codec)),
       frameCount_(codec_->getFrameCount()),
       repetitionCount_(codec_->getRepetitionCount()),
       nextFrameIndex_(0) {}
 
-MultiFrameCodec::~MultiFrameCodec() = default;
-
 static void InvokeNextFrameCallback(
     fml::RefPtr<FrameInfo> frameInfo,
     std::unique_ptr<DartPersistentValue> callback,
@@ -70,7 +73,7 @@ static bool CopyToBitmap(SkBitmap* dst,
   return true;
 }
 
-sk_sp<SkImage> MultiFrameCodec::GetNextFrameImage(
+sk_sp<SkImage> MultiFrameCodec::State::GetNextFrameImage(
     fml::WeakPtr<GrContext> resourceContext) {
   SkBitmap bitmap = SkBitmap();
   SkImageInfo info = codec_->getInfo().makeColorType(kN32_SkColorType);
@@ -128,7 +131,7 @@ sk_sp<SkImage> MultiFrameCodec::GetNextFrameImage(
   }
 }
 
-void MultiFrameCodec::GetNextFrameAndInvokeCallback(
+void MultiFrameCodec::State::GetNextFrameAndInvokeCallback(
     std::unique_ptr<DartPersistentValue> callback,
     fml::RefPtr<fml::TaskRunner> ui_task_runner,
     fml::WeakPtr<GrContext> resourceContext,
@@ -167,9 +170,16 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) {
   task_runners.GetIOTaskRunner()->PostTask(fml::MakeCopyable(
       [callback = std::make_unique<DartPersistentValue>(
            tonic::DartState::Current(), callback_handle),
-       this, trace_id, ui_task_runner = task_runners.GetUITaskRunner(),
+       weak_state = std::weak_ptr<MultiFrameCodec::State>(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

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<SkCodec> codec_;
-  const int frameCount_;
-  const int repetitionCount_;
-  int nextFrameIndex_;
-
-  // The last decoded frame that's required to decode any subsequent frames.
-  std::unique_ptr<SkBitmap> lastRequiredFrame_;
-  // The index of the last decoded required frame.
-  int lastRequiredFrameIndex_ = -1;
-
-  sk_sp<SkImage> GetNextFrameImage(fml::WeakPtr<GrContext> resourceContext);
-
-  void GetNextFrameAndInvokeCallback(
-      std::unique_ptr<DartPersistentValue> callback,
-      fml::RefPtr<fml::TaskRunner> ui_task_runner,
-      fml::WeakPtr<GrContext> resourceContext,
-      fml::RefPtr<flutter::SkiaUnrefQueue> 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<SkCodec> codec);
+
+    const std::unique_ptr<SkCodec> 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<SkBitmap> lastRequiredFrame_;
+
+    // The index of the last decoded required frame.
+    int lastRequiredFrameIndex_ = -1;
+
+    sk_sp<SkImage> GetNextFrameImage(fml::WeakPtr<GrContext> resourceContext);
+
+    void GetNextFrameAndInvokeCallback(
+        std::unique_ptr<DartPersistentValue> callback,
+        fml::RefPtr<fml::TaskRunner> ui_task_runner,
+        fml::WeakPtr<GrContext> resourceContext,
+        fml::RefPtr<flutter::SkiaUnrefQueue> unref_queue,
+        size_t trace_id);
+  };
+
+  // Shared across the UI and IO task runners.
+  std::shared_ptr<State> state_;
 
   FML_FRIEND_MAKE_REF_COUNTED(MultiFrameCodec);
   FML_FRIEND_REF_COUNTED_THREAD_SAFE(MultiFrameCodec);