From 1cf74939f4623609341f1dd9b69616da88482c58 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Mon, 1 May 2023 14:35:48 -0700 Subject: Reland "Use reference counted pointers for ApkAssets" This reverts commit cf6e79f809034386b04ae551db815ab087f76c0a Updates: Prepare the shared pointers for the whole operation at once instead of re-locking them on each iteration. Still a regression of about 5% for changing theme's AssetManager object, vs the original 40% + change the log message to a warning as it doesn't break the app Original comment: Use reference counted pointers for ApkAssets The primary reason for memory corruption is freed ApkAssets Java expected them to only be freed in the finalizers, but there are explicit close() calls now, destroying objects that are still in use in some AssetManager2 objects This CL makes sure those AssetManagers don't assume ApkAssets always exist, but instead tries to lock them in memory for any access It also adds logging in case of deleting an assets object with any weak pointers still existing. Those will get into the bugreports attached to related bugs to help with investigation. Benchmarks don't regress, and the device appears to be working. Given that the crashes used to be pretty rare, let's wait for any new reports or lack of those. + add a missing .clang-format file to the jni directory + enabled C++23 in the project that uses AssetManager headers Bug: 197260547 Bug: 276922628 Test: unit tests + boot + benchmarks Old change id: I495fd9e012fe370a1f725dbb0265b4ee1be8d805 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b3455190124b41e2deb1774d9c05b396b73b41a2) Merged-In: Id668fbcf07db17b09691a344c04e98df83006f97 Change-Id: Id668fbcf07db17b09691a344c04e98df83006f97 --- libs/androidfw/AssetManager2.cpp | 305 ++++++++++++++++++++++----------------- 1 file changed, 175 insertions(+), 130 deletions(-) (limited to 'libs/androidfw/AssetManager2.cpp') diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp index 68f5e4a88c7e..3a7c91ef5b08 100644 --- a/libs/androidfw/AssetManager2.cpp +++ b/libs/androidfw/AssetManager2.cpp @@ -91,13 +91,14 @@ struct FindEntryResult { StringPoolRef entry_string_ref; }; -AssetManager2::AssetManager2() { - memset(&configuration_, 0, sizeof(configuration_)); +AssetManager2::AssetManager2(ApkAssetsList apk_assets, const ResTable_config& configuration) + : configuration_(configuration) { + // Don't invalidate caches here as there's nothing cached yet. + SetApkAssets(apk_assets, false); } -bool AssetManager2::SetApkAssets(std::vector apk_assets, bool invalidate_caches) { - apk_assets_ = std::move(apk_assets); - BuildDynamicRefTable(); +bool AssetManager2::SetApkAssets(ApkAssetsList apk_assets, bool invalidate_caches) { + BuildDynamicRefTable(apk_assets); RebuildFilterList(); if (invalidate_caches) { InvalidateCaches(static_cast(-1)); @@ -105,7 +106,13 @@ bool AssetManager2::SetApkAssets(std::vector apk_assets, bool return true; } -void AssetManager2::BuildDynamicRefTable() { +bool AssetManager2::SetApkAssets(std::initializer_list apk_assets, + bool invalidate_caches) { + return SetApkAssets(ApkAssetsList(apk_assets.begin(), apk_assets.size()), invalidate_caches); +} + +void AssetManager2::BuildDynamicRefTable(ApkAssetsList apk_assets) { + apk_assets_.assign(apk_assets.begin(), apk_assets.end()); package_groups_.clear(); package_ids_.fill(0xff); @@ -116,16 +123,19 @@ void AssetManager2::BuildDynamicRefTable() { // Overlay resources are not directly referenced by an application so their resource ids // can change throughout the application's lifetime. Assign overlay package ids last. - std::vector sorted_apk_assets(apk_assets_); - std::stable_partition(sorted_apk_assets.begin(), sorted_apk_assets.end(), [](const ApkAssets* a) { - return !a->IsOverlay(); - }); + std::vector sorted_apk_assets; + sorted_apk_assets.reserve(apk_assets_.size()); + for (auto& asset : apk_assets) { + sorted_apk_assets.push_back(asset.get()); + } + std::stable_partition(sorted_apk_assets.begin(), sorted_apk_assets.end(), + [](auto a) { return !a->IsOverlay(); }); // The assets cookie must map to the position of the apk assets in the unsorted apk assets list. std::unordered_map apk_assets_cookies; - apk_assets_cookies.reserve(apk_assets_.size()); - for (size_t i = 0, n = apk_assets_.size(); i < n; i++) { - apk_assets_cookies[apk_assets_[i]] = static_cast(i); + apk_assets_cookies.reserve(apk_assets.size()); + for (size_t i = 0, n = apk_assets.size(); i < n; i++) { + apk_assets_cookies[apk_assets[i].get()] = static_cast(i); } // 0x01 is reserved for the android package. @@ -242,7 +252,8 @@ void AssetManager2::DumpToLog() const { std::string list; for (const auto& apk_assets : apk_assets_) { - base::StringAppendF(&list, "%s,", apk_assets->GetDebugName().c_str()); + auto assets = apk_assets.promote(); + base::StringAppendF(&list, "%s,", assets ? assets->GetDebugName().c_str() : "nullptr"); } LOG(INFO) << "ApkAssets: " << list; @@ -279,7 +290,8 @@ const ResStringPool* AssetManager2::GetStringPoolForCookie(ApkAssetsCookie cooki if (cookie < 0 || static_cast(cookie) >= apk_assets_.size()) { return nullptr; } - return apk_assets_[cookie]->GetLoadedArsc()->GetStringPool(); + auto assets = apk_assets_[cookie].promote(); + return assets ? assets->GetLoadedArsc()->GetStringPool() : nullptr; } const DynamicRefTable* AssetManager2::GetDynamicRefTableForPackage(uint32_t package_id) const { @@ -331,7 +343,11 @@ bool AssetManager2::GetOverlayablesToString(android::StringPiece package_name, std::string* out) const { uint8_t package_id = 0U; for (const auto& apk_assets : apk_assets_) { - const LoadedArsc* loaded_arsc = apk_assets->GetLoadedArsc(); + auto assets = apk_assets.promote(); + if (!assets) { + continue; + } + const LoadedArsc* loaded_arsc = assets->GetLoadedArsc(); if (loaded_arsc == nullptr) { continue; } @@ -384,8 +400,10 @@ bool AssetManager2::GetOverlayablesToString(android::StringPiece package_name, } bool AssetManager2::ContainsAllocatedTable() const { - return std::find_if(apk_assets_.begin(), apk_assets_.end(), - std::mem_fn(&ApkAssets::IsTableAllocated)) != apk_assets_.end(); + return std::find_if(apk_assets_.begin(), apk_assets_.end(), [](auto&& assets_weak) { + auto assets = assets_weak.promote(); + return assets && assets->IsTableAllocated(); + }) != apk_assets_.end(); } void AssetManager2::SetConfiguration(const ResTable_config& configuration) { @@ -398,8 +416,8 @@ void AssetManager2::SetConfiguration(const ResTable_config& configuration) { } } -std::set AssetManager2::GetNonSystemOverlays() const { - std::set non_system_overlays; +std::set AssetManager2::GetNonSystemOverlays() const { + std::set non_system_overlays; for (const PackageGroup& package_group : package_groups_) { bool found_system_package = false; for (const ConfiguredPackage& package : package_group.packages_) { @@ -411,7 +429,9 @@ std::set AssetManager2::GetNonSystemOverlays() const { if (!found_system_package) { for (const ConfiguredOverlay& overlay : package_group.overlays_) { - non_system_overlays.insert(apk_assets_[overlay.cookie]); + if (auto asset = apk_assets_[overlay.cookie].promote()) { + non_system_overlays.insert(std::move(asset)); + } } } } @@ -423,21 +443,24 @@ base::expected, IOError> AssetManager2::GetResourceCon bool exclude_system, bool exclude_mipmap) const { ATRACE_NAME("AssetManager::GetResourceConfigurations"); const auto non_system_overlays = - (exclude_system) ? GetNonSystemOverlays() : std::set(); + exclude_system ? GetNonSystemOverlays() : std::set(); std::set configurations; for (const PackageGroup& package_group : package_groups_) { for (size_t i = 0; i < package_group.packages_.size(); i++) { const ConfiguredPackage& package = package_group.packages_[i]; - if (exclude_system && package.loaded_package_->IsSystem()) { - continue; - } - - auto apk_assets = apk_assets_[package_group.cookies_[i]]; - if (exclude_system && apk_assets->IsOverlay() && - non_system_overlays.find(apk_assets) == non_system_overlays.end()) { - // Exclude overlays that target system resources. - continue; + if (exclude_system) { + if (package.loaded_package_->IsSystem()) { + continue; + } + if (!non_system_overlays.empty()) { + // Exclude overlays that target only system resources. + auto apk_assets = apk_assets_[package_group.cookies_[i]].promote(); + if (apk_assets && apk_assets->IsOverlay() && + non_system_overlays.find(apk_assets) == non_system_overlays.end()) { + continue; + } + } } auto result = package.loaded_package_->CollectConfigurations(exclude_mipmap, &configurations); @@ -454,20 +477,23 @@ std::set AssetManager2::GetResourceLocales(bool exclude_system, ATRACE_NAME("AssetManager::GetResourceLocales"); std::set locales; const auto non_system_overlays = - (exclude_system) ? GetNonSystemOverlays() : std::set(); + exclude_system ? GetNonSystemOverlays() : std::set(); for (const PackageGroup& package_group : package_groups_) { for (size_t i = 0; i < package_group.packages_.size(); i++) { const ConfiguredPackage& package = package_group.packages_[i]; - if (exclude_system && package.loaded_package_->IsSystem()) { - continue; - } - - auto apk_assets = apk_assets_[package_group.cookies_[i]]; - if (exclude_system && apk_assets->IsOverlay() && - non_system_overlays.find(apk_assets) == non_system_overlays.end()) { - // Exclude overlays that target system resources. - continue; + if (exclude_system) { + if (package.loaded_package_->IsSystem()) { + continue; + } + if (!non_system_overlays.empty()) { + // Exclude overlays that target only system resources. + auto apk_assets = apk_assets_[package_group.cookies_[i]].promote(); + if (apk_assets && apk_assets->IsOverlay() && + non_system_overlays.find(apk_assets) == non_system_overlays.end()) { + continue; + } + } } package.loaded_package_->CollectLocales(merge_equivalent_languages, &locales); @@ -492,13 +518,12 @@ std::unique_ptr AssetManager2::OpenDir(const std::string& dirname) con ATRACE_NAME("AssetManager::OpenDir"); std::string full_path = "assets/" + dirname; - std::unique_ptr> files = - util::make_unique>(); + auto files = util::make_unique>(); // Start from the back. for (auto iter = apk_assets_.rbegin(); iter != apk_assets_.rend(); ++iter) { - const ApkAssets* apk_assets = *iter; - if (apk_assets->IsOverlay()) { + auto apk_assets = iter->promote(); + if (!apk_assets || apk_assets->IsOverlay()) { continue; } @@ -527,14 +552,15 @@ std::unique_ptr AssetManager2::OpenNonAsset(const std::string& filename, Asset::AccessMode mode, ApkAssetsCookie* out_cookie) const { for (int32_t i = apk_assets_.size() - 1; i >= 0; i--) { + const auto assets = apk_assets_[i].promote(); // Prevent RRO from modifying assets and other entries accessed by file // path. Explicitly asking for a path in a given package (denoted by a // cookie) is still OK. - if (apk_assets_[i]->IsOverlay()) { + if (!assets || assets->IsOverlay()) { continue; } - std::unique_ptr asset = apk_assets_[i]->GetAssetsProvider()->Open(filename, mode); + std::unique_ptr asset = assets->GetAssetsProvider()->Open(filename, mode); if (asset) { if (out_cookie != nullptr) { *out_cookie = i; @@ -555,7 +581,8 @@ std::unique_ptr AssetManager2::OpenNonAsset(const std::string& filename, if (cookie < 0 || static_cast(cookie) >= apk_assets_.size()) { return {}; } - return apk_assets_[cookie]->GetAssetsProvider()->Open(filename, mode); + auto assets = apk_assets_[cookie].promote(); + return assets ? assets->GetAssetsProvider()->Open(filename, mode) : nullptr; } base::expected AssetManager2::FindEntry( @@ -603,90 +630,97 @@ base::expected AssetManager2::FindEntry( } bool overlaid = false; - if (!stop_at_first_match && !ignore_configuration && !apk_assets_[result->cookie]->IsLoader()) { - for (const auto& id_map : package_group.overlays_) { - auto overlay_entry = id_map.overlay_res_maps_.Lookup(resid); - if (!overlay_entry) { - // No id map entry exists for this target resource. - continue; - } - if (overlay_entry.IsInlineValue()) { - // The target resource is overlaid by an inline value not represented by a resource. - ConfigDescription best_frro_config; - Res_value best_frro_value; - bool frro_found = false; - for( const auto& [config, value] : overlay_entry.GetInlineValue()) { - if ((!frro_found || config.isBetterThan(best_frro_config, desired_config)) - && config.match(*desired_config)) { - frro_found = true; - best_frro_config = config; - best_frro_value = value; - } - } - if (!frro_found) { + if (!stop_at_first_match && !ignore_configuration) { + auto assets = apk_assets_[result->cookie].promote(); + if (!assets) { + ALOGE("Found expired ApkAssets #%d for resource ID 0x%08x.", result->cookie, resid); + return base::unexpected(std::nullopt); + } + if (!assets->IsLoader()) { + for (const auto& id_map : package_group.overlays_) { + auto overlay_entry = id_map.overlay_res_maps_.Lookup(resid); + if (!overlay_entry) { + // No id map entry exists for this target resource. continue; } - result->entry = best_frro_value; - result->dynamic_ref_table = id_map.overlay_res_maps_.GetOverlayDynamicRefTable(); - result->cookie = id_map.cookie; - - if (UNLIKELY(logging_enabled)) { - last_resolution_.steps.push_back( - Resolution::Step{Resolution::Step::Type::OVERLAID_INLINE, String8(), result->cookie}); - if (auto path = apk_assets_[result->cookie]->GetPath()) { - const std::string overlay_path = path->data(); - if (IsFabricatedOverlay(overlay_path)) { - // FRRO don't have package name so we use the creating package here. - String8 frro_name = String8("FRRO"); - // Get the first part of it since the expected one should be like - // {overlayPackageName}-{overlayName}-{4 alphanumeric chars}.frro - // under /data/resource-cache/. - const std::string name = overlay_path.substr(overlay_path.rfind('/') + 1); - const size_t end = name.find('-'); - if (frro_name.size() != overlay_path.size() && end != std::string::npos) { - frro_name.append(base::StringPrintf(" created by %s", - name.substr(0 /* pos */, - end).c_str()).c_str()); + if (overlay_entry.IsInlineValue()) { + // The target resource is overlaid by an inline value not represented by a resource. + ConfigDescription best_frro_config; + Res_value best_frro_value; + bool frro_found = false; + for( const auto& [config, value] : overlay_entry.GetInlineValue()) { + if ((!frro_found || config.isBetterThan(best_frro_config, desired_config)) + && config.match(*desired_config)) { + frro_found = true; + best_frro_config = config; + best_frro_value = value; + } + } + if (!frro_found) { + continue; + } + result->entry = best_frro_value; + result->dynamic_ref_table = id_map.overlay_res_maps_.GetOverlayDynamicRefTable(); + result->cookie = id_map.cookie; + + if (UNLIKELY(logging_enabled)) { + last_resolution_.steps.push_back( + Resolution::Step{Resolution::Step::Type::OVERLAID_INLINE, String8(), result->cookie}); + if (auto path = assets->GetPath()) { + const std::string overlay_path = path->data(); + if (IsFabricatedOverlay(overlay_path)) { + // FRRO don't have package name so we use the creating package here. + String8 frro_name = String8("FRRO"); + // Get the first part of it since the expected one should be like + // {overlayPackageName}-{overlayName}-{4 alphanumeric chars}.frro + // under /data/resource-cache/. + const std::string name = overlay_path.substr(overlay_path.rfind('/') + 1); + const size_t end = name.find('-'); + if (frro_name.size() != overlay_path.size() && end != std::string::npos) { + frro_name.append(base::StringPrintf(" created by %s", + name.substr(0 /* pos */, + end).c_str()).c_str()); + } + last_resolution_.best_package_name = frro_name; + } else { + last_resolution_.best_package_name = result->package_name->c_str(); } - last_resolution_.best_package_name = frro_name; - } else { - last_resolution_.best_package_name = result->package_name->c_str(); } + overlaid = true; } - overlaid = true; + continue; } - continue; - } - auto overlay_result = FindEntry(overlay_entry.GetResourceId(), density_override, - false /* stop_at_first_match */, - false /* ignore_configuration */); - if (UNLIKELY(IsIOError(overlay_result))) { - return base::unexpected(overlay_result.error()); - } - if (!overlay_result.has_value()) { - continue; - } + auto overlay_result = FindEntry(overlay_entry.GetResourceId(), density_override, + false /* stop_at_first_match */, + false /* ignore_configuration */); + if (UNLIKELY(IsIOError(overlay_result))) { + return base::unexpected(overlay_result.error()); + } + if (!overlay_result.has_value()) { + continue; + } - if (!overlay_result->config.isBetterThan(result->config, desired_config) - && overlay_result->config.compare(result->config) != 0) { - // The configuration of the entry for the overlay must be equal to or better than the target - // configuration to be chosen as the better value. - continue; - } + if (!overlay_result->config.isBetterThan(result->config, desired_config) + && overlay_result->config.compare(result->config) != 0) { + // The configuration of the entry for the overlay must be equal to or better than the target + // configuration to be chosen as the better value. + continue; + } - result->cookie = overlay_result->cookie; - result->entry = overlay_result->entry; - result->config = overlay_result->config; - result->dynamic_ref_table = id_map.overlay_res_maps_.GetOverlayDynamicRefTable(); + result->cookie = overlay_result->cookie; + result->entry = overlay_result->entry; + result->config = overlay_result->config; + result->dynamic_ref_table = id_map.overlay_res_maps_.GetOverlayDynamicRefTable(); - if (UNLIKELY(logging_enabled)) { - last_resolution_.steps.push_back( - Resolution::Step{Resolution::Step::Type::OVERLAID, overlay_result->config.toString(), - overlay_result->cookie}); - last_resolution_.best_package_name = - overlay_result->package_name->c_str(); - overlaid = true; + if (UNLIKELY(logging_enabled)) { + last_resolution_.steps.push_back( + Resolution::Step{Resolution::Step::Type::OVERLAID, overlay_result->config.toString(), + overlay_result->cookie}); + last_resolution_.best_package_name = + overlay_result->package_name->c_str(); + overlaid = true; + } } } } @@ -868,7 +902,9 @@ std::string AssetManager2::GetLastResourceResolution() const { } const uint32_t resid = last_resolution_.resid; - const auto package = apk_assets_[cookie]->GetLoadedArsc()->GetPackageById(get_package_id(resid)); + auto assets = apk_assets_[cookie].promote(); + const auto package = + assets ? assets->GetLoadedArsc()->GetPackageById(get_package_id(resid)) : nullptr; std::string resource_name_string; if (package != nullptr) { @@ -898,8 +934,9 @@ std::string AssetManager2::GetLastResourceResolution() const { if (prefix == kStepStrings.end()) { continue; } - - log_stream << "\n\t" << prefix->second << ": " << apk_assets_[step.cookie]->GetDebugName(); + auto assets = apk_assets_[step.cookie].promote(); + log_stream << "\n\t" << prefix->second << ": " + << (assets ? assets->GetDebugName() : "") << " #" << step.cookie; if (!step.config_name.isEmpty()) { log_stream << " - " << step.config_name; } @@ -1563,12 +1600,20 @@ base::expected Theme::SetTo(const Theme& source) { // Determine which ApkAssets are loaded in both theme AssetManagers. const auto& src_assets = source.asset_manager_->GetApkAssets(); - for (size_t i = 0; i < src_assets.size(); i++) { - const ApkAssets* src_asset = src_assets[i]; + const auto& dest_assets = asset_manager_->GetApkAssets(); + std::vector promoted_src_assets; + promoted_src_assets.reserve(src_assets.size()); + for (const auto& src_asset : src_assets) { + promoted_src_assets.emplace_back(src_asset.promote()); + } - const auto& dest_assets = asset_manager_->GetApkAssets(); - for (size_t j = 0; j < dest_assets.size(); j++) { - const ApkAssets* dest_asset = dest_assets[j]; + for (size_t j = 0; j < dest_assets.size(); j++) { + auto dest_asset = dest_assets[j].promote(); + if (!dest_asset) { + continue; + } + for (size_t i = 0; i < promoted_src_assets.size(); i++) { + const auto& src_asset = promoted_src_assets[i]; if (src_asset != dest_asset) { // ResourcesManager caches and reuses ApkAssets when the same apk must be present in // multiple AssetManagers. Two ApkAssets point to the same version of the same resources -- cgit v1.2.3-59-g8ed1b