[automerge] Add defensive try-catch to ModelDownloader inside TCS. 2p: af378fae69

Original change: https://googleplex-android-review.googlesource.com/c/platform/external/libtextclassifier/+/16931243

Bug: 219998215
Change-Id: I8c8ada678a557b4903deb188e01b9f197278ec4d

3 files changed, 158 insertions, 68 deletions

diff --git a/java/src/com/android/textclassifier/downloader/ModelDownloadManager.java b/java/src/com/android/textclassifier/downloader/ModelDownloadManager.java
index b125f13..3614d35 100644
--- a/java/src/com/android/textclassifier/downloader/ModelDownloadManager.java
+++ b/java/src/com/android/textclassifier/downloader/ModelDownloadManager.java
@@ -44,6 +44,7 @@ import com.android.textclassifier.utils.IndentingPrintWriter;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Enums;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.hash.Hashing;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
@@ -54,6 +55,7 @@ import java.time.Instant;
 import java.util.List;
 import java.util.Locale;
 import java.util.UUID;
+import java.util.concurrent.Callable;
 import javax.annotation.Nullable;
 
 /** Manager to listen to config update and download latest models. */
@@ -64,6 +66,7 @@ public final class ModelDownloadManager {
 
   private final Context appContext;
   private final Class<? extends ListenableWorker> modelDownloadWorkerClass;
+  private final Callable<WorkManager> workManagerSupplier;
   private final DownloadedModelManager downloadedModelManager;
   private final TextClassifierSettings settings;
   private final ListeningExecutorService executorService;
@@ -84,6 +87,7 @@ public final class ModelDownloadManager {
     this(
         appContext,
         ModelDownloadWorker.class,
+        () -> WorkManager.getInstance(appContext),
         DownloadedModelManagerImpl.getInstance(appContext),
         settings,
         executorService);
@@ -93,11 +97,13 @@ public final class ModelDownloadManager {
   public ModelDownloadManager(
       Context appContext,
       Class<? extends ListenableWorker> modelDownloadWorkerClass,
+      Callable<WorkManager> workManagerSupplier,
       DownloadedModelManager downloadedModelManager,
       TextClassifierSettings settings,
       ListeningExecutorService executorService) {
     this.appContext = Preconditions.checkNotNull(appContext);
     this.modelDownloadWorkerClass = Preconditions.checkNotNull(modelDownloadWorkerClass);
+    this.workManagerSupplier = Preconditions.checkNotNull(workManagerSupplier);
     this.downloadedModelManager = Preconditions.checkNotNull(downloadedModelManager);
     this.settings = Preconditions.checkNotNull(settings);
     this.executorService = Preconditions.checkNotNull(executorService);
@@ -121,22 +127,31 @@ public final class ModelDownloadManager {
   /** Returns the downlaoded models for the given modelType. */
   @Nullable
   public List<File> listDownloadedModels(@ModelTypeDef String modelType) {
-    return downloadedModelManager.listModels(modelType);
+    try {
+      return downloadedModelManager.listModels(modelType);
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed to list downloaded models", t);
+      return ImmutableList.of();
+    }
   }
 
   /** Notifies the model downlaoder that the text classifier service is created. */
   public void onTextClassifierServiceCreated() {
-    DeviceConfig.addOnPropertiesChangedListener(
-        DeviceConfig.NAMESPACE_TEXTCLASSIFIER, executorService, deviceConfigListener);
-    appContext.registerReceiver(
-        localeChangedReceiver, new IntentFilter(Intent.ACTION_LOCALE_CHANGED));
-    TcLog.d(TAG, "DeviceConfig listener and locale change listener are registered.");
-    if (!settings.isModelDownloadManagerEnabled()) {
-      return;
+    try {
+      DeviceConfig.addOnPropertiesChangedListener(
+          DeviceConfig.NAMESPACE_TEXTCLASSIFIER, executorService, deviceConfigListener);
+      appContext.registerReceiver(
+          localeChangedReceiver, new IntentFilter(Intent.ACTION_LOCALE_CHANGED));
+      TcLog.d(TAG, "DeviceConfig listener and locale change listener are registered.");
+      if (!settings.isModelDownloadManagerEnabled()) {
+        return;
+      }
+      maybeOverrideLocaleListForTesting();
+      TcLog.v(TAG, "Try to schedule model download work because TextClassifierService started.");
+      scheduleDownloadWork(REASON_TO_SCHEDULE_TCS_STARTED);
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed inside onTextClassifierServiceCreated", t);
     }
-    maybeOverrideLocaleListForTesting();
-    TcLog.v(TAG, "Try to schedule model download work because TextClassifierService started.");
-    scheduleDownloadWork(REASON_TO_SCHEDULE_TCS_STARTED);
   }
 
   // TODO(licha): Make this private. Let the constructor accept a receiver to enable testing.
@@ -147,7 +162,11 @@ public final class ModelDownloadManager {
       return;
     }
     TcLog.v(TAG, "Try to schedule model download work because of system locale changes.");
-    scheduleDownloadWork(REASON_TO_SCHEDULE_LOCALE_SETTINGS_CHANGED);
+    try {
+      scheduleDownloadWork(REASON_TO_SCHEDULE_LOCALE_SETTINGS_CHANGED);
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed inside onLocaleChanged", t);
+    }
   }
 
   // TODO(licha): Make this private. Let the constructor accept a receiver to enable testing.
@@ -157,16 +176,24 @@ public final class ModelDownloadManager {
     if (!settings.isModelDownloadManagerEnabled()) {
       return;
     }
-    maybeOverrideLocaleListForTesting();
     TcLog.v(TAG, "Try to schedule model download work because of device config changes.");
-    scheduleDownloadWork(REASON_TO_SCHEDULE_DEVICE_CONFIG_UPDATED);
+    try {
+      maybeOverrideLocaleListForTesting();
+      scheduleDownloadWork(REASON_TO_SCHEDULE_DEVICE_CONFIG_UPDATED);
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed inside onTextClassifierDeviceConfigChanged", t);
+    }
   }
 
   /** Clean up internal states on destroying. */
   public void destroy() {
-    DeviceConfig.removeOnPropertiesChangedListener(deviceConfigListener);
-    appContext.unregisterReceiver(localeChangedReceiver);
-    TcLog.d(TAG, "DeviceConfig and Locale listener unregistered by ModelDownloadeManager");
+    try {
+      DeviceConfig.removeOnPropertiesChangedListener(deviceConfigListener);
+      appContext.unregisterReceiver(localeChangedReceiver);
+      TcLog.d(TAG, "DeviceConfig and Locale listener unregistered by ModelDownloadeManager");
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed to destroy ModelDownloadManager", t);
+    }
   }
 
   /**
@@ -178,10 +205,14 @@ public final class ModelDownloadManager {
     if (!settings.isModelDownloadManagerEnabled()) {
       return;
     }
-    printWriter.println("ModelDownloadManager:");
-    printWriter.increaseIndent();
-    downloadedModelManager.dump(printWriter);
-    printWriter.decreaseIndent();
+    try {
+      printWriter.println("ModelDownloadManager:");
+      printWriter.increaseIndent();
+      downloadedModelManager.dump(printWriter);
+      printWriter.decreaseIndent();
+    } catch (Throwable t) {
+      TcLog.e(TAG, "Failed to dump ModelDownloadManager", t);
+    }
   }
 
   /**
@@ -193,54 +224,62 @@ public final class ModelDownloadManager {
   private void scheduleDownloadWork(int reasonToSchedule) {
     long workId =
         Hashing.farmHashFingerprint64().hashUnencodedChars(UUID.randomUUID().toString()).asLong();
-    NetworkType networkType =
-        Enums.getIfPresent(NetworkType.class, settings.getManifestDownloadRequiredNetworkType())
-            .or(NetworkType.UNMETERED);
-    OneTimeWorkRequest downloadRequest =
-        new OneTimeWorkRequest.Builder(modelDownloadWorkerClass)
-            .setConstraints(
-                new Constraints.Builder()
-                    .setRequiredNetworkType(networkType)
-                    .setRequiresBatteryNotLow(true)
-                    .setRequiresStorageNotLow(true)
-                    .setRequiresDeviceIdle(settings.getManifestDownloadRequiresDeviceIdle())
-                    .setRequiresCharging(settings.getManifestDownloadRequiresCharging())
-                    .build())
-            .setBackoffCriteria(
-                BackoffPolicy.EXPONENTIAL,
-                settings.getModelDownloadBackoffDelayInMillis(),
-                MILLISECONDS)
-            .setInputData(
-                new Data.Builder()
-                    .putLong(ModelDownloadWorker.INPUT_DATA_KEY_WORK_ID, workId)
-                    .putLong(
-                        ModelDownloadWorker.INPUT_DATA_KEY_SCHEDULED_TIMESTAMP,
-                        Instant.now().toEpochMilli())
-                    .build())
-            .build();
-    ListenableFuture<Operation.State.SUCCESS> enqueueResultFuture =
-        WorkManager.getInstance(appContext)
-            .enqueueUniqueWork(
-                UNIQUE_QUEUE_NAME, ExistingWorkPolicy.APPEND_OR_REPLACE, downloadRequest)
-            .getResult();
-    Futures.addCallback(
-        enqueueResultFuture,
-        new FutureCallback<Operation.State.SUCCESS>() {
-          @Override
-          public void onSuccess(Operation.State.SUCCESS unused) {
-            TcLog.v(TAG, "Download work scheduled.");
-            TextClassifierDownloadLogger.downloadWorkScheduled(
-                workId, reasonToSchedule, /* failedToSchedule= */ false);
-          }
+    try {
+      NetworkType networkType =
+          Enums.getIfPresent(NetworkType.class, settings.getManifestDownloadRequiredNetworkType())
+              .or(NetworkType.UNMETERED);
+      OneTimeWorkRequest downloadRequest =
+          new OneTimeWorkRequest.Builder(modelDownloadWorkerClass)
+              .setConstraints(
+                  new Constraints.Builder()
+                      .setRequiredNetworkType(networkType)
+                      .setRequiresBatteryNotLow(true)
+                      .setRequiresStorageNotLow(true)
+                      .setRequiresDeviceIdle(settings.getManifestDownloadRequiresDeviceIdle())
+                      .setRequiresCharging(settings.getManifestDownloadRequiresCharging())
+                      .build())
+              .setBackoffCriteria(
+                  BackoffPolicy.EXPONENTIAL,
+                  settings.getModelDownloadBackoffDelayInMillis(),
+                  MILLISECONDS)
+              .setInputData(
+                  new Data.Builder()
+                      .putLong(ModelDownloadWorker.INPUT_DATA_KEY_WORK_ID, workId)
+                      .putLong(
+                          ModelDownloadWorker.INPUT_DATA_KEY_SCHEDULED_TIMESTAMP,
+                          Instant.now().toEpochMilli())
+                      .build())
+              .build();
+      ListenableFuture<Operation.State.SUCCESS> enqueueResultFuture =
+          workManagerSupplier
+              .call()
+              .enqueueUniqueWork(
+                  UNIQUE_QUEUE_NAME, ExistingWorkPolicy.APPEND_OR_REPLACE, downloadRequest)
+              .getResult();
+      Futures.addCallback(
+          enqueueResultFuture,
+          new FutureCallback<Operation.State.SUCCESS>() {
+            @Override
+            public void onSuccess(Operation.State.SUCCESS unused) {
+              TcLog.v(TAG, "Download work scheduled.");
+              TextClassifierDownloadLogger.downloadWorkScheduled(
+                  workId, reasonToSchedule, /* failedToSchedule= */ false);
+            }
 
-          @Override
-          public void onFailure(Throwable t) {
-            TcLog.e(TAG, "Failed to schedule download work: ", t);
-            TextClassifierDownloadLogger.downloadWorkScheduled(
-                workId, reasonToSchedule, /* failedToSchedule= */ true);
-          }
-        },
-        executorService);
+            @Override
+            public void onFailure(Throwable t) {
+              TcLog.e(TAG, "Failed to schedule download work: ", t);
+              TextClassifierDownloadLogger.downloadWorkScheduled(
+                  workId, reasonToSchedule, /* failedToSchedule= */ true);
+            }
+          },
+          executorService);
+    } catch (Throwable t) {
+      // TODO(licha): this is just for temporary fix. Refactor the try-catch in the future.
+      TcLog.e(TAG, "Failed to schedule download work: ", t);
+      TextClassifierDownloadLogger.downloadWorkScheduled(
+          workId, reasonToSchedule, /* failedToSchedule= */ true);
+    }
   }
 
   private void maybeOverrideLocaleListForTesting() {
diff --git a/java/tests/instrumentation/src/com/android/textclassifier/ModelFileManagerImplTest.java b/java/tests/instrumentation/src/com/android/textclassifier/ModelFileManagerImplTest.java
index 20ae592..0e40515 100644
--- a/java/tests/instrumentation/src/com/android/textclassifier/ModelFileManagerImplTest.java
+++ b/java/tests/instrumentation/src/com/android/textclassifier/ModelFileManagerImplTest.java
@@ -25,6 +25,7 @@ import android.os.LocaleList;
 import androidx.test.core.app.ApplicationProvider;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import androidx.test.filters.SmallTest;
+import androidx.work.WorkManager;
 import com.android.textclassifier.ModelFileManagerImpl.DownloaderModelsLister;
 import com.android.textclassifier.ModelFileManagerImpl.RegularFileFullMatchLister;
 import com.android.textclassifier.ModelFileManagerImpl.RegularFilePatternMatchLister;
@@ -87,6 +88,7 @@ public final class ModelFileManagerImplTest {
         new ModelDownloadManager(
             context,
             ModelDownloadWorker.class,
+            () -> WorkManager.getInstance(context),
             downloadedModelManager,
             settings,
             MoreExecutors.newDirectExecutorService());
diff --git a/java/tests/instrumentation/src/com/android/textclassifier/downloader/ModelDownloadManagerTest.java b/java/tests/instrumentation/src/com/android/textclassifier/downloader/ModelDownloadManagerTest.java
index 394b7ad..5a67f93 100644
--- a/java/tests/instrumentation/src/com/android/textclassifier/downloader/ModelDownloadManagerTest.java
+++ b/java/tests/instrumentation/src/com/android/textclassifier/downloader/ModelDownloadManagerTest.java
@@ -67,6 +67,7 @@ public final class ModelDownloadManagerTest {
   private TestingDeviceConfig deviceConfig;
   private WorkManager workManager;
   private ModelDownloadManager downloadManager;
+  private ModelDownloadManager downloadManagerWithBadWorkManager;
   @Mock DownloadedModelManager downloadedModelManager;
 
   @Before
@@ -80,6 +81,17 @@ public final class ModelDownloadManagerTest {
         new ModelDownloadManager(
             context,
             ModelDownloadWorker.class,
+            () -> workManager,
+            downloadedModelManager,
+            new TextClassifierSettings(deviceConfig),
+            MoreExecutors.newDirectExecutorService());
+    this.downloadManagerWithBadWorkManager =
+        new ModelDownloadManager(
+            context,
+            ModelDownloadWorker.class,
+            () -> {
+              throw new IllegalStateException("WorkManager may fail!");
+            },
             downloadedModelManager,
             new TextClassifierSettings(deviceConfig),
             MoreExecutors.newDirectExecutorService());
@@ -96,6 +108,16 @@ public final class ModelDownloadManagerTest {
   }
 
   @Test
+  public void onTextClassifierServiceCreated_workManagerCrashed() throws Exception {
+    downloadManagerWithBadWorkManager.onTextClassifierServiceCreated();
+
+    TextClassifierDownloadWorkScheduled atom =
+        Iterables.getOnlyElement(loggerTestRule.getLoggedDownloadWorkScheduledAtoms());
+    assertThat(atom.getReasonToSchedule()).isEqualTo(ReasonToSchedule.TCS_STARTED);
+    assertThat(atom.getFailedToSchedule()).isTrue();
+  }
+
+  @Test
   public void onTextClassifierServiceCreated_requestEnqueued() throws Exception {
     downloadManager.onTextClassifierServiceCreated();
 
@@ -119,6 +141,16 @@ public final class ModelDownloadManagerTest {
   }
 
   @Test
+  public void onLocaleChanged_workManagerCrashed() throws Exception {
+    downloadManagerWithBadWorkManager.onLocaleChanged();
+
+    TextClassifierDownloadWorkScheduled atom =
+        Iterables.getOnlyElement(loggerTestRule.getLoggedDownloadWorkScheduledAtoms());
+    assertThat(atom.getReasonToSchedule()).isEqualTo(ReasonToSchedule.LOCALE_SETTINGS_CHANGED);
+    assertThat(atom.getFailedToSchedule()).isTrue();
+  }
+
+  @Test
   public void onLocaleChanged_requestEnqueued() throws Exception {
     downloadManager.onLocaleChanged();
 
@@ -131,6 +163,16 @@ public final class ModelDownloadManagerTest {
   }
 
   @Test
+  public void onTextClassifierDeviceConfigChanged_workManagerCrashed() throws Exception {
+    downloadManagerWithBadWorkManager.onTextClassifierDeviceConfigChanged();
+
+    TextClassifierDownloadWorkScheduled atom =
+        Iterables.getOnlyElement(loggerTestRule.getLoggedDownloadWorkScheduledAtoms());
+    assertThat(atom.getReasonToSchedule()).isEqualTo(ReasonToSchedule.DEVICE_CONFIG_UPDATED);
+    assertThat(atom.getFailedToSchedule()).isTrue();
+  }
+
+  @Test
   public void onTextClassifierDeviceConfigChanged_requestEnqueued() throws Exception {
     downloadManager.onTextClassifierDeviceConfigChanged();
 
@@ -188,6 +230,13 @@ public final class ModelDownloadManagerTest {
     assertThat(downloadManager.listDownloadedModels(MODEL_TYPE)).containsExactly(modelFile);
   }
 
+  @Test
+  public void listDownloadedModels_doNotCrashOnError() throws Exception {
+    when(downloadedModelManager.listModels(MODEL_TYPE)).thenThrow(new IllegalStateException());
+
+    assertThat(downloadManager.listDownloadedModels(MODEL_TYPE)).isEmpty();
+  }
+
   private void verifyWorkScheduledLogging(ReasonToSchedule reasonToSchedule) throws Exception {
     TextClassifierDownloadWorkScheduled atom =
         Iterables.getOnlyElement(loggerTestRule.getLoggedDownloadWorkScheduledAtoms());