Preserve specified AssetResolvers when performing a hot restart or up…

…dating the asset directory (flutter#21611) Follow up from flutter#21436 . That PR works for all embeddings except for Android, which creates a special JNI AssetResolver. Since the shell cannot recreate this resolver, update the logic to preserve existing resolvers instead.
chaselatta · Oct 8, 2020 · 15c5874 · 15c5874
1 parent 4c6f2ad
commit 15c5874
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 31 deletions.
diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc
@@ -29,6 +29,10 @@ void AssetManager::PushBack(std::unique_ptr<AssetResolver> resolver) {
   resolvers_.push_back(std::move(resolver));
 }
 
+std::deque<std::unique_ptr<AssetResolver>> AssetManager::TakeResolvers() {
+  return std::move(resolvers_);
+}
+
 // |AssetResolver|
 std::unique_ptr<fml::Mapping> AssetManager::GetAsMapping(
     const std::string& asset_name) const {
@@ -52,4 +56,9 @@ bool AssetManager::IsValid() const {
   return resolvers_.size() > 0;
 }
 
+// |AssetResolver|
+bool AssetManager::IsValidAfterAssetManagerChange() const {
+  return false;
+}
+
 }  // namespace flutter
diff --git a/assets/asset_manager.h b/assets/asset_manager.h
@@ -25,9 +25,14 @@ class AssetManager final : public AssetResolver {
 
   void PushBack(std::unique_ptr<AssetResolver> resolver);
 
+  std::deque<std::unique_ptr<AssetResolver>> TakeResolvers();
+
   // |AssetResolver|
   bool IsValid() const override;
 
+  // |AssetResolver|
+  bool IsValidAfterAssetManagerChange() const override;
+
   // |AssetResolver|
   std::unique_ptr<fml::Mapping> GetAsMapping(
       const std::string& asset_name) const override;

diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h
@@ -21,6 +21,24 @@ class AssetResolver {
 
   virtual bool IsValid() const = 0;
 
+  //----------------------------------------------------------------------------
+  /// @brief      Certain asset resolvers are still valid after the asset
+  ///             manager is replaced before a hot reload, or after a new run
+  ///             configuration is created during a hot restart. By preserving
+  ///             these resolvers and re-inserting them into the new resolver or
+  ///             run configuration, the tooling can avoid needing to sync all
+  ///             application assets through the Dart devFS upon connecting to
+  ///             the VM Service. Besides improving the startup performance of
+  ///             running a Flutter application, it also reduces the occurance
+  ///             of tool failures due to repeated network flakes caused by
+  ///             damaged cables or hereto unknown bugs in the Dart HTTP server
+  ///             implementation.
+  ///
+  /// @return     Returns whether this resolver is valid after the asset manager
+  ///             or run configuration is updated.
+  ///
+  virtual bool IsValidAfterAssetManagerChange() const = 0;
+
   [[nodiscard]] virtual std::unique_ptr<fml::Mapping> GetAsMapping(
       const std::string& asset_name) const = 0;
 

diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc
@@ -12,11 +12,14 @@
 
 namespace flutter {
 
-DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor)
+DirectoryAssetBundle::DirectoryAssetBundle(
+    fml::UniqueFD descriptor,
+    bool is_valid_after_asset_manager_change)
     : descriptor_(std::move(descriptor)) {
   if (!fml::IsDirectory(descriptor_)) {
     return;
   }
+  is_valid_after_asset_manager_change_ = is_valid_after_asset_manager_change;
   is_valid_ = true;
 }
 
@@ -27,6 +30,11 @@ bool DirectoryAssetBundle::IsValid() const {
   return is_valid_;
 }
 
+// |AssetResolver|
+bool DirectoryAssetBundle::IsValidAfterAssetManagerChange() const {
+  return is_valid_after_asset_manager_change_;
+}
+
 // |AssetResolver|
 std::unique_ptr<fml::Mapping> DirectoryAssetBundle::GetAsMapping(
     const std::string& asset_name) const {

diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h
@@ -14,17 +14,22 @@ namespace flutter {
 
 class DirectoryAssetBundle : public AssetResolver {
  public:
-  explicit DirectoryAssetBundle(fml::UniqueFD descriptor);
+  DirectoryAssetBundle(fml::UniqueFD descriptor,
+                       bool is_valid_after_asset_manager_change);
 
   ~DirectoryAssetBundle() override;
 
  private:
   const fml::UniqueFD descriptor_;
   bool is_valid_ = false;
+  bool is_valid_after_asset_manager_change_ = false;
 
   // |AssetResolver|
   bool IsValid() const override;
 
+  // |AssetResolver|
+  bool IsValidAfterAssetManagerChange() const override;
+
   // |AssetResolver|
   std::unique_ptr<fml::Mapping> GetAsMapping(
       const std::string& asset_name) const override;

diff --git a/shell/common/engine.cc b/shell/common/engine.cc
@@ -106,6 +106,10 @@ void Engine::SetupDefaultFontManager() {
   font_collection_.SetupDefaultFontManager();
 }
 
+std::shared_ptr<AssetManager> Engine::GetAssetManager() {
+  return asset_manager_;
+}
+
 bool Engine::UpdateAssetManager(
     std::shared_ptr<AssetManager> new_asset_manager) {
   if (asset_manager_ == new_asset_manager) {

diff --git a/shell/common/engine.h b/shell/common/engine.h
@@ -734,6 +734,9 @@ class Engine final : public RuntimeDelegate,
   // |RuntimeDelegate|
   FontCollection& GetFontCollection() override;
 
+  // Return the asset manager associated with the current engine, or nullptr.
+  std::shared_ptr<AssetManager> GetAssetManager();
+
   // |PointerDataDispatcher::Delegate|
   void DoDispatchPacket(std::unique_ptr<PointerDataPacket> packet,
                         uint64_t trace_flow_id) override;

diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc
@@ -185,9 +185,10 @@ TEST_F(ShellTest, CanLoadSkSLsFromAsset) {
   ResetAssetManager();
   auto asset_manager = std::make_shared<AssetManager>();
   RunConfiguration config(nullptr, asset_manager);
-  asset_manager->PushBack(
-      std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
-          asset_dir.path().c_str(), false, fml::FilePermission::kRead)));
+  asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
+      fml::OpenDirectory(asset_dir.path().c_str(), false,
+                         fml::FilePermission::kRead),
+      false));
   CheckTwoSkSLsAreLoaded();
 
   // 3rd, test the content of the SkSLs in the asset.

diff --git a/shell/common/run_configuration.cc b/shell/common/run_configuration.cc
@@ -21,12 +21,13 @@ RunConfiguration RunConfiguration::InferFromSettings(
 
   if (fml::UniqueFD::traits_type::IsValid(settings.assets_dir)) {
     asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
-        fml::Duplicate(settings.assets_dir)));
+        fml::Duplicate(settings.assets_dir), true));
   }
 
-  asset_manager->PushBack(
-      std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
-          settings.assets_path.c_str(), false, fml::FilePermission::kRead)));
+  asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
+      fml::OpenDirectory(settings.assets_path.c_str(), false,
+                         fml::FilePermission::kRead),
+      true));
 
   return {IsolateConfiguration::InferFromSettings(settings, asset_manager,
                                                   io_worker),

diff --git a/shell/common/shell.cc b/shell/common/shell.cc
@@ -1401,10 +1401,21 @@ bool Shell::OnServiceProtocolRunInView(
   configuration.SetEntrypointAndLibrary(engine_->GetLastEntrypoint(),
                                         engine_->GetLastEntrypointLibrary());
 
-  configuration.AddAssetResolver(
-      std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
-          asset_directory_path.c_str(), false, fml::FilePermission::kRead)));
-  configuration.AddAssetResolver(RestoreOriginalAssetResolver());
+  configuration.AddAssetResolver(std::make_unique<DirectoryAssetBundle>(
+      fml::OpenDirectory(asset_directory_path.c_str(), false,
+                         fml::FilePermission::kRead),
+      false));
+
+  // Preserve any original asset resolvers to avoid syncing unchanged assets
+  // over the DevFS connection.
+  auto old_asset_manager = engine_->GetAssetManager();
+  if (old_asset_manager != nullptr) {
+    for (auto& old_resolver : old_asset_manager->TakeResolvers()) {
+      if (old_resolver->IsValidAfterAssetManagerChange()) {
+        configuration.AddAssetResolver(std::move(old_resolver));
+      }
+    }
+  }
 
   auto& allocator = response->GetAllocator();
   response->SetObject();
@@ -1517,10 +1528,21 @@ bool Shell::OnServiceProtocolSetAssetBundlePath(
 
   auto asset_manager = std::make_shared<AssetManager>();
 
-  asset_manager->PushFront(RestoreOriginalAssetResolver());
   asset_manager->PushFront(std::make_unique<DirectoryAssetBundle>(
       fml::OpenDirectory(params.at("assetDirectory").data(), false,
-                         fml::FilePermission::kRead)));
+                         fml::FilePermission::kRead),
+      false));
+
+  // Preserve any original asset resolvers to avoid syncing unchanged assets
+  // over the DevFS connection.
+  auto old_asset_manager = engine_->GetAssetManager();
+  if (old_asset_manager != nullptr) {
+    for (auto& old_resolver : old_asset_manager->TakeResolvers()) {
+      if (old_resolver->IsValidAfterAssetManagerChange()) {
+        asset_manager->PushBack(std::move(old_resolver));
+      }
+    }
+  }
 
   if (engine_->UpdateAssetManager(std::move(asset_manager))) {
     response->AddMember("type", "Success", allocator);
@@ -1624,16 +1646,4 @@ void Shell::OnDisplayUpdates(DisplayUpdateType update_type,
   display_manager_->HandleDisplayUpdates(update_type, displays);
 }
 
-// Add the original asset directory to the resolvers so that unmodified assets
-// bundled with the application specific format (APK, IPA) can be used without
-// syncing to the Dart devFS.
-std::unique_ptr<DirectoryAssetBundle> Shell::RestoreOriginalAssetResolver() {
-  if (fml::UniqueFD::traits_type::IsValid(settings_.assets_dir)) {
-    return std::make_unique<DirectoryAssetBundle>(
-        fml::Duplicate(settings_.assets_dir));
-  }
-  return std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
-      settings_.assets_path.c_str(), false, fml::FilePermission::kRead));
-};
-
 }  // namespace flutter
diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc
@@ -27,6 +27,10 @@ bool APKAssetProvider::IsValid() const {
   return true;
 }
 
+bool APKAssetProvider::IsValidAfterAssetManagerChange() const {
+  return true;
+}
+
 class APKAssetMapping : public fml::Mapping {
  public:
   APKAssetMapping(AAsset* asset) : asset_(asset) {}

diff --git a/shell/platform/android/apk_asset_provider.h b/shell/platform/android/apk_asset_provider.h
@@ -29,6 +29,9 @@ class APKAssetProvider final : public AssetResolver {
   // |flutter::AssetResolver|
   bool IsValid() const override;
 
+  // |flutter::AssetResolver|
+  bool IsValidAfterAssetManagerChange() const override;
+
   // |flutter::AssetResolver|
   std::unique_ptr<fml::Mapping> GetAsMapping(
       const std::string& asset_name) const override;

diff --git a/shell/testing/tester_main.cc b/shell/testing/tester_main.cc
@@ -192,10 +192,11 @@ int RunTester(const flutter::Settings& settings,
 
   auto asset_manager = std::make_shared<flutter::AssetManager>();
   asset_manager->PushBack(std::make_unique<flutter::DirectoryAssetBundle>(
-      fml::Duplicate(settings.assets_dir)));
-  asset_manager->PushBack(
-      std::make_unique<flutter::DirectoryAssetBundle>(fml::OpenDirectory(
-          settings.assets_path.c_str(), false, fml::FilePermission::kRead)));
+      fml::Duplicate(settings.assets_dir), true));
+  asset_manager->PushBack(std::make_unique<flutter::DirectoryAssetBundle>(
+      fml::OpenDirectory(settings.assets_path.c_str(), false,
+                         fml::FilePermission::kRead),
+      true));
 
   RunConfiguration run_configuration(std::move(isolate_configuration),
                                      std::move(asset_manager));