From e859f4655eb8a586282be29d2df32e93c227f72d Mon Sep 17 00:00:00 2001 From: Mark Punzalan Date: Wed, 23 Feb 2022 00:48:38 +0000 Subject: [PATCH] Don't cache created Translators in TranslationManager. Bug: 208599087 Test: atest CtsTranslationTestCases Change-Id: I0c2f606ef8acc7001beafdd02b48e37e9734182e --- .../view/translation/TranslationManager.java | 81 ++++++------------- 1 file changed, 26 insertions(+), 55 deletions(-) diff --git a/core/java/android/view/translation/TranslationManager.java b/core/java/android/view/translation/TranslationManager.java index 6610375d46dfa..db1c606bc99b3 100644 --- a/core/java/android/view/translation/TranslationManager.java +++ b/core/java/android/view/translation/TranslationManager.java @@ -33,9 +33,9 @@ import android.os.RemoteException; import android.os.SynchronousResultReceiver; import android.util.ArrayMap; import android.util.ArraySet; +import android.util.IntArray; import android.util.Log; import android.util.Pair; -import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; import com.android.internal.util.SyncResultReceiver; @@ -102,12 +102,7 @@ public final class TranslationManager { @NonNull @GuardedBy("mLock") - private final SparseArray mTranslators = new SparseArray<>(); - - @NonNull - @GuardedBy("mLock") - private final ArrayMap mTranslatorIds = - new ArrayMap<>(); + private final IntArray mTranslatorIds = new IntArray(); @NonNull private final Handler mHandler; @@ -127,6 +122,12 @@ public final class TranslationManager { /** * Creates an on-device Translator for natural language translation. * + *

In Android 12, this method provided the same cached Translator object when given the + * same TranslationContext object. Do not use a Translator destroyed elsewhere as this will + * cause an exception on Android 12. + * + *

In later versions, this method never returns a cached Translator. + * * @param translationContext {@link TranslationContext} containing the specs for creating the * Translator. * @param executor Executor to run callback operations @@ -140,45 +141,25 @@ public final class TranslationManager { Objects.requireNonNull(callback, "callback cannot be null"); synchronized (mLock) { - // TODO(b/176464808): Disallow multiple Translator now, it will throw - // IllegalStateException. Need to discuss if we can allow multiple Translators. - if (mTranslatorIds.containsKey(translationContext)) { - executor.execute(() -> callback.accept( - mTranslators.get(mTranslatorIds.get(translationContext)))); - return; - } - int translatorId; do { translatorId = Math.abs(ID_GENERATOR.nextInt()); - } while (translatorId == 0 || mTranslators.indexOfKey(translatorId) >= 0); + } while (translatorId == 0 || mTranslatorIds.indexOf(translatorId) >= 0); final int tId = translatorId; - new Translator(mContext, translationContext, translatorId, this, mHandler, mService, - new Consumer() { - @Override - public void accept(Translator translator) { - if (translator == null) { - final long token = Binder.clearCallingIdentity(); - try { - executor.execute(() -> callback.accept(null)); - } finally { - Binder.restoreCallingIdentity(token); - } - return; - } - - synchronized (mLock) { - mTranslators.put(tId, translator); - mTranslatorIds.put(translationContext, tId); - } - final long token = Binder.clearCallingIdentity(); - try { - executor.execute(() -> callback.accept(translator)); - } finally { - Binder.restoreCallingIdentity(token); - } + new Translator(mContext, translationContext, tId, this, mHandler, mService, + translator -> { + if (translator == null) { + Binder.withCleanCallingIdentity( + () -> executor.execute(() -> callback.accept(null))); + return; } + + synchronized (mLock) { + mTranslatorIds.add(tId); + } + Binder.withCleanCallingIdentity( + () -> executor.execute(() -> callback.accept(translator))); }); } } @@ -201,16 +182,10 @@ public final class TranslationManager { Objects.requireNonNull(translationContext, "translationContext cannot be null"); synchronized (mLock) { - // TODO(b/176464808): Disallow multiple Translator now, it will throw - // IllegalStateException. Need to discuss if we can allow multiple Translators. - if (mTranslatorIds.containsKey(translationContext)) { - return mTranslators.get(mTranslatorIds.get(translationContext)); - } - int translatorId; do { translatorId = Math.abs(ID_GENERATOR.nextInt()); - } while (translatorId == 0 || mTranslators.indexOfKey(translatorId) >= 0); + } while (translatorId == 0 || mTranslatorIds.indexOf(translatorId) >= 0); final Translator newTranslator = new Translator(mContext, translationContext, translatorId, this, mHandler, mService); @@ -220,8 +195,7 @@ public final class TranslationManager { if (!newTranslator.isSessionCreated()) { return null; } - mTranslators.put(translatorId, newTranslator); - mTranslatorIds.put(translationContext, translatorId); + mTranslatorIds.add(translatorId); return newTranslator; } catch (Translator.ServiceBinderReceiver.TimeoutException e) { // TODO(b/176464808): maybe make SyncResultReceiver.TimeoutException constructor @@ -460,12 +434,9 @@ public final class TranslationManager { void removeTranslator(int id) { synchronized (mLock) { - mTranslators.remove(id); - for (int i = 0; i < mTranslatorIds.size(); i++) { - if (mTranslatorIds.valueAt(i) == id) { - mTranslatorIds.removeAt(i); - break; - } + int index = mTranslatorIds.indexOf(id); + if (index >= 0) { + mTranslatorIds.remove(index); } } }