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/ApkAssets.cpp | 58 ++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 32 deletions(-) (limited to 'libs/androidfw/ApkAssets.cpp') diff --git a/libs/androidfw/ApkAssets.cpp b/libs/androidfw/ApkAssets.cpp index 15aaae25f754..f0c639574a9f 100644 --- a/libs/androidfw/ApkAssets.cpp +++ b/libs/androidfw/ApkAssets.cpp @@ -27,39 +27,34 @@ using base::unique_fd; constexpr const char* kResourcesArsc = "resources.arsc"; -ApkAssets::ApkAssets(std::unique_ptr resources_asset, +ApkAssets::ApkAssets(PrivateConstructorUtil, std::unique_ptr resources_asset, std::unique_ptr loaded_arsc, - std::unique_ptr assets, - package_property_t property_flags, - std::unique_ptr idmap_asset, - std::unique_ptr loaded_idmap) + std::unique_ptr assets, package_property_t property_flags, + std::unique_ptr idmap_asset, std::unique_ptr loaded_idmap) : resources_asset_(std::move(resources_asset)), loaded_arsc_(std::move(loaded_arsc)), assets_provider_(std::move(assets)), property_flags_(property_flags), idmap_asset_(std::move(idmap_asset)), - loaded_idmap_(std::move(loaded_idmap)) {} + loaded_idmap_(std::move(loaded_idmap)) { +} -std::unique_ptr ApkAssets::Load(const std::string& path, package_property_t flags) { +ApkAssetsPtr ApkAssets::Load(const std::string& path, package_property_t flags) { return Load(ZipAssetsProvider::Create(path, flags), flags); } -std::unique_ptr ApkAssets::LoadFromFd(base::unique_fd fd, - const std::string& debug_name, - package_property_t flags, - off64_t offset, - off64_t len) { +ApkAssetsPtr ApkAssets::LoadFromFd(base::unique_fd fd, const std::string& debug_name, + package_property_t flags, off64_t offset, off64_t len) { return Load(ZipAssetsProvider::Create(std::move(fd), debug_name, offset, len), flags); } -std::unique_ptr ApkAssets::Load(std::unique_ptr assets, - package_property_t flags) { +ApkAssetsPtr ApkAssets::Load(std::unique_ptr assets, package_property_t flags) { return LoadImpl(std::move(assets), flags, nullptr /* idmap_asset */, nullptr /* loaded_idmap */); } -std::unique_ptr ApkAssets::LoadTable(std::unique_ptr resources_asset, - std::unique_ptr assets, - package_property_t flags) { +ApkAssetsPtr ApkAssets::LoadTable(std::unique_ptr resources_asset, + std::unique_ptr assets, + package_property_t flags) { if (resources_asset == nullptr) { return {}; } @@ -67,8 +62,7 @@ std::unique_ptr ApkAssets::LoadTable(std::unique_ptr resources nullptr /* loaded_idmap */); } -std::unique_ptr ApkAssets::LoadOverlay(const std::string& idmap_path, - package_property_t flags) { +ApkAssetsPtr ApkAssets::LoadOverlay(const std::string& idmap_path, package_property_t flags) { CHECK((flags & PROPERTY_LOADER) == 0U) << "Cannot load RROs through loaders"; auto idmap_asset = AssetsProvider::CreateAssetFromFile(idmap_path); if (idmap_asset == nullptr) { @@ -103,10 +97,10 @@ std::unique_ptr ApkAssets::LoadOverlay(const std::string& idmap_path, std::move(loaded_idmap)); } -std::unique_ptr ApkAssets::LoadImpl(std::unique_ptr assets, - package_property_t property_flags, - std::unique_ptr idmap_asset, - std::unique_ptr loaded_idmap) { +ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr assets, + package_property_t property_flags, + std::unique_ptr idmap_asset, + std::unique_ptr loaded_idmap) { if (assets == nullptr) { return {}; } @@ -125,11 +119,11 @@ std::unique_ptr ApkAssets::LoadImpl(std::unique_ptr a std::move(idmap_asset), std::move(loaded_idmap)); } -std::unique_ptr ApkAssets::LoadImpl(std::unique_ptr resources_asset, - std::unique_ptr assets, - package_property_t property_flags, - std::unique_ptr idmap_asset, - std::unique_ptr loaded_idmap) { +ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr resources_asset, + std::unique_ptr assets, + package_property_t property_flags, + std::unique_ptr idmap_asset, + std::unique_ptr loaded_idmap) { if (assets == nullptr ) { return {}; } @@ -155,10 +149,9 @@ std::unique_ptr ApkAssets::LoadImpl(std::unique_ptr resources_ return {}; } - return std::unique_ptr(new ApkAssets(std::move(resources_asset), - std::move(loaded_arsc), std::move(assets), - property_flags, std::move(idmap_asset), - std::move(loaded_idmap))); + return ApkAssetsPtr::make(PrivateConstructorUtil{}, std::move(resources_asset), + std::move(loaded_arsc), std::move(assets), property_flags, + std::move(idmap_asset), std::move(loaded_idmap)); } std::optional ApkAssets::GetPath() const { @@ -174,4 +167,5 @@ bool ApkAssets::IsUpToDate() const { return IsLoader() || ((!loaded_idmap_ || loaded_idmap_->IsUpToDate()) && assets_provider_->IsUpToDate()); } + } // namespace android -- cgit v1.2.3-59-g8ed1b