Don't use ApkAssets::GetPath for equality checks
With the introduction of ResourcesProviders, not all ApkAssets have
paths on disk. Theme::SetTo and various AssetManager methods use the
path to perform equality checking on ApkAssets. This equality check
will be performed on the debug string of an ApkAssets if it does not
have a path on disk. This causes ApkAssets with the same debug name
(like "<empty>") to be seen as the same ApkAssets. Rather than using
path, the pointer to the ApkAssets should be used for equality checking
since ResourcesManager caches and reuses ApkAssets when multiple
AssetManagers request the same assets.
Bug: 177101983
Test: atest CtsResourcesLoaderTests
Change-Id: I11f6a2a3a7cc8febe3f976236792f78e41cf07e6
diff --git a/core/java/android/content/res/ApkAssets.java b/core/java/android/content/res/ApkAssets.java
index 2d381eb..de48ed7 100644
--- a/core/java/android/content/res/ApkAssets.java
+++ b/core/java/android/content/res/ApkAssets.java
@@ -22,6 +22,7 @@
import android.content.om.OverlayableInfo;
import android.content.res.loader.AssetsProvider;
import android.content.res.loader.ResourcesProvider;
+import android.text.TextUtils;
import com.android.internal.annotations.GuardedBy;
@@ -344,7 +345,14 @@
@UnsupportedAppUsage
public @NonNull String getAssetPath() {
synchronized (this) {
- return nativeGetAssetPath(mNativePtr);
+ return TextUtils.emptyIfNull(nativeGetAssetPath(mNativePtr));
+ }
+ }
+
+ /** @hide */
+ public @NonNull String getDebugName() {
+ synchronized (this) {
+ return nativeGetDebugName(mNativePtr);
}
}
@@ -422,7 +430,7 @@
@Override
public String toString() {
- return "ApkAssets{path=" + getAssetPath() + "}";
+ return "ApkAssets{path=" + getDebugName() + "}";
}
/**
@@ -450,6 +458,7 @@
@NonNull FileDescriptor fd, @NonNull String friendlyName, long offset, long length,
@PropertyFlags int flags, @Nullable AssetsProvider asset) throws IOException;
private static native @NonNull String nativeGetAssetPath(long ptr);
+ private static native @NonNull String nativeGetDebugName(long ptr);
private static native long nativeGetStringBlock(long ptr);
private static native boolean nativeIsUpToDate(long ptr);
private static native long nativeOpenXml(long ptr, @NonNull String fileName) throws IOException;
diff --git a/core/jni/android_content_res_ApkAssets.cpp b/core/jni/android_content_res_ApkAssets.cpp
index c7439f1..b0c5751 100644
--- a/core/jni/android_content_res_ApkAssets.cpp
+++ b/core/jni/android_content_res_ApkAssets.cpp
@@ -83,6 +83,10 @@
return true;
}
+ std::optional<std::string_view> GetPath() const override {
+ return {};
+ }
+
const std::string& GetDebugName() const override {
return debug_name_;
}
@@ -358,8 +362,16 @@
}
static jstring NativeGetAssetPath(JNIEnv* env, jclass /*clazz*/, jlong ptr) {
- const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
- return env->NewStringUTF(apk_assets->GetPath().c_str());
+ auto apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+ if (auto path = apk_assets->GetPath()) {
+ return env->NewStringUTF(path->data());
+ }
+ return nullptr;
+}
+
+static jstring NativeGetDebugName(JNIEnv* env, jclass /*clazz*/, jlong ptr) {
+ auto apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+ return env->NewStringUTF(apk_assets->GetDebugName().c_str());
}
static jlong NativeGetStringBlock(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) {
@@ -467,6 +479,7 @@
(void*)NativeLoadFromFdOffset},
{"nativeGetFinalizer", "()J", (void*)NativeGetFinalizer},
{"nativeGetAssetPath", "(J)Ljava/lang/String;", (void*)NativeGetAssetPath},
+ {"nativeGetDebugName", "(J)Ljava/lang/String;", (void*)NativeGetDebugName},
{"nativeGetStringBlock", "(J)J", (void*)NativeGetStringBlock},
{"nativeIsUpToDate", "(J)Z", (void*)NativeIsUpToDate},
{"nativeOpenXml", "(JLjava/lang/String;)J", (void*)NativeOpenXml},
diff --git a/libs/androidfw/ApkAssets.cpp b/libs/androidfw/ApkAssets.cpp
index 9c743ce..76366fc 100755
--- a/libs/androidfw/ApkAssets.cpp
+++ b/libs/androidfw/ApkAssets.cpp
@@ -156,7 +156,11 @@
std::move(loaded_idmap)));
}
-const std::string& ApkAssets::GetPath() const {
+std::optional<std::string_view> ApkAssets::GetPath() const {
+ return assets_provider_->GetPath();
+}
+
+const std::string& ApkAssets::GetDebugName() const {
return assets_provider_->GetDebugName();
}
diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp
index 36bde5c..c0ef7be 100644
--- a/libs/androidfw/AssetManager2.cpp
+++ b/libs/androidfw/AssetManager2.cpp
@@ -116,8 +116,10 @@
package_groups_.clear();
package_ids_.fill(0xff);
- // A mapping from apk assets path to the runtime package id of its first loaded package.
- std::unordered_map<std::string, uint8_t> apk_assets_package_ids;
+ // A mapping from path of apk assets that could be target packages of overlays to the runtime
+ // package id of its first loaded package. Overlays currently can only override resources in the
+ // first package in the target resource table.
+ std::unordered_map<std::string, uint8_t> target_assets_package_ids;
// 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.
@@ -140,8 +142,8 @@
if (auto loaded_idmap = apk_assets->GetLoadedIdmap(); loaded_idmap != nullptr) {
// The target package must precede the overlay package in the apk assets paths in order
// to take effect.
- auto iter = apk_assets_package_ids.find(std::string(loaded_idmap->TargetApkPath()));
- if (iter == apk_assets_package_ids.end()) {
+ auto iter = target_assets_package_ids.find(std::string(loaded_idmap->TargetApkPath()));
+ if (iter == target_assets_package_ids.end()) {
LOG(INFO) << "failed to find target package for overlay "
<< loaded_idmap->OverlayApkPath();
} else {
@@ -205,7 +207,10 @@
package_name, static_cast<uint8_t>(entry.package_id));
}
- apk_assets_package_ids.insert(std::make_pair(apk_assets->GetPath(), package_id));
+ if (auto apk_assets_path = apk_assets->GetPath()) {
+ // Overlay target ApkAssets must have been created using path based load apis.
+ target_assets_package_ids.insert(std::make_pair(std::string(*apk_assets_path), package_id));
+ }
}
}
@@ -227,7 +232,7 @@
std::string list;
for (const auto& apk_assets : apk_assets_) {
- base::StringAppendF(&list, "%s,", apk_assets->GetPath().c_str());
+ base::StringAppendF(&list, "%s,", apk_assets->GetDebugName().c_str());
}
LOG(INFO) << "ApkAssets: " << list;
@@ -383,8 +388,8 @@
}
}
-std::set<std::string> AssetManager2::GetNonSystemOverlayPaths() const {
- std::set<std::string> non_system_overlays;
+std::set<const ApkAssets*> AssetManager2::GetNonSystemOverlays() const {
+ std::set<const ApkAssets*> non_system_overlays;
for (const PackageGroup& package_group : package_groups_) {
bool found_system_package = false;
for (const ConfiguredPackage& package : package_group.packages_) {
@@ -396,7 +401,7 @@
if (!found_system_package) {
for (const ConfiguredOverlay& overlay : package_group.overlays_) {
- non_system_overlays.insert(apk_assets_[overlay.cookie]->GetPath());
+ non_system_overlays.insert(apk_assets_[overlay.cookie]);
}
}
}
@@ -408,7 +413,7 @@
bool exclude_system, bool exclude_mipmap) const {
ATRACE_NAME("AssetManager::GetResourceConfigurations");
const auto non_system_overlays =
- (exclude_system) ? GetNonSystemOverlayPaths() : std::set<std::string>();
+ (exclude_system) ? GetNonSystemOverlays() : std::set<const ApkAssets*>();
std::set<ResTable_config> configurations;
for (const PackageGroup& package_group : package_groups_) {
@@ -419,8 +424,8 @@
}
auto apk_assets = apk_assets_[package_group.cookies_[i]];
- if (exclude_system && apk_assets->IsOverlay()
- && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) {
+ if (exclude_system && apk_assets->IsOverlay() &&
+ non_system_overlays.find(apk_assets) == non_system_overlays.end()) {
// Exclude overlays that target system resources.
continue;
}
@@ -439,7 +444,7 @@
ATRACE_NAME("AssetManager::GetResourceLocales");
std::set<std::string> locales;
const auto non_system_overlays =
- (exclude_system) ? GetNonSystemOverlayPaths() : std::set<std::string>();
+ (exclude_system) ? GetNonSystemOverlays() : std::set<const ApkAssets*>();
for (const PackageGroup& package_group : package_groups_) {
for (size_t i = 0; i < package_group.packages_.size(); i++) {
@@ -449,8 +454,8 @@
}
auto apk_assets = apk_assets_[package_group.cookies_[i]];
- if (exclude_system && apk_assets->IsOverlay()
- && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) {
+ if (exclude_system && apk_assets->IsOverlay() &&
+ non_system_overlays.find(apk_assets) == non_system_overlays.end()) {
// Exclude overlays that target system resources.
continue;
}
@@ -491,7 +496,7 @@
AssetDir::FileInfo info;
info.setFileName(String8(name.data(), name.size()));
info.setFileType(type);
- info.setSourceName(String8(apk_assets->GetPath().c_str()));
+ info.setSourceName(String8(apk_assets->GetDebugName().c_str()));
files->add(info);
};
@@ -846,7 +851,7 @@
}
log_stream << "\n\t" << prefix->second << ": " << *step.package_name << " ("
- << apk_assets_[step.cookie]->GetPath() << ")";
+ << apk_assets_[step.cookie]->GetDebugName() << ")";
if (!step.config_name.isEmpty()) {
log_stream << " -" << step.config_name;
}
@@ -1556,41 +1561,32 @@
std::map<ApkAssetsCookie, SourceToDestinationRuntimePackageMap> src_asset_cookie_id_map;
// Determine which ApkAssets are loaded in both theme AssetManagers.
- std::vector<const ApkAssets*> src_assets = o.asset_manager_->GetApkAssets();
+ const auto src_assets = o.asset_manager_->GetApkAssets();
for (size_t i = 0; i < src_assets.size(); i++) {
const ApkAssets* src_asset = src_assets[i];
- std::vector<const ApkAssets*> dest_assets = asset_manager_->GetApkAssets();
+ const auto dest_assets = asset_manager_->GetApkAssets();
for (size_t j = 0; j < dest_assets.size(); j++) {
const ApkAssets* dest_asset = dest_assets[j];
-
- // Map the runtime package of the source apk asset to the destination apk asset.
- if (src_asset->GetPath() == dest_asset->GetPath()) {
- const auto& src_packages = src_asset->GetLoadedArsc()->GetPackages();
- const auto& dest_packages = dest_asset->GetLoadedArsc()->GetPackages();
-
- SourceToDestinationRuntimePackageMap package_map;
-
- // The source and destination package should have the same number of packages loaded in
- // the same order.
- const size_t N = src_packages.size();
- CHECK(N == dest_packages.size())
- << " LoadedArsc " << src_asset->GetPath() << " differs number of packages.";
- for (size_t p = 0; p < N; p++) {
- auto& src_package = src_packages[p];
- auto& dest_package = dest_packages[p];
- CHECK(src_package->GetPackageName() == dest_package->GetPackageName())
- << " Package " << src_package->GetPackageName() << " differs in load order.";
-
- int src_package_id = o.asset_manager_->GetAssignedPackageId(src_package.get());
- int dest_package_id = asset_manager_->GetAssignedPackageId(dest_package.get());
- package_map[src_package_id] = dest_package_id;
- }
-
- src_to_dest_asset_cookies.insert(std::make_pair(i, j));
- src_asset_cookie_id_map.insert(std::make_pair(i, package_map));
- break;
+ 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
+ // if they are the same instance.
+ continue;
}
+
+ // Map the package ids of the asset in the source AssetManager to the package ids of the
+ // asset in th destination AssetManager.
+ SourceToDestinationRuntimePackageMap package_map;
+ for (const auto& loaded_package : src_asset->GetLoadedArsc()->GetPackages()) {
+ const int src_package_id = o.asset_manager_->GetAssignedPackageId(loaded_package.get());
+ const int dest_package_id = asset_manager_->GetAssignedPackageId(loaded_package.get());
+ package_map[src_package_id] = dest_package_id;
+ }
+
+ src_to_dest_asset_cookies.insert(std::make_pair(i, j));
+ src_asset_cookie_id_map.insert(std::make_pair(i, package_map));
+ break;
}
}
diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp
index f3c48f7..0aaf0b3 100644
--- a/libs/androidfw/AssetsProvider.cpp
+++ b/libs/androidfw/AssetsProvider.cpp
@@ -261,6 +261,13 @@
return entry.crc32;
}
+std::optional<std::string_view> ZipAssetsProvider::GetPath() const {
+ if (name_.GetPath() != nullptr) {
+ return *name_.GetPath();
+ }
+ return {};
+}
+
const std::string& ZipAssetsProvider::GetDebugName() const {
return name_.GetDebugName();
}
@@ -318,6 +325,10 @@
return true;
}
+std::optional<std::string_view> DirectoryAssetsProvider::GetPath() const {
+ return dir_;
+}
+
const std::string& DirectoryAssetsProvider::GetDebugName() const {
return dir_;
}
@@ -336,13 +347,9 @@
std::unique_ptr<AssetsProvider>&& secondary)
: primary_(std::forward<std::unique_ptr<AssetsProvider>>(primary)),
secondary_(std::forward<std::unique_ptr<AssetsProvider>>(secondary)) {
- if (primary_->GetDebugName() == kEmptyDebugString) {
- debug_name_ = secondary_->GetDebugName();
- } else if (secondary_->GetDebugName() == kEmptyDebugString) {
- debug_name_ = primary_->GetDebugName();
- } else {
- debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName();
- }
+ debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName();
+ path_ = (primary_->GetDebugName() != kEmptyDebugString) ? primary_->GetPath()
+ : secondary_->GetPath();
}
std::unique_ptr<AssetsProvider> MultiAssetsProvider::Create(
@@ -367,6 +374,10 @@
return primary_->ForEachFile(root_path, f) && secondary_->ForEachFile(root_path, f);
}
+std::optional<std::string_view> MultiAssetsProvider::GetPath() const {
+ return path_;
+}
+
const std::string& MultiAssetsProvider::GetDebugName() const {
return debug_name_;
}
@@ -394,6 +405,10 @@
return true;
}
+std::optional<std::string_view> EmptyAssetsProvider::GetPath() const {
+ return {};
+}
+
const std::string& EmptyAssetsProvider::GetDebugName() const {
const static std::string kEmpty = kEmptyDebugString;
return kEmpty;
diff --git a/libs/androidfw/include/androidfw/ApkAssets.h b/libs/androidfw/include/androidfw/ApkAssets.h
index d0019ed..6f88f41 100644
--- a/libs/androidfw/include/androidfw/ApkAssets.h
+++ b/libs/androidfw/include/androidfw/ApkAssets.h
@@ -34,7 +34,6 @@
// Holds an APK.
class ApkAssets {
public:
-
// Creates an ApkAssets from a path on device.
static std::unique_ptr<ApkAssets> Load(const std::string& path,
package_property_t flags = 0U);
@@ -61,12 +60,11 @@
static std::unique_ptr<ApkAssets> LoadOverlay(const std::string& idmap_path,
package_property_t flags = 0U);
- // TODO(177101983): Remove all uses of GetPath for checking whether two ApkAssets are the same.
- // With the introduction of ResourcesProviders, not all ApkAssets have paths. This could cause
- // bugs when path is used for comparison because multiple ApkAssets could have the same "firendly
- // name". Use pointer equality instead. ResourceManager caches and reuses ApkAssets so the
- // same asset should have the same pointer.
- const std::string& GetPath() const;
+ // Path to the contents of the ApkAssets on disk. The path could represent an APk, a directory,
+ // or some other file type.
+ std::optional<std::string_view> GetPath() const;
+
+ const std::string& GetDebugName() const;
const AssetsProvider* GetAssetsProvider() const {
return assets_provider_.get();
diff --git a/libs/androidfw/include/androidfw/AssetManager2.h b/libs/androidfw/include/androidfw/AssetManager2.h
index 2255973f..119f531 100644
--- a/libs/androidfw/include/androidfw/AssetManager2.h
+++ b/libs/androidfw/include/androidfw/AssetManager2.h
@@ -412,7 +412,7 @@
void RebuildFilterList();
// Retrieves the APK paths of overlays that overlay non-system packages.
- std::set<std::string> GetNonSystemOverlayPaths() const;
+ std::set<const ApkAssets*> GetNonSystemOverlays() const;
// AssetManager2::GetBag(resid) wraps this function to track which resource ids have already
// been seen while traversing bag parents.
diff --git a/libs/androidfw/include/androidfw/AssetsProvider.h b/libs/androidfw/include/androidfw/AssetsProvider.h
index 6f16ff4..63bbdcc 100644
--- a/libs/androidfw/include/androidfw/AssetsProvider.h
+++ b/libs/androidfw/include/androidfw/AssetsProvider.h
@@ -48,6 +48,10 @@
virtual bool ForEachFile(const std::string& path,
const std::function<void(const StringPiece&, FileType)>& f) const = 0;
+ // Retrieves the path to the contents of the AssetsProvider on disk. The path could represent an
+ // APk, a directory, or some other file type.
+ WARN_UNUSED virtual std::optional<std::string_view> GetPath() const = 0;
+
// Retrieves a name that represents the interface. This may or may not be the path of the
// interface source.
WARN_UNUSED virtual const std::string& GetDebugName() const = 0;
@@ -85,9 +89,9 @@
bool ForEachFile(const std::string& root_path,
const std::function<void(const StringPiece&, FileType)>& f) const override;
+ WARN_UNUSED std::optional<std::string_view> GetPath() const override;
WARN_UNUSED const std::string& GetDebugName() const override;
WARN_UNUSED bool IsUpToDate() const override;
-
WARN_UNUSED std::optional<uint32_t> GetCrc(std::string_view path) const;
~ZipAssetsProvider() override = default;
@@ -125,6 +129,7 @@
bool ForEachFile(const std::string& path,
const std::function<void(const StringPiece&, FileType)>& f) const override;
+ WARN_UNUSED std::optional<std::string_view> GetPath() const override;
WARN_UNUSED const std::string& GetDebugName() const override;
WARN_UNUSED bool IsUpToDate() const override;
@@ -149,6 +154,7 @@
bool ForEachFile(const std::string& root_path,
const std::function<void(const StringPiece&, FileType)>& f) const override;
+ WARN_UNUSED std::optional<std::string_view> GetPath() const override;
WARN_UNUSED const std::string& GetDebugName() const override;
WARN_UNUSED bool IsUpToDate() const override;
@@ -163,6 +169,7 @@
std::unique_ptr<AssetsProvider> primary_;
std::unique_ptr<AssetsProvider> secondary_;
+ std::optional<std::string_view> path_;
std::string debug_name_;
};
@@ -173,6 +180,7 @@
bool ForEachFile(const std::string& path,
const std::function<void(const StringPiece&, FileType)>& f) const override;
+ WARN_UNUSED std::optional<std::string_view> GetPath() const override;
WARN_UNUSED const std::string& GetDebugName() const override;
WARN_UNUSED bool IsUpToDate() const override;
diff --git a/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java b/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java
index 65d4e23..4b9a431 100644
--- a/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java
+++ b/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java
@@ -200,7 +200,7 @@
Log.d(TAG, " assetPaths=");
ApkAssets[] assets = context.getResources().getAssets().getApkAssets();
for (ApkAssets a : assets) {
- Log.d(TAG, " " + a.getAssetPath());
+ Log.d(TAG, " " + a.getDebugName());
}
}
}