Reland: Move asset opening to background thread, fix dart persistent value destruction

fml/concurrent_message_loop.cc

@@ -170,4 +170,14 @@ void ConcurrentTaskRunner::PostTask(const fml::closure& task) {
   task();
 }
 
+bool ConcurrentMessageLoop::RunsTasksOnCurrentThread() {
+  std::scoped_lock lock(tasks_mutex_);
+  for (const auto& worker_thread_id : worker_thread_ids_) {
+    if (worker_thread_id == std::this_thread::get_id()) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace fml

fml/concurrent_message_loop.h

@@ -34,6 +34,8 @@ class ConcurrentMessageLoop
 
   void PostTaskToAllWorkers(const fml::closure& task);
 
+  bool RunsTasksOnCurrentThread();
+
  private:
   friend ConcurrentTaskRunner;
 

lib/ui/painting.dart

@@ -6294,7 +6294,12 @@ class ImmutableBuffer extends NativeFieldWrapperClass1 {
     final ImmutableBuffer instance = ImmutableBuffer._(0);
     return _futurize((_Callback<int> callback) {
       return instance._initFromAsset(encodedKey, callback);
-    }).then((int length) => instance.._length = length);
+    }).then((int length) {
+      if (length == -1) {
+        throw Exception('Asset not found');
+      }
+      return instance.._length = length;
+    });
   }
 
   /// Create a buffer from the file with [path].

lib/ui/painting/immutable_buffer.cc

@@ -43,7 +43,7 @@ Dart_Handle ImmutableBuffer::init(Dart_Handle buffer_handle,
   return Dart_Null();
 }
 
-Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle buffer_handle,
+Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle raw_buffer_handle,
                                            Dart_Handle asset_name_handle,
                                            Dart_Handle callback_handle) {
   UIDartState::ThrowIfUIOperationsProhibited();
@@ -62,21 +62,60 @@ Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle buffer_handle,
   std::string asset_name = std::string{reinterpret_cast<const char*>(chars),
                                        static_cast<size_t>(asset_length)};
 
-  std::shared_ptr<AssetManager> asset_manager = UIDartState::Current()
-                                                    ->platform_configuration()
-                                                    ->client()
-                                                    ->GetAssetManager();
-  std::unique_ptr<fml::Mapping> data = asset_manager->GetAsMapping(asset_name);
-  if (data == nullptr) {
-    return tonic::ToDart("Asset not found");
-  }
+  auto* dart_state = UIDartState::Current();
+  auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner();
+  auto* buffer_callback_ptr =
+      new tonic::DartPersistentValue(dart_state, callback_handle);
+  auto* buffer_handle_ptr =
+      new tonic::DartPersistentValue(dart_state, raw_buffer_handle);
+  auto asset_manager = UIDartState::Current()
+                           ->platform_configuration()
+                           ->client()
+                           ->GetAssetManager();
 
-  auto size = data->GetSize();
-  const void* bytes = static_cast<const void*>(data->GetMapping());
-  auto sk_data = MakeSkDataWithCopy(bytes, size);
-  auto buffer = fml::MakeRefCounted<ImmutableBuffer>(sk_data);
-  buffer->AssociateWithDartWrapper(buffer_handle);
-  tonic::DartInvoke(callback_handle, {tonic::ToDart(size)});
+  auto ui_task = fml::MakeCopyable(
+      [buffer_callback_ptr, buffer_handle_ptr](const sk_sp<SkData>& sk_data,
+                                               size_t buffer_size) mutable {
+        std::unique_ptr<tonic::DartPersistentValue> buffer_handle(
+            buffer_handle_ptr);
+        std::unique_ptr<tonic::DartPersistentValue> buffer_callback(
+            buffer_callback_ptr);
+
+        auto dart_state = buffer_callback->dart_state().lock();
+        if (!dart_state) {
+          return;
+        }
+        tonic::DartState::Scope scope(dart_state);
+
+        if (!sk_data) {
+          // -1 is used as a sentinel that the file could not be opened.
+          tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(-1)});
+          return;
+        }
+        auto buffer = fml::MakeRefCounted<ImmutableBuffer>(sk_data);
+        buffer->AssociateWithDartWrapper(buffer_handle->Get());
+        tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(buffer_size)});
+      });
+
+  dart_state->GetConcurrentTaskRunner()->PostTask(
+      [asset_name = std::move(asset_name),
+       asset_manager = std::move(asset_manager),
+       ui_task_runner = std::move(ui_task_runner), ui_task] {
+        std::unique_ptr<fml::Mapping> mapping =
+            asset_manager->GetAsMapping(asset_name);
+
+        sk_sp<SkData> sk_data;
+        size_t buffer_size = 0;
+        if (mapping != nullptr) {
+          buffer_size = mapping->GetSize();
+          const void* bytes = static_cast<const void*>(mapping->GetMapping());
+          sk_data = MakeSkDataWithCopy(bytes, buffer_size);
+        }
+        ui_task_runner->PostTask(
+            [sk_data = std::move(sk_data), ui_task = ui_task, buffer_size]() {
+              ui_task(sk_data, buffer_size);
+            });
+      });
   return Dart_Null();
 }
 
@@ -101,20 +140,24 @@ Dart_Handle ImmutableBuffer::initFromFile(Dart_Handle raw_buffer_handle,
 
   auto* dart_state = UIDartState::Current();
   auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner();
-  auto buffer_callback =
-      std::make_unique<tonic::DartPersistentValue>(dart_state, callback_handle);
-  auto buffer_handle = std::make_unique<tonic::DartPersistentValue>(
-      dart_state, raw_buffer_handle);
+  auto* buffer_callback_ptr =
+      new tonic::DartPersistentValue(dart_state, callback_handle);
+  auto* buffer_handle_ptr =
+      new tonic::DartPersistentValue(dart_state, raw_buffer_handle);
 
   auto ui_task = fml::MakeCopyable(
-      [buffer_callback = std::move(buffer_callback),
-       buffer_handle = std::move(buffer_handle)](const sk_sp<SkData>& sk_data,
-                                                 size_t buffer_size) mutable {
+      [buffer_callback_ptr, buffer_handle_ptr](const sk_sp<SkData>& sk_data,
+                                               size_t buffer_size) mutable {
+        std::unique_ptr<tonic::DartPersistentValue> buffer_handle(
+            buffer_handle_ptr);
+        std::unique_ptr<tonic::DartPersistentValue> buffer_callback(
+            buffer_callback_ptr);
         auto dart_state = buffer_callback->dart_state().lock();
         if (!dart_state) {
           return;
         }
         tonic::DartState::Scope scope(dart_state);
+
         if (!sk_data) {
           // -1 is used as a sentinel that the file could not be opened.
           tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(-1)});

shell/common/fixtures/shell_test.dart

@@ -470,3 +470,11 @@ Future<void> testPluginUtilitiesCallbackHandle() async {
   }
   notifyNativeBool(true);
 }
+
+@pragma('vm:entry-point')
+Future<void> testThatAssetLoadingHappensOnWorkerThread() async {
+  try {
+    await ImmutableBuffer.fromAsset('DoesNotExist');
+  } catch (err) { /* Do nothing */ }
+  notifyNative();
+}

shell/common/shell_unittests.cc

@@ -19,6 +19,7 @@
 #include "flutter/flow/layers/layer_raster_cache_item.h"
 #include "flutter/flow/layers/platform_view_layer.h"
 #include "flutter/flow/layers/transform_layer.h"
+#include "flutter/fml/backtrace.h"
 #include "flutter/fml/command_line.h"
 #include "flutter/fml/dart/dart_converter.h"
 #include "flutter/fml/make_copyable.h"
@@ -194,6 +195,42 @@ class TestAssetResolver : public AssetResolver {
   AssetResolver::AssetResolverType type_;
 };
 
+class ThreadCheckingAssetResolver : public AssetResolver {
+ public:
+  explicit ThreadCheckingAssetResolver(
+      std::shared_ptr<fml::ConcurrentMessageLoop> concurrent_loop)
+      : concurrent_loop_(std::move(concurrent_loop)) {}
+
+  // |AssetResolver|
+  bool IsValid() const override { return true; }
+
+  // |AssetResolver|
+  bool IsValidAfterAssetManagerChange() const override { return true; }
+
+  // |AssetResolver|
+  AssetResolverType GetType() const {
+    return AssetResolverType::kApkAssetProvider;
+  }
+
+  // |AssetResolver|
+  std::unique_ptr<fml::Mapping> GetAsMapping(
+      const std::string& asset_name) const override {
+    if (asset_name == "FontManifest.json") {
+      // This file is loaded directly by the engine.
+      return nullptr;
+    }
+    mapping_requests.push_back(asset_name);
+    EXPECT_TRUE(concurrent_loop_->RunsTasksOnCurrentThread())
+        << fml::BacktraceHere();
+    return nullptr;
+  }
+
+  mutable std::vector<std::string> mapping_requests;
+
+ private:
+  std::shared_ptr<fml::ConcurrentMessageLoop> concurrent_loop_;
+};
+
 static bool ValidateShell(Shell* shell) {
   if (!shell) {
     return false;
@@ -3805,6 +3842,39 @@ TEST_F(ShellTest, SpawnWorksWithOnError) {
   ASSERT_FALSE(DartVMRef::IsInstanceRunning());
 }
 
+TEST_F(ShellTest, ImmutableBufferLoadsAssetOnBackgroundThread) {
+  Settings settings = CreateSettingsForFixture();
+  auto task_runner = CreateNewThread();
+  TaskRunners task_runners("test", task_runner, task_runner, task_runner,
+                           task_runner);
+  std::unique_ptr<Shell> shell = CreateShell(settings, task_runners);
+
+  fml::CountDownLatch latch(1);
+  AddNativeCallback("NotifyNative",
+                    CREATE_NATIVE_ENTRY([&](auto args) { latch.CountDown(); }));
+
+  // Create the surface needed by rasterizer
+  PlatformViewNotifyCreated(shell.get());
+
+  auto configuration = RunConfiguration::InferFromSettings(settings);
+  configuration.SetEntrypoint("testThatAssetLoadingHappensOnWorkerThread");
+  auto asset_manager = configuration.GetAssetManager();
+  auto test_resolver = std::make_unique<ThreadCheckingAssetResolver>(
+      shell->GetDartVM()->GetConcurrentMessageLoop());
+  auto leaked_resolver = test_resolver.get();
+  asset_manager->PushBack(std::move(test_resolver));
+
+  RunEngine(shell.get(), std::move(configuration));
+  PumpOneFrame(shell.get());
+
+  latch.Wait();
+
+  EXPECT_EQ(leaked_resolver->mapping_requests[0], "DoesNotExist");
+
+  PlatformViewNotifyDestroyed(shell.get());
+  DestroyShell(std::move(shell), task_runners);
+}
+
 TEST_F(ShellTest, PictureToImageSync) {
 #if !SHELL_ENABLE_GL
   // This test uses the GL backend.