From ff717e642c8f8d782adcbb1e6cab10c7c3cc1e24 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Sun, 4 Oct 2020 10:00:08 -0700 Subject: [PATCH 01/11] attempt sync throw from futurize --- lib/ui/painting.dart | 10 ++++++++-- testing/dart/codec_test.dart | 29 +++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index a8ebd7a632004..e878acfcfce43 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -5102,14 +5102,20 @@ typedef _Callbacker = String? Function(_Callback callback); /// ``` Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); + bool syncResult = true; final String? error = callbacker((T t) { - if (t == null) { + if (t == null && !syncResult) { + // If an error is thrown before the caller has a chance to attach a listener + // to the resulting future, it will instead end up being thrown to the + // Zones uncaught exception handler. completer.completeError(Exception('operation failed')); } else { completer.complete(t); } }); - if (error != null) + if (error != null) { throw Exception(error); + } + syncResult = false; return completer.future; } diff --git a/testing/dart/codec_test.dart b/testing/dart/codec_test.dart index 961f54f4ace6d..e8b976ddf4cd4 100644 --- a/testing/dart/codec_test.dart +++ b/testing/dart/codec_test.dart @@ -34,6 +34,35 @@ void main() { ); }); + test('Fails with invalid data and can be caught with try catch', () async { + await Future.delayed(const Duration(seconds: 10)); + final Uint8List data = Uint8List.fromList([1, 2, 3]); + + // Use try catch to prove it can be handled by "normal" code and not just + // package:test + try { + await ui.instantiateImageCodec(data); + fail('Should not have loaded'); + } catch (err) { + expect(err.toString(), contains('Invalid image data')); + } + }); + + test('Fails with invalid data and can be caught with catch error', () async { + final Uint8List data = Uint8List.fromList([1, 2, 3]); + + // Use catchError to prove it can be handled by "normal" code and not just + // package:test + await ui.instantiateImageCodec(data) + .then((ui.Codec codec) { + fail('Should not have loaded'); + }) + .catchError((dynamic err, StackTrace stackTrace) { + expect(err.toString(), contains('Invalid image data')); + }); + }); + + test('nextFrame', () async { final Uint8List data = await _getSkiaResource('test640x479.gif').readAsBytes(); final ui.Codec codec = await ui.instantiateImageCodec(data); From bfa9783b4cb039f44356b83f25f970207b3a72c5 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 5 Oct 2020 17:16:30 -0700 Subject: [PATCH 02/11] attempt to preserve specified asset managers on reload and restart --- assets/asset_manager.cc | 16 +++++++++++++ assets/asset_manager.h | 5 ++++ assets/asset_resolver.h | 11 +++++++++ assets/directory_asset_bundle.cc | 9 +++++++- assets/directory_asset_bundle.h | 6 ++++- shell/common/engine.cc | 4 ++++ shell/common/persistent_cache_unittests.cc | 7 +++--- shell/common/run_configuration.cc | 9 ++++---- shell/common/shell.cc | 24 +++++--------------- shell/platform/android/apk_asset_provider.cc | 4 ++++ shell/platform/android/apk_asset_provider.h | 3 +++ shell/testing/tester_main.cc | 9 ++++---- 12 files changed, 76 insertions(+), 31 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 60d169a31ebb2..ed7f7e564ad77 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -29,6 +29,17 @@ void AssetManager::PushBack(std::unique_ptr resolver) { resolvers_.push_back(std::move(resolver)); } +void AssetManager::TakeResolvers(std::shared_ptr manager) { + if (manager->resolvers_.size() > 0) { + for (int i = 0; i < manager->resolvers_.size(); i++) { + auto resolver = std::move(manager->resolvers_[i]); + if (resolver->ShouldPreserve()) { + resolvers_.push_back(std::move(resolver)); + } + } + } +} + // |AssetResolver| std::unique_ptr AssetManager::GetAsMapping( const std::string& asset_name) const { @@ -52,4 +63,9 @@ bool AssetManager::IsValid() const { return resolvers_.size() > 0; } +// |AssetResolver| +bool AssetManager::ShouldPreserve() const { + return false; +} + } // namespace flutter diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 2278742f50113..9e9ca598b1b3b 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -25,9 +25,14 @@ class AssetManager final : public AssetResolver { void PushBack(std::unique_ptr resolver); + void TakeResolvers(std::shared_ptr manager); + // |AssetResolver| bool IsValid() const override; + // |AssetResolver| + bool ShouldPreserve() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index b6cdb88b84ac0..6dcabee74e43d 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -21,6 +21,17 @@ class AssetResolver { virtual bool IsValid() const = 0; + //---------------------------------------------------------------------------- + /// @brief Certain asset resolvers need to be preserved when the asset + /// directory is updated or the isolate recreated. While some + /// could be recated from the settings object, the Android + /// specific asset resolvers require a reference to the JNI to + /// create. + /// + /// @return Returns whether this resolver should be preserved. + /// + virtual bool ShouldPreserve() const = 0; + [[nodiscard]] virtual std::unique_ptr GetAsMapping( const std::string& asset_name) const = 0; diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index 5ad7297313c99..ac6a94f271bfb 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -12,11 +12,13 @@ namespace flutter { -DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor) +DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor, + bool should_preserve) : descriptor_(std::move(descriptor)) { if (!fml::IsDirectory(descriptor_)) { return; } + should_preserve_ = should_preserve; is_valid_ = true; } @@ -27,6 +29,11 @@ bool DirectoryAssetBundle::IsValid() const { return is_valid_; } +// |AssetResolver| +bool DirectoryAssetBundle::ShouldPreserve() const { + return should_preserve_; +} + // |AssetResolver| std::unique_ptr DirectoryAssetBundle::GetAsMapping( const std::string& asset_name) const { diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index 0a0a94c7aba15..bbfde3e34b8e4 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -14,17 +14,21 @@ namespace flutter { class DirectoryAssetBundle : public AssetResolver { public: - explicit DirectoryAssetBundle(fml::UniqueFD descriptor); + explicit DirectoryAssetBundle(fml::UniqueFD descriptor, bool should_preserve); ~DirectoryAssetBundle() override; private: const fml::UniqueFD descriptor_; bool is_valid_ = false; + bool should_preserve_ = false; // |AssetResolver| bool IsValid() const override; + // |AssetResolver| + bool ShouldPreserve() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index d2384c03579cf..17c5c4978ed7a 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -112,11 +112,15 @@ bool Engine::UpdateAssetManager( return false; } + auto old_asset_manager = asset_manager_; asset_manager_ = new_asset_manager; if (!asset_manager_) { return false; } + if (old_asset_manager != nullptr) { + asset_manager_->TakeResolvers(old_asset_manager); + } // Using libTXT as the text engine. font_collection_.RegisterFonts(asset_manager_); diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc index 52977113e7cdc..94e22fa8ea089 100644 --- 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(); RunConfiguration config(nullptr, asset_manager); - asset_manager->PushBack( - std::make_unique(fml::OpenDirectory( - asset_dir.path().c_str(), false, fml::FilePermission::kRead))); + asset_manager->PushBack(std::make_unique( + 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 index 8f0966b6bc896..1b32d5c295fbd 100644 --- 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( - fml::Duplicate(settings.assets_dir))); + fml::Duplicate(settings.assets_dir), true)); } - asset_manager->PushBack( - std::make_unique(fml::OpenDirectory( - settings.assets_path.c_str(), false, fml::FilePermission::kRead))); + asset_manager->PushBack(std::make_unique( + 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 index 2da9e9786e385..84d45473da45a 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1401,10 +1401,10 @@ bool Shell::OnServiceProtocolRunInView( configuration.SetEntrypointAndLibrary(engine_->GetLastEntrypoint(), engine_->GetLastEntrypointLibrary()); - configuration.AddAssetResolver( - std::make_unique(fml::OpenDirectory( - asset_directory_path.c_str(), false, fml::FilePermission::kRead))); - configuration.AddAssetResolver(RestoreOriginalAssetResolver()); + configuration.AddAssetResolver(std::make_unique( + fml::OpenDirectory(asset_directory_path.c_str(), false, + fml::FilePermission::kRead), + false)); auto& allocator = response->GetAllocator(); response->SetObject(); @@ -1517,10 +1517,10 @@ bool Shell::OnServiceProtocolSetAssetBundlePath( auto asset_manager = std::make_shared(); - asset_manager->PushFront(RestoreOriginalAssetResolver()); asset_manager->PushFront(std::make_unique( fml::OpenDirectory(params.at("assetDirectory").data(), false, - fml::FilePermission::kRead))); + fml::FilePermission::kRead), + false)); if (engine_->UpdateAssetManager(std::move(asset_manager))) { response->AddMember("type", "Success", allocator); @@ -1624,16 +1624,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 Shell::RestoreOriginalAssetResolver() { - if (fml::UniqueFD::traits_type::IsValid(settings_.assets_dir)) { - return std::make_unique( - fml::Duplicate(settings_.assets_dir)); - } - return std::make_unique(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 index 49af5ffa0182a..d3a7d116b1258 100644 --- 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::ShouldPreserve() 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 index 6ff1e8da7b776..65a835edc7bcc 100644 --- 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 ShouldPreserve() const override; + // |flutter::AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/testing/tester_main.cc b/shell/testing/tester_main.cc index 0f0264ce9ebdb..464b51c147fdb 100644 --- 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(); asset_manager->PushBack(std::make_unique( - fml::Duplicate(settings.assets_dir))); - asset_manager->PushBack( - std::make_unique(fml::OpenDirectory( - settings.assets_path.c_str(), false, fml::FilePermission::kRead))); + fml::Duplicate(settings.assets_dir), true)); + asset_manager->PushBack(std::make_unique( + fml::OpenDirectory(settings.assets_path.c_str(), false, + fml::FilePermission::kRead), + true)); RunConfiguration run_configuration(std::move(isolate_configuration), std::move(asset_manager)); From 13fb0c1c6f4505ab40b0411ec5f7fabcea1049a5 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 5 Oct 2020 17:19:30 -0700 Subject: [PATCH 03/11] revert unrelated changes --- lib/ui/painting.dart | 10 ++-------- testing/dart/codec_test.dart | 29 ----------------------------- 2 files changed, 2 insertions(+), 37 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index e878acfcfce43..a8ebd7a632004 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -5102,20 +5102,14 @@ typedef _Callbacker = String? Function(_Callback callback); /// ``` Future _futurize(_Callbacker callbacker) { final Completer completer = Completer.sync(); - bool syncResult = true; final String? error = callbacker((T t) { - if (t == null && !syncResult) { - // If an error is thrown before the caller has a chance to attach a listener - // to the resulting future, it will instead end up being thrown to the - // Zones uncaught exception handler. + if (t == null) { completer.completeError(Exception('operation failed')); } else { completer.complete(t); } }); - if (error != null) { + if (error != null) throw Exception(error); - } - syncResult = false; return completer.future; } diff --git a/testing/dart/codec_test.dart b/testing/dart/codec_test.dart index e8b976ddf4cd4..961f54f4ace6d 100644 --- a/testing/dart/codec_test.dart +++ b/testing/dart/codec_test.dart @@ -34,35 +34,6 @@ void main() { ); }); - test('Fails with invalid data and can be caught with try catch', () async { - await Future.delayed(const Duration(seconds: 10)); - final Uint8List data = Uint8List.fromList([1, 2, 3]); - - // Use try catch to prove it can be handled by "normal" code and not just - // package:test - try { - await ui.instantiateImageCodec(data); - fail('Should not have loaded'); - } catch (err) { - expect(err.toString(), contains('Invalid image data')); - } - }); - - test('Fails with invalid data and can be caught with catch error', () async { - final Uint8List data = Uint8List.fromList([1, 2, 3]); - - // Use catchError to prove it can be handled by "normal" code and not just - // package:test - await ui.instantiateImageCodec(data) - .then((ui.Codec codec) { - fail('Should not have loaded'); - }) - .catchError((dynamic err, StackTrace stackTrace) { - expect(err.toString(), contains('Invalid image data')); - }); - }); - - test('nextFrame', () async { final Uint8List data = await _getSkiaResource('test640x479.gif').readAsBytes(); final ui.Codec codec = await ui.instantiateImageCodec(data); From 6810d05a17325f9a105054f8ec05769b83e74109 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 5 Oct 2020 17:48:26 -0700 Subject: [PATCH 04/11] Update asset_manager.cc --- assets/asset_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index ed7f7e564ad77..17bc86944a36c 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -31,7 +31,7 @@ void AssetManager::PushBack(std::unique_ptr resolver) { void AssetManager::TakeResolvers(std::shared_ptr manager) { if (manager->resolvers_.size() > 0) { - for (int i = 0; i < manager->resolvers_.size(); i++) { + for (unsigned long i = 0; i < manager->resolvers_.size(); i++) { auto resolver = std::move(manager->resolvers_[i]); if (resolver->ShouldPreserve()) { resolvers_.push_back(std::move(resolver)); From 27b200a39654d97fe16e53f47250607fc9b6788a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 14:28:03 -0700 Subject: [PATCH 05/11] hoist into shell --- assets/asset_manager.cc | 11 ++--------- assets/asset_manager.h | 2 +- shell/common/engine.cc | 8 ++++---- shell/common/engine.h | 3 +++ shell/common/shell.cc | 20 ++++++++++++++++++++ 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 17bc86944a36c..91c9b5e506772 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -29,15 +29,8 @@ void AssetManager::PushBack(std::unique_ptr resolver) { resolvers_.push_back(std::move(resolver)); } -void AssetManager::TakeResolvers(std::shared_ptr manager) { - if (manager->resolvers_.size() > 0) { - for (unsigned long i = 0; i < manager->resolvers_.size(); i++) { - auto resolver = std::move(manager->resolvers_[i]); - if (resolver->ShouldPreserve()) { - resolvers_.push_back(std::move(resolver)); - } - } - } +std::deque> AssetManager::TakeResolvers() { + return std::move(resolvers_); } // |AssetResolver| diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 9e9ca598b1b3b..1d5f381153951 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -25,7 +25,7 @@ class AssetManager final : public AssetResolver { void PushBack(std::unique_ptr resolver); - void TakeResolvers(std::shared_ptr manager); + std::deque> TakeResolvers(); // |AssetResolver| bool IsValid() const override; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 17c5c4978ed7a..348e69a00a9aa 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -106,21 +106,21 @@ void Engine::SetupDefaultFontManager() { font_collection_.SetupDefaultFontManager(); } +std::shared_ptr Engine::GetAssetManager() { + return asset_manager_; +} + bool Engine::UpdateAssetManager( std::shared_ptr new_asset_manager) { if (asset_manager_ == new_asset_manager) { return false; } - auto old_asset_manager = asset_manager_; asset_manager_ = new_asset_manager; if (!asset_manager_) { return false; } - if (old_asset_manager != nullptr) { - asset_manager_->TakeResolvers(old_asset_manager); - } // Using libTXT as the text engine. font_collection_.RegisterFonts(asset_manager_); diff --git a/shell/common/engine.h b/shell/common/engine.h index b8d98aa766291..c5bbfc42c9c74 100644 --- 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 GetAssetManager(); + // |PointerDataDispatcher::Delegate| void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) override; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 84d45473da45a..3921455d04972 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1406,6 +1406,16 @@ bool Shell::OnServiceProtocolRunInView( fml::FilePermission::kRead), false)); + // Preserve any original asset resolvers to avoid syncing unchanged assets + // over the DevFS connection. + auto old_resolvers = engine_->GetAssetManager()->TakeResolvers(); + for (uint64_t i = 0; i < old_resolvers.size(); i++) { + auto old_resolver = std::move(old_resolvers[i]); + if (old_resolver->ShouldPreserve()) { + configuration.AddAssetResolver(std::move(old_resolver)); + } + } + auto& allocator = response->GetAllocator(); response->SetObject(); if (engine_->Restart(std::move(configuration))) { @@ -1522,6 +1532,16 @@ bool Shell::OnServiceProtocolSetAssetBundlePath( fml::FilePermission::kRead), false)); + // Preserve any original asset resolvers to avoid syncing unchanged assets + // over the DevFS connection. + auto old_resolvers = engine_->GetAssetManager()->TakeResolvers(); + for (uint64_t i = 0; i < old_resolvers.size(); i++) { + auto old_resolver = std::move(old_resolvers[i]); + if (old_resolver->ShouldPreserve()) { + asset_manager->PushBack(std::move(old_resolver)); + } + } + if (engine_->UpdateAssetManager(std::move(asset_manager))) { response->AddMember("type", "Success", allocator); auto new_description = GetServiceProtocolDescription(); From 5a4d2abceb7105fe15241e51837560a216799569 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 16:36:54 -0700 Subject: [PATCH 06/11] use magic iterator logic --- shell/common/shell.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 3921455d04972..27bf351355c98 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1408,9 +1408,7 @@ bool Shell::OnServiceProtocolRunInView( // Preserve any original asset resolvers to avoid syncing unchanged assets // over the DevFS connection. - auto old_resolvers = engine_->GetAssetManager()->TakeResolvers(); - for (uint64_t i = 0; i < old_resolvers.size(); i++) { - auto old_resolver = std::move(old_resolvers[i]); + for (auto& old_resolver : engine_->GetAssetManager()->TakeResolvers()) if (old_resolver->ShouldPreserve()) { configuration.AddAssetResolver(std::move(old_resolver)); } @@ -1534,9 +1532,7 @@ bool Shell::OnServiceProtocolSetAssetBundlePath( // Preserve any original asset resolvers to avoid syncing unchanged assets // over the DevFS connection. - auto old_resolvers = engine_->GetAssetManager()->TakeResolvers(); - for (uint64_t i = 0; i < old_resolvers.size(); i++) { - auto old_resolver = std::move(old_resolvers[i]); + for (auto& old_resolver : engine_->GetAssetManager()->TakeResolvers()) if (old_resolver->ShouldPreserve()) { asset_manager->PushBack(std::move(old_resolver)); } From 980ecb70a475b772e6ea98763cbea7321eaedf55 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 16:46:56 -0700 Subject: [PATCH 07/11] actually compile --- shell/common/shell.cc | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 27bf351355c98..39b4644d94ce2 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1408,9 +1408,12 @@ bool Shell::OnServiceProtocolRunInView( // Preserve any original asset resolvers to avoid syncing unchanged assets // over the DevFS connection. - for (auto& old_resolver : engine_->GetAssetManager()->TakeResolvers()) - if (old_resolver->ShouldPreserve()) { - configuration.AddAssetResolver(std::move(old_resolver)); + auto old_asset_manager = engine_->GetAssetManager(); + if (old_asset_manager != nullptr) { + for (auto& old_resolver : old_asset_manager->TakeResolvers()) { + if (old_resolver->ShouldPreserve()) { + configuration.AddAssetResolver(std::move(old_resolver)); + } } } @@ -1532,9 +1535,12 @@ bool Shell::OnServiceProtocolSetAssetBundlePath( // Preserve any original asset resolvers to avoid syncing unchanged assets // over the DevFS connection. - for (auto& old_resolver : engine_->GetAssetManager()->TakeResolvers()) - if (old_resolver->ShouldPreserve()) { - asset_manager->PushBack(std::move(old_resolver)); + auto old_asset_manager = engine_->GetAssetManager(); + if (old_asset_manager != nullptr) { + for (auto& old_resolver : old_asset_manager->TakeResolvers()) { + if (old_resolver->ShouldPreserve()) { + asset_manager->PushBack(std::move(old_resolver)); + } } } From 2e94df61f0958fec625be4141dda64916f5c9fb0 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 17:17:50 -0700 Subject: [PATCH 08/11] more doc comments, rename to IsValidAfterAssetManagerChange --- assets/asset_resolver.h | 21 +++++++++++++------- assets/directory_asset_bundle.cc | 9 +++++---- assets/directory_asset_bundle.h | 6 +++--- shell/platform/android/apk_asset_provider.cc | 2 +- shell/platform/android/apk_asset_provider.h | 2 +- 5 files changed, 24 insertions(+), 16 deletions(-) diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 6dcabee74e43d..8b3e323218939 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -22,15 +22,22 @@ class AssetResolver { virtual bool IsValid() const = 0; //---------------------------------------------------------------------------- - /// @brief Certain asset resolvers need to be preserved when the asset - /// directory is updated or the isolate recreated. While some - /// could be recated from the settings object, the Android - /// specific asset resolvers require a reference to the JNI to - /// create. + /// @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 should be preserved. + /// @return Returns whether this resolver is valid after the asset manager + /// or run configuration is updated. /// - virtual bool ShouldPreserve() const = 0; + virtual bool IsValidAfterAssetManagerChange() const = 0; [[nodiscard]] virtual std::unique_ptr GetAsMapping( const std::string& asset_name) const = 0; diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index ac6a94f271bfb..fd6b85d751e3e 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -12,13 +12,14 @@ namespace flutter { -DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor, - bool should_preserve) +DirectoryAssetBundle::DirectoryAssetBundle( + fml::UniqueFD descriptor, + bool is_valid_after_asset_manager_change) : descriptor_(std::move(descriptor)) { if (!fml::IsDirectory(descriptor_)) { return; } - should_preserve_ = should_preserve; + is_valid_after_asset_manager_change_ = is_valid_after_asset_manager_change; is_valid_ = true; } @@ -30,7 +31,7 @@ bool DirectoryAssetBundle::IsValid() const { } // |AssetResolver| -bool DirectoryAssetBundle::ShouldPreserve() const { +bool DirectoryAssetBundle::IsValidAfterAssetManagerChange() const { return should_preserve_; } diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index bbfde3e34b8e4..53774bba4469f 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -14,20 +14,20 @@ namespace flutter { class DirectoryAssetBundle : public AssetResolver { public: - explicit DirectoryAssetBundle(fml::UniqueFD descriptor, bool should_preserve); + DirectoryAssetBundle(fml::UniqueFD descriptor, bool should_preserve); ~DirectoryAssetBundle() override; private: const fml::UniqueFD descriptor_; bool is_valid_ = false; - bool should_preserve_ = false; + bool is_valid_after_asset_manager_change_ = false; // |AssetResolver| bool IsValid() const override; // |AssetResolver| - bool ShouldPreserve() const override; + bool IsValidAfterAssetManagerChange() const override; // |AssetResolver| std::unique_ptr GetAsMapping( diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index d3a7d116b1258..73a4a26bd1d2d 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -27,7 +27,7 @@ bool APKAssetProvider::IsValid() const { return true; } -bool APKAssetProvider::ShouldPreserve() const { +bool APKAssetProvider::IsValidAfterAssetManagerChange() const { return true; } diff --git a/shell/platform/android/apk_asset_provider.h b/shell/platform/android/apk_asset_provider.h index 65a835edc7bcc..f457b6c4ecc16 100644 --- a/shell/platform/android/apk_asset_provider.h +++ b/shell/platform/android/apk_asset_provider.h @@ -30,7 +30,7 @@ class APKAssetProvider final : public AssetResolver { bool IsValid() const override; // |flutter::AssetResolver| - bool ShouldPreserve() const override; + bool IsValidAfterAssetManagerChange() const override; // |flutter::AssetResolver| std::unique_ptr GetAsMapping( From 5f920dac2440a5e6a91159789803a8a47e976c3f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 17:18:28 -0700 Subject: [PATCH 09/11] fix shell.cc --- shell/common/shell.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 39b4644d94ce2..c250ca25a8552 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1411,7 +1411,7 @@ bool Shell::OnServiceProtocolRunInView( auto old_asset_manager = engine_->GetAssetManager(); if (old_asset_manager != nullptr) { for (auto& old_resolver : old_asset_manager->TakeResolvers()) { - if (old_resolver->ShouldPreserve()) { + if (old_resolver->IsValidAfterAssetManagerChange()) { configuration.AddAssetResolver(std::move(old_resolver)); } } @@ -1538,7 +1538,7 @@ bool Shell::OnServiceProtocolSetAssetBundlePath( auto old_asset_manager = engine_->GetAssetManager(); if (old_asset_manager != nullptr) { for (auto& old_resolver : old_asset_manager->TakeResolvers()) { - if (old_resolver->ShouldPreserve()) { + if (old_resolver->IsValidAfterAssetManagerChange()) { asset_manager->PushBack(std::move(old_resolver)); } } From 9a8269f889c518d21473d630c302f5398ae89746 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 17:24:25 -0700 Subject: [PATCH 10/11] dont forget asset manager --- assets/asset_manager.cc | 2 +- assets/asset_manager.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 91c9b5e506772..0fbc28702e9f7 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -57,7 +57,7 @@ bool AssetManager::IsValid() const { } // |AssetResolver| -bool AssetManager::ShouldPreserve() const { +bool AssetManager::IsValidAfterAssetManagerChange() const { return false; } diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 1d5f381153951..0a0f0ff170d4c 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -31,7 +31,7 @@ class AssetManager final : public AssetResolver { bool IsValid() const override; // |AssetResolver| - bool ShouldPreserve() const override; + bool IsValidAfterAssetManagerChange() const override; // |AssetResolver| std::unique_ptr GetAsMapping( From c336ec705e2491b3f9cb92c4283fcca82cb38704 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Oct 2020 17:37:39 -0700 Subject: [PATCH 11/11] woops --- assets/directory_asset_bundle.cc | 2 +- assets/directory_asset_bundle.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index fd6b85d751e3e..d15a7e7373c15 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -32,7 +32,7 @@ bool DirectoryAssetBundle::IsValid() const { // |AssetResolver| bool DirectoryAssetBundle::IsValidAfterAssetManagerChange() const { - return should_preserve_; + return is_valid_after_asset_manager_change_; } // |AssetResolver| diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index 53774bba4469f..49b02cdd27c71 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -14,7 +14,8 @@ namespace flutter { class DirectoryAssetBundle : public AssetResolver { public: - DirectoryAssetBundle(fml::UniqueFD descriptor, bool should_preserve); + DirectoryAssetBundle(fml::UniqueFD descriptor, + bool is_valid_after_asset_manager_change); ~DirectoryAssetBundle() override;