From 39cacf2de79e201ac77e7e1100a7b0ba29abb8f5 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 16 Mar 2020 14:54:02 -0700 Subject: [PATCH] Allow using loaders on non-RM Resources instances Currently there is a limitation where ResourcesLoaders cannot be used on Resources object not created through ResourcesManager. This change creates an update handler for Resources objects that are not registered with ResourcesManager. The handler changes the loaders on the asset manager owned by the Resources instance. Bug: 151666644 Test: atest ResourceLoaderValuesTest Change-Id: I5a89f686386bdb088dc964014e7becc0c2b4770f --- core/java/android/app/ResourcesManager.java | 5 +- .../android/content/res/AssetManager.java | 43 +++++- core/java/android/content/res/Resources.java | 37 +++++- .../res/loader/test/ResourceLoaderTestBase.kt | 46 +++---- .../loader/test/ResourceLoaderValuesTest.kt | 123 ++++++++++++++---- libs/androidfw/ApkAssets.cpp | 2 +- libs/androidfw/tests/ApkAssets_test.cpp | 3 +- libs/androidfw/tests/Theme_test.cpp | 2 +- 8 files changed, 198 insertions(+), 63 deletions(-) diff --git a/core/java/android/app/ResourcesManager.java b/core/java/android/app/ResourcesManager.java index 60f61cef0b61f..5f756033390b8 100644 --- a/core/java/android/app/ResourcesManager.java +++ b/core/java/android/app/ResourcesManager.java @@ -1288,7 +1288,8 @@ public class ResourcesManager { * instance uses. */ @Override - public void onLoadersChanged(Resources resources, List newLoader) { + public void onLoadersChanged(@NonNull Resources resources, + @NonNull List newLoader) { synchronized (ResourcesManager.this) { final ResourcesKey oldKey = findKeyForResourceImplLocked(resources.getImpl()); if (oldKey == null) { @@ -1316,7 +1317,7 @@ public class ResourcesManager { * {@code loader} to apply any changes of the set of {@link ApkAssets}. **/ @Override - public void onLoaderUpdated(ResourcesLoader loader) { + public void onLoaderUpdated(@NonNull ResourcesLoader loader) { synchronized (ResourcesManager.this) { final ArrayMap updatedResourceImplKeys = new ArrayMap<>(); diff --git a/core/java/android/content/res/AssetManager.java b/core/java/android/content/res/AssetManager.java index 7b2b93949a9fb..d2103af1d247b 100644 --- a/core/java/android/content/res/AssetManager.java +++ b/core/java/android/content/res/AssetManager.java @@ -148,12 +148,9 @@ public final class AssetManager implements AutoCloseable { final List currentLoaderApkAssets = mLoaders.get(i).getApkAssets(); for (int j = currentLoaderApkAssets.size() - 1; j >= 0; j--) { final ApkAssets apkAssets = currentLoaderApkAssets.get(j); - if (uniqueLoaderApkAssets.contains(apkAssets)) { - continue; + if (uniqueLoaderApkAssets.add(apkAssets)) { + loaderApkAssets.add(0, apkAssets); } - - uniqueLoaderApkAssets.add(apkAssets); - loaderApkAssets.add(0, apkAssets); } } @@ -328,6 +325,42 @@ public final class AssetManager implements AutoCloseable { } } + /** + * Changes the {@link ResourcesLoader ResourcesLoaders} used in this AssetManager. + * @hide + */ + void setLoaders(@NonNull List newLoaders) { + Objects.requireNonNull(newLoaders, "newLoaders"); + + final ArrayList apkAssets = new ArrayList<>(); + for (int i = 0; i < mApkAssets.length; i++) { + // Filter out the previous loader apk assets. + if (!mApkAssets[i].isForLoader()) { + apkAssets.add(mApkAssets[i]); + } + } + + if (!newLoaders.isEmpty()) { + // Filter so that assets provided by multiple loaders are only included once + // in the final assets list. The last appearance of the ApkAssets dictates its load + // order. + final int loaderStartIndex = apkAssets.size(); + final ArraySet uniqueLoaderApkAssets = new ArraySet<>(); + for (int i = newLoaders.size() - 1; i >= 0; i--) { + final List currentLoaderApkAssets = newLoaders.get(i).getApkAssets(); + for (int j = currentLoaderApkAssets.size() - 1; j >= 0; j--) { + final ApkAssets loaderApkAssets = currentLoaderApkAssets.get(j); + if (uniqueLoaderApkAssets.add(loaderApkAssets)) { + apkAssets.add(loaderStartIndex, loaderApkAssets); + } + } + } + } + + mLoaders = newLoaders.toArray(new ResourcesLoader[0]); + setApkAssets(apkAssets.toArray(new ApkAssets[0]), true /* invalidate_caches */); + } + /** * Invalidates the caches in this AssetManager according to the bitmask `diff`. * diff --git a/core/java/android/content/res/Resources.java b/core/java/android/content/res/Resources.java index e77d8af498730..d6a9f6990abee 100644 --- a/core/java/android/content/res/Resources.java +++ b/core/java/android/content/res/Resources.java @@ -66,6 +66,7 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.ArrayUtils; import com.android.internal.util.GrowingArrayUtils; +import com.android.internal.util.Preconditions; import com.android.internal.util.XmlUtils; import org.xmlpull.v1.XmlPullParser; @@ -244,7 +245,36 @@ public class Resources { * @param resources the instance being updated * @param newLoaders the new set of loaders for the instance */ - void onLoadersChanged(Resources resources, List newLoaders); + void onLoadersChanged(@NonNull Resources resources, + @NonNull List newLoaders); + } + + /** + * Handler that propagates updates of the {@link Resources} instance to the underlying + * {@link AssetManager} when the Resources is not registered with a + * {@link android.app.ResourcesManager}. + * @hide + */ + public class AssetManagerUpdateHandler implements UpdateCallbacks{ + + @Override + public void onLoadersChanged(@NonNull Resources resources, + @NonNull List newLoaders) { + Preconditions.checkArgument(Resources.this == resources); + final ResourcesImpl impl = mResourcesImpl; + impl.clearAllCaches(); + impl.getAssets().setLoaders(newLoaders); + } + + @Override + public void onLoaderUpdated(@NonNull ResourcesLoader loader) { + final ResourcesImpl impl = mResourcesImpl; + final AssetManager assets = impl.getAssets(); + if (assets.getLoaders().contains(loader)) { + impl.clearAllCaches(); + assets.setLoaders(assets.getLoaders()); + } + } } /** @@ -2367,8 +2397,9 @@ public class Resources { private void checkCallbacksRegistered() { if (mCallbacks == null) { - throw new IllegalArgumentException("Cannot modify resource loaders of Resources" - + " instances created outside of ResourcesManager"); + // Fallback to updating the underlying AssetManager if the Resources is not associated + // with a ResourcesManager. + mCallbacks = new AssetManagerUpdateHandler(); } } diff --git a/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderTestBase.kt b/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderTestBase.kt index 4764c1008d2fd..ec6a605340ae1 100644 --- a/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderTestBase.kt +++ b/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderTestBase.kt @@ -68,8 +68,7 @@ abstract class ResourceLoaderTestBase { open lateinit var dataType: DataType protected lateinit var context: Context - protected open val resources: Resources - get() = context.resources + protected lateinit var resources: Resources // Track opened streams and ResourcesProviders to close them after testing private val openedObjects = mutableListOf() @@ -78,6 +77,7 @@ abstract class ResourceLoaderTestBase { fun setUpBase() { context = InstrumentationRegistry.getTargetContext() .createConfigurationContext(Configuration()) + resources = context.resources } @After @@ -92,42 +92,42 @@ abstract class ResourceLoaderTestBase { } } - protected fun String.openProvider(dataType: DataType, assetsProvider: MemoryAssetsProvider?) - :ResourcesProvider { + protected fun String.openProvider(dataType: DataType, + assetsProvider: MemoryAssetsProvider?): ResourcesProvider { if (assetsProvider != null) { openedObjects += assetsProvider } return when (dataType) { DataType.APK_DISK_FD -> { - val file = context.copiedAssetFile("${this}.apk") + val file = context.copiedAssetFile("$this.apk") ResourcesProvider.loadFromApk(ParcelFileDescriptor.fromFd(file.fd), assetsProvider).apply { file.close() } } DataType.APK_DISK_FD_OFFSETS -> { - val asset = context.assets.openFd("${this}.apk") + val asset = context.assets.openFd("$this.apk") ResourcesProvider.loadFromApk(asset.parcelFileDescriptor, asset.startOffset, asset.length, assetsProvider).apply { asset.close() } } DataType.ARSC_DISK_FD -> { - val file = context.copiedAssetFile("${this}.arsc") + val file = context.copiedAssetFile("$this.arsc") ResourcesProvider.loadFromTable(ParcelFileDescriptor.fromFd(file.fd), assetsProvider).apply { file.close() } } DataType.ARSC_DISK_FD_OFFSETS -> { - val asset = context.assets.openFd("${this}.arsc") + val asset = context.assets.openFd("$this.arsc") ResourcesProvider.loadFromTable(asset.parcelFileDescriptor, asset.startOffset, asset.length, assetsProvider).apply { asset.close() } } DataType.APK_RAM_OFFSETS -> { - val asset = context.assets.openFd("${this}.apk") + val asset = context.assets.openFd("$this.apk") val leadingGarbageSize = 100L val trailingGarbageSize = 55L val fd = loadAssetIntoMemory(asset, leadingGarbageSize.toInt(), @@ -139,7 +139,7 @@ abstract class ResourceLoaderTestBase { } } DataType.APK_RAM_FD -> { - val asset = context.assets.openFd("${this}.apk") + val asset = context.assets.openFd("$this.apk") var fd = loadAssetIntoMemory(asset) ResourcesProvider.loadFromApk(fd, assetsProvider).apply { asset.close() @@ -147,7 +147,7 @@ abstract class ResourceLoaderTestBase { } } DataType.ARSC_RAM_MEMORY -> { - val asset = context.assets.openFd("${this}.arsc") + val asset = context.assets.openFd("$this.arsc") var fd = loadAssetIntoMemory(asset) ResourcesProvider.loadFromTable(fd, assetsProvider).apply { asset.close() @@ -155,7 +155,7 @@ abstract class ResourceLoaderTestBase { } } DataType.ARSC_RAM_MEMORY_OFFSETS -> { - val asset = context.assets.openFd("${this}.arsc") + val asset = context.assets.openFd("$this.arsc") val leadingGarbageSize = 100L val trailingGarbageSize = 55L val fd = loadAssetIntoMemory(asset, leadingGarbageSize.toInt(), @@ -175,7 +175,7 @@ abstract class ResourceLoaderTestBase { } } DataType.DIRECTORY -> { - ResourcesProvider.loadFromDirectory(zipToDir("${this}.apk").absolutePath, + ResourcesProvider.loadFromDirectory(zipToDir("$this.apk").absolutePath, assetsProvider) } DataType.SPLIT -> { @@ -186,9 +186,9 @@ abstract class ResourceLoaderTestBase { class EmptyAssetsProvider : AssetsProvider - /** */ - inner class ZipAssetsProvider(val providerName : String) : AssetsProvider { - val root: File = zipToDir("${providerName}.apk") + /** An AssetsProvider that reads from a zip asset. */ + inner class ZipAssetsProvider(val providerName: String) : AssetsProvider { + val root: File = zipToDir("$providerName.apk") override fun loadAssetFd(path: String, accessMode: Int): AssetFileDescriptor? { val f = File(root, path) @@ -203,7 +203,7 @@ abstract class ResourceLoaderTestBase { class MemoryAssetsProvider : AssetsProvider, Closeable { var loadAssetResults = HashMap() - fun addLoadAssetFdResult(path : String, value : String) = apply { + fun addLoadAssetFdResult(path: String, value: String) = apply { val fd = Os.memfd_create(path, 0) val valueBytes = value.toByteArray() Os.write(fd, valueBytes, 0, valueBytes.size) @@ -224,7 +224,7 @@ abstract class ResourceLoaderTestBase { } /** Extracts an archive-based asset into a directory on disk. */ - private fun zipToDir(name : String) : File { + private fun zipToDir(name: String): File { val root = File(context.filesDir, name.split('.')[0]) if (root.exists()) { return root @@ -252,13 +252,13 @@ abstract class ResourceLoaderTestBase { return root } - /** Loads the asset into a temporary file stored in RAM. */ - private fun loadAssetIntoMemory(asset: AssetFileDescriptor, - leadingGarbageSize: Int = 0, - trailingGarbageSize: Int = 0 + private fun loadAssetIntoMemory( + asset: AssetFileDescriptor, + leadingGarbageSize: Int = 0, + trailingGarbageSize: Int = 0 ): ParcelFileDescriptor { - val originalFd = Os.memfd_create(asset.toString(), 0 /* flags */); + val originalFd = Os.memfd_create(asset.toString(), 0 /* flags */) val fd = ParcelFileDescriptor.dup(originalFd) Os.close(originalFd) diff --git a/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderValuesTest.kt b/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderValuesTest.kt index a9945369d1df2..5aa8814c7481a 100644 --- a/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderValuesTest.kt +++ b/core/tests/ResourceLoaderTests/src/android/content/res/loader/test/ResourceLoaderValuesTest.kt @@ -19,6 +19,7 @@ package android.content.res.loader.test import android.app.Activity import android.content.Context import android.content.Intent +import android.content.res.AssetManager import android.content.res.Configuration import android.content.res.Resources import android.content.res.loader.ResourcesLoader @@ -63,22 +64,40 @@ class ResourceLoaderValuesTest : ResourceLoaderTestBase() { }, "getAdditional" to { res -> res.getString(0x7f0400fe /* R.string.additional */) + }, + "getIdentifier" to { res -> + res.getString(res.getIdentifier("test", "string", + "android.content.res.loader.test")) + }, + "getIdentifierAdditional" to { res -> + res.getString(res.getIdentifier("additional", "string", + "android.content.res.loader.test")) } )), mapOf("getOverlaid" to "Not overlaid", - "getAdditional" to "NotFoundException"), + "getAdditional" to "NotFoundException", + "getIdentifier" to "Not overlaid", + "getIdentifierAdditional" to "NotFoundException"), mapOf("getOverlaid" to "One", - "getAdditional" to "One"), + "getAdditional" to "One", + "getIdentifier" to "One", + "getIdentifierAdditional" to "One"), mapOf("getOverlaid" to "Two", - "getAdditional" to "Two"), + "getAdditional" to "Two", + "getIdentifier" to "Two", + "getIdentifierAdditional" to "Two"), mapOf("getOverlaid" to "Three", - "getAdditional" to "Three"), + "getAdditional" to "Three", + "getIdentifier" to "Three", + "getIdentifierAdditional" to "Three"), mapOf("getOverlaid" to "Four", - "getAdditional" to "Four"), + "getAdditional" to "Four", + "getIdentifier" to "Four", + "getIdentifierAdditional" to "Four"), listOf(DataType.APK_DISK_FD, DataType.APK_DISK_FD_OFFSETS, DataType.APK_RAM_FD, DataType.APK_RAM_OFFSETS, DataType.ARSC_DISK_FD, DataType.ARSC_DISK_FD_OFFSETS, DataType.ARSC_RAM_MEMORY, @@ -224,7 +243,7 @@ class ResourceLoaderValuesTest : ResourceLoaderTestBase() { lateinit var parameter: Parameter private val valueOriginal by lazy { mapToString(parameter.valueOriginal) } - private val valueOne by lazy { mapToString(parameter.valueOne) } + private val valueOne by lazy { mapToString(parameter.valueOne) } private val valueTwo by lazy { mapToString(parameter.valueTwo) } private val valueThree by lazy { mapToString(parameter.valueThree) } private val valueFour by lazy { mapToString(parameter.valueFour) } @@ -241,6 +260,7 @@ class ResourceLoaderValuesTest : ResourceLoaderTestBase() { // Class method for syntax highlighting purposes private fun getValue(c: Context = context) = parameter.getValue(c.resources) + private fun getValue(r: Resources) = parameter.getValue(r) @Test fun assertValueUniqueness() { @@ -713,28 +733,79 @@ class ResourceLoaderValuesTest : ResourceLoaderTestBase() { provider.close() } + @Test + fun addLoadersRepeatedlyCustomResources() { + val res = Resources(AssetManager::class.java.newInstance(), resources.displayMetrics, + resources.configuration!!) + val originalValue = getValue(res) + val testOne = openOne() + val testTwo = openTwo() + val loader1 = ResourcesLoader() + val loader2 = ResourcesLoader() + + res.addLoaders(loader1) + loader1.addProvider(testOne) + assertEquals(valueOne, getValue(res)) + + res.addLoaders(loader2) + loader2.addProvider(testTwo) + assertEquals(valueTwo, getValue(res)) + + res.removeLoaders(loader1) + res.addLoaders(loader1) + assertEquals(valueOne, getValue(res)) + + res.removeLoaders(loader1) + assertEquals(valueTwo, getValue(res)) + + res.removeLoaders(loader2) + assertEquals(originalValue, getValue(res)) + } + + @Test + fun setMultipleProvidersCustomResources() { + val res = Resources(AssetManager::class.java.newInstance(), resources.displayMetrics, + resources.configuration!!) + val originalValue = getValue(res) + val testOne = openOne() + val testTwo = openTwo() + val loader = ResourcesLoader() + + res.addLoaders(loader) + loader.providers = listOf(testOne, testTwo) + assertEquals(valueTwo, getValue(res)) + + loader.removeProvider(testTwo) + assertEquals(valueOne, getValue(res)) + + loader.providers = Collections.emptyList() + assertEquals(originalValue, getValue(res)) + } + data class Parameter( - val testPrefix: String, - val getValue: Resources.() -> String, - val valueOriginal: Map, - val valueOne: Map, - val assetProviderOne: (() -> MemoryAssetsProvider)? = null, - val valueTwo: Map, - val assetProviderTwo: (() -> MemoryAssetsProvider)? = null, - val valueThree: Map, - val assetProviderThree: (() -> MemoryAssetsProvider)? = null, - val valueFour: Map, - val assetProviderFour: (() -> MemoryAssetsProvider)? = null, - val dataTypes: List + val testPrefix: String, + val getValue: Resources.() -> String, + val valueOriginal: Map, + val valueOne: Map, + val assetProviderOne: (() -> MemoryAssetsProvider)? = null, + val valueTwo: Map, + val assetProviderTwo: (() -> MemoryAssetsProvider)? = null, + val valueThree: Map, + val assetProviderThree: (() -> MemoryAssetsProvider)? = null, + val valueFour: Map, + val assetProviderFour: (() -> MemoryAssetsProvider)? = null, + val dataTypes: List ) { - constructor(testPrefix: String, - getValue: Resources.() -> String, - valueOriginal : Map, - valueOne: Map, - valueTwo: Map, - valueThree: Map, - valueFour: Map, - dataTypes: List): this(testPrefix, getValue, valueOriginal, valueOne, + constructor( + testPrefix: String, + getValue: Resources.() -> String, + valueOriginal: Map, + valueOne: Map, + valueTwo: Map, + valueThree: Map, + valueFour: Map, + dataTypes: List + ): this(testPrefix, getValue, valueOriginal, valueOne, null, valueTwo, null, valueThree, null, valueFour, null, dataTypes) override fun toString() = testPrefix diff --git a/libs/androidfw/ApkAssets.cpp b/libs/androidfw/ApkAssets.cpp index 202651dc86d58..918e7af12d312 100644 --- a/libs/androidfw/ApkAssets.cpp +++ b/libs/androidfw/ApkAssets.cpp @@ -471,7 +471,7 @@ std::unique_ptr ApkAssets::LoadImpl( bool resources_asset_exists = false; auto resources_asset_ = assets->Open(kResourcesArsc, Asset::AccessMode::ACCESS_BUFFER, &resources_asset_exists); - + assets = MultiAssetsProvider::Create(std::move(override_assets), std::move(assets)); // Wrap the handle in a unique_ptr so it gets automatically closed. diff --git a/libs/androidfw/tests/ApkAssets_test.cpp b/libs/androidfw/tests/ApkAssets_test.cpp index ce9e53244801b..19db25ce88116 100644 --- a/libs/androidfw/tests/ApkAssets_test.cpp +++ b/libs/androidfw/tests/ApkAssets_test.cpp @@ -50,8 +50,7 @@ TEST(ApkAssetsTest, LoadApkFromFd) { unique_fd fd(::open(path.c_str(), O_RDONLY | O_BINARY)); ASSERT_THAT(fd.get(), Ge(0)); - std::unique_ptr loaded_apk = - ApkAssets::LoadFromFd(std::move(fd), path, false /*system*/, false /*force_shared_lib*/); + std::unique_ptr loaded_apk = ApkAssets::LoadFromFd(std::move(fd), path); ASSERT_THAT(loaded_apk, NotNull()); const LoadedArsc* loaded_arsc = loaded_apk->GetLoadedArsc(); diff --git a/libs/androidfw/tests/Theme_test.cpp b/libs/androidfw/tests/Theme_test.cpp index be5ecd94a588b..16b9c75982fbc 100644 --- a/libs/androidfw/tests/Theme_test.cpp +++ b/libs/androidfw/tests/Theme_test.cpp @@ -36,7 +36,7 @@ namespace android { class ThemeTest : public ::testing::Test { public: void SetUp() override { - system_assets_ = ApkAssets::Load(GetTestDataPath() + "/system/system.apk", true /*system*/); + system_assets_ = ApkAssets::Load(GetTestDataPath() + "/system/system.apk", PROPERTY_SYSTEM); ASSERT_NE(nullptr, system_assets_); style_assets_ = ApkAssets::Load(GetTestDataPath() + "/styles/styles.apk");