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
diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp
index 28fcc1a..725a1b8 100644
--- a/tools/aapt2/cmd/Link_test.cpp
+++ b/tools/aapt2/cmd/Link_test.cpp
@@ -575,7 +575,7 @@
android::AssetManager2 am;
auto android_asset = android::ApkAssets::Load(android_apk);
ASSERT_THAT(android_asset, NotNull());
- ASSERT_TRUE(am.SetApkAssets({android_asset.get()}));
+ ASSERT_TRUE(am.SetApkAssets({android_asset}));
auto result = am.GetResourceId("android:attr/finalized_res");
ASSERT_TRUE(result.has_value());
@@ -631,7 +631,7 @@
auto app_against_non_final = android::ApkAssets::Load(app_apk);
ASSERT_THAT(android_asset, NotNull());
ASSERT_THAT(app_against_non_final, NotNull());
- ASSERT_TRUE(am.SetApkAssets({android_asset.get(), app_against_non_final.get()}));
+ ASSERT_TRUE(am.SetApkAssets({android_asset, app_against_non_final}));
auto result = am.GetResourceId("android:attr/finalized_res");
ASSERT_TRUE(result.has_value());
@@ -667,7 +667,7 @@
auto app_against_final = android::ApkAssets::Load(app_apk_respin);
ASSERT_THAT(app_against_final, NotNull());
- ASSERT_TRUE(am.SetApkAssets({android_asset.get(), app_against_final.get()}));
+ ASSERT_TRUE(am.SetApkAssets({android_asset, app_against_final}));
{
auto style = am.GetBag(0x7f020000);
diff --git a/tools/aapt2/process/SymbolTable.cpp b/tools/aapt2/process/SymbolTable.cpp
index bca62da..b75458a 100644
--- a/tools/aapt2/process/SymbolTable.cpp
+++ b/tools/aapt2/process/SymbolTable.cpp
@@ -220,15 +220,9 @@
bool AssetManagerSymbolSource::AddAssetPath(StringPiece path) {
TRACE_CALL();
- if (std::unique_ptr<const ApkAssets> apk = ApkAssets::Load(path.data())) {
+ if (auto apk = ApkAssets::Load(path.data())) {
apk_assets_.push_back(std::move(apk));
-
- std::vector<const ApkAssets*> apk_assets;
- for (const std::unique_ptr<const ApkAssets>& apk_asset : apk_assets_) {
- apk_assets.push_back(apk_asset.get());
- }
-
- asset_manager_.SetApkAssets(apk_assets);
+ asset_manager_.SetApkAssets(apk_assets_);
return true;
}
return false;
@@ -251,7 +245,7 @@
return true;
}
- for (const std::unique_ptr<const ApkAssets>& assets : apk_assets_) {
+ for (auto&& assets : apk_assets_) {
for (const std::unique_ptr<const android::LoadedPackage>& loaded_package
: assets->GetLoadedArsc()->GetPackages()) {
if (package_name == loaded_package->GetPackageName() && loaded_package->IsDynamic()) {
diff --git a/tools/aapt2/process/SymbolTable.h b/tools/aapt2/process/SymbolTable.h
index b09ff70..36eb0ba 100644
--- a/tools/aapt2/process/SymbolTable.h
+++ b/tools/aapt2/process/SymbolTable.h
@@ -207,8 +207,8 @@
}
private:
+ std::vector<android::AssetManager2::ApkAssetsPtr> apk_assets_;
android::AssetManager2 asset_manager_;
- std::vector<std::unique_ptr<const android::ApkAssets>> apk_assets_;
DISALLOW_COPY_AND_ASSIGN(AssetManagerSymbolSource);
};
