Merge "Don't use ApkAssets::GetPath for equality checks" into sc-dev

core/java/android/content/res/ApkAssets.java

@@ -22,6 +22,7 @@ import android.compat.annotation.UnsupportedAppUsage;
 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 @@ public final class ApkAssets {
     @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 @@ public final class ApkAssets {
 
     @Override
     public String toString() {
-        return "ApkAssets{path=" + getAssetPath() + "}";
+        return "ApkAssets{path=" + getDebugName() + "}";
     }
 
     /**
@@ -450,6 +458,7 @@ public final class ApkAssets {
             @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;

core/jni/android_content_res_ApkAssets.cpp

@@ -83,6 +83,10 @@ class LoaderAssetsProvider : public AssetsProvider {
     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 jlong NativeGetFinalizer(JNIEnv* /*env*/, jclass /*clazz*/) {
 }
 
 static jstring NativeGetAssetPath(JNIEnv* env, jclass /*clazz*/, jlong ptr) {
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+  auto apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
-  return env->NewStringUTF(apk_assets->GetPath().c_str());
+  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 @@ static const JNINativeMethod gApkAssetsMethods[] = {
      (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},

libs/androidfw/ApkAssets.cpp

@@ -156,7 +156,11 @@ std::unique_ptr<ApkAssets> ApkAssets::LoadImpl(std::unique_ptr<Asset> resources_
                                                   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();
 }
 

libs/androidfw/AssetManager2.cpp

@@ -116,8 +116,10 @@ void AssetManager2::BuildDynamicRefTable() {
   package_groups_.clear();
   package_ids_.fill(0xff);
 
-  // A mapping from apk assets path to the runtime package id of its first loaded package.
+  // A mapping from path of apk assets that could be target packages of overlays to the runtime
-  std::unordered_map<std::string, uint8_t> apk_assets_package_ids;
+  // 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 @@ void AssetManager2::BuildDynamicRefTable() {
     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()));
+      auto iter = target_assets_package_ids.find(std::string(loaded_idmap->TargetApkPath()));
-      if (iter == apk_assets_package_ids.end()) {
+      if (iter == target_assets_package_ids.end()) {
          LOG(INFO) << "failed to find target package for overlay "
                    << loaded_idmap->OverlayApkPath();
       } else {
@@ -205,7 +207,10 @@ void AssetManager2::BuildDynamicRefTable() {
             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 @@ void AssetManager2::DumpToLog() const {
 
   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 @@ void AssetManager2::SetConfiguration(const ResTable_config& configuration) {
   }
 }
 
-std::set<std::string> AssetManager2::GetNonSystemOverlayPaths() const {
+std::set<const ApkAssets*> AssetManager2::GetNonSystemOverlays() const {
-  std::set<std::string> non_system_overlays;
+  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 @@ std::set<std::string> AssetManager2::GetNonSystemOverlayPaths() const {
 
     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 @@ base::expected<std::set<ResTable_config>, IOError> AssetManager2::GetResourceCon
     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 @@ base::expected<std::set<ResTable_config>, IOError> AssetManager2::GetResourceCon
       }
 
       auto apk_assets = apk_assets_[package_group.cookies_[i]];
-      if (exclude_system && apk_assets->IsOverlay()
+      if (exclude_system && apk_assets->IsOverlay() &&
-          && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) {
+          non_system_overlays.find(apk_assets) == non_system_overlays.end()) {
         // Exclude overlays that target system resources.
         continue;
       }
@@ -439,7 +444,7 @@ std::set<std::string> AssetManager2::GetResourceLocales(bool exclude_system,
   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 @@ std::set<std::string> AssetManager2::GetResourceLocales(bool exclude_system,
       }
 
       auto apk_assets = apk_assets_[package_group.cookies_[i]];
-      if (exclude_system && apk_assets->IsOverlay()
+      if (exclude_system && apk_assets->IsOverlay() &&
-          && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) {
+          non_system_overlays.find(apk_assets) == non_system_overlays.end()) {
         // Exclude overlays that target system resources.
         continue;
       }
@@ -491,7 +496,7 @@ std::unique_ptr<AssetDir> AssetManager2::OpenDir(const std::string& dirname) con
       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 @@ std::string AssetManager2::GetLastResourceResolution() const {
     }
 
     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 @@ base::expected<std::monostate, IOError> Theme::SetTo(const Theme& o) {
     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];
-
+        if (src_asset != dest_asset) {
-        // Map the runtime package of the source apk asset to the destination apk asset.
+          // ResourcesManager caches and reuses ApkAssets when the same apk must be present in
-        if (src_asset->GetPath() == dest_asset->GetPath()) {
+          // multiple AssetManagers. Two ApkAssets point to the same version of the same resources
-          const auto& src_packages = src_asset->GetLoadedArsc()->GetPackages();
+          // if they are the same instance.
-          const auto& dest_packages = dest_asset->GetLoadedArsc()->GetPackages();
+          continue;
-
-          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;
         }
+
+        // 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;
       }
     }
 

libs/androidfw/AssetsProvider.cpp

@@ -261,6 +261,13 @@ std::optional<uint32_t> ZipAssetsProvider::GetCrc(std::string_view path) const {
   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 @@ bool DirectoryAssetsProvider::ForEachFile(
   return true;
 }
 
+std::optional<std::string_view> DirectoryAssetsProvider::GetPath() const {
+  return dir_;
+}
+
 const std::string& DirectoryAssetsProvider::GetDebugName() const {
   return dir_;
 }
@@ -336,13 +347,9 @@ MultiAssetsProvider::MultiAssetsProvider(std::unique_ptr<AssetsProvider>&& prima
                                          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_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName();
-    debug_name_ = secondary_->GetDebugName();
+  path_ = (primary_->GetDebugName() != kEmptyDebugString) ? primary_->GetPath()
-  } else if (secondary_->GetDebugName() == kEmptyDebugString) {
+                                                          : secondary_->GetPath();
-    debug_name_ = primary_->GetDebugName();
-  } else {
-    debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName();
-  }
 }
 
 std::unique_ptr<AssetsProvider> MultiAssetsProvider::Create(
@@ -367,6 +374,10 @@ bool MultiAssetsProvider::ForEachFile(const std::string& root_path,
   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 @@ bool EmptyAssetsProvider::ForEachFile(
   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;

libs/androidfw/include/androidfw/ApkAssets.h

@@ -34,7 +34,6 @@ namespace android {
 // 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 @@ class ApkAssets {
   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.
+  // Path to the contents of the ApkAssets on disk. The path could represent an APk, a directory,
-  //  With the introduction of ResourcesProviders, not all ApkAssets have paths. This could cause
+  // or some other file type.
-  //  bugs when path is used for comparison because multiple ApkAssets could have the same "firendly
+  std::optional<std::string_view> GetPath() const;
-  //  name". Use pointer equality instead. ResourceManager caches and reuses ApkAssets so the
+
-  //  same asset should have the same pointer.
+  const std::string& GetDebugName() const;
-  const std::string& GetPath() const;
 
   const AssetsProvider* GetAssetsProvider() const {
     return assets_provider_.get();

libs/androidfw/include/androidfw/AssetManager2.h

@@ -412,7 +412,7 @@ class AssetManager2 {
   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.

libs/androidfw/include/androidfw/AssetsProvider.h

@@ -48,6 +48,10 @@ struct AssetsProvider {
   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 @@ struct ZipAssetsProvider : public AssetsProvider {
   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 @@ struct DirectoryAssetsProvider : public AssetsProvider {
   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 @@ struct MultiAssetsProvider : public AssetsProvider {
   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 @@ struct MultiAssetsProvider : public AssetsProvider {
 
   std::unique_ptr<AssetsProvider> primary_;
   std::unique_ptr<AssetsProvider> secondary_;
+  std::optional<std::string_view> path_;
   std::string debug_name_;
 };
 
@@ -173,6 +180,7 @@ struct EmptyAssetsProvider : public AssetsProvider {
   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;
 

packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java

@@ -200,7 +200,7 @@ public class NavigationModeController implements Dumpable {
         Log.d(TAG, "  assetPaths=");
         ApkAssets[] assets = context.getResources().getAssets().getApkAssets();
         for (ApkAssets a : assets) {
-            Log.d(TAG, "    " + a.getAssetPath());
+            Log.d(TAG, "    " + a.getDebugName());
         }
     }
 }