move more jobs to disk write (#6156)

Move file clean up task to disk write worker.
Fix unit test: close session has to run on a background thread

Author: themiswang

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java

@@ -213,9 +213,10 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal()
         .thenReturn(new TreeSet<>(Collections.singleton(sessionId)));
 
     controller.writeNonFatalException(thread, nonFatal);
-    controller.doCloseSessions(testSettingsProvider);
+    crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(testSettingsProvider));
 
     crashlyticsWorkers.common.await();
+    crashlyticsWorkers.diskWrite.await();
 
     verify(mockSessionReportingCoordinator)
         .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong());
@@ -489,6 +490,8 @@ public void testUploadDisabledThenOptOut() throws Exception {
     await(controller.deleteUnsentReports());
     await(task);
 
+    crashlyticsWorkers.diskWrite.await();
+
     verify(mockSessionReportingCoordinator).removeAllReports();
     verifyNoMoreInteractions(mockSessionReportingCoordinator);
   }

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java

@@ -170,6 +170,7 @@ public void testWriteUserData_escaped() throws Exception {
     crashlyticsWorkers.diskWrite.await();
     UserMetadata userData =
         UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers);
+    Thread.sleep(10);
     assertEquals(ESCAPED.trim(), userData.getUserId());
   }
 

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java

@@ -205,7 +205,6 @@ public Task<Void> call() throws Exception {
 
                 // We've fatally crashed, so write the marker file that indicates a crash occurred.
                 crashMarker.create();
-
                 reportingCoordinator.persistFatalEvent(
                     ex, thread, currentSessionId, timestampSeconds);
 
@@ -363,8 +362,11 @@ public Task<Void> then(@Nullable Boolean send) throws Exception {
                       public Task<Void> call() throws Exception {
                         if (!send) {
                           Logger.getLogger().v("Deleting cached crash reports...");
-                          deleteFiles(listAppExceptionMarkerFiles());
-                          reportingCoordinator.removeAllReports();
+                          crashlyticsWorkers.diskWrite.submit(
+                              () -> {
+                                deleteFiles(listAppExceptionMarkerFiles());
+                                reportingCoordinator.removeAllReports();
+                              });
                           unsentReportsHandled.trySetResult(null);
                           return Tasks.forResult(null);
                         }
@@ -512,7 +514,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) {
 
     Logger.getLogger().v("Finalizing previously open sessions.");
     try {
-      doCloseSessions(true, settingsProvider);
+      doCloseSessions(true, settingsProvider, true);
     } catch (Exception e) {
       Logger.getLogger().e("Unable to finalize previously open sessions.", e);
       return false;
@@ -557,15 +559,19 @@ private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) {
     reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds);
   }
 
+  // This is only used for exception handler close session (we have another close session in
+  // background initialization)
   void doCloseSessions(SettingsProvider settingsProvider) {
-    doCloseSessions(false, settingsProvider);
+    doCloseSessions(false, settingsProvider, false);
   }
 
   /**
-   * Not synchronized/locked. Must be executed from the single thread executor service used by this
-   * class.
+   *
+   * Not synchronized/locked. Must be executed from the executor service runs tasks in serial order
    */
-  private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settingsProvider) {
+  private void doCloseSessions(
+      boolean skipCurrentSession, SettingsProvider settingsProvider, boolean isInitProcess) {
+    CrashlyticsWorkers.checkBackgroundThread();
     final int offset = skipCurrentSession ? 1 : 0;
 
     // :TODO HW2021 this implementation can be cleaned up.
@@ -579,13 +585,15 @@ private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settin
 
     final String mostRecentSessionIdToClose = sortedOpenSessions.get(offset);
 
-    if (settingsProvider.getSettingsSync().featureFlagData.collectAnrs) {
+    // We only collect ANR info for finalize report during initialization process
+    if (isInitProcess && settingsProvider.getSettingsSync().featureFlagData.collectAnrs) {
       writeApplicationExitInfoEventIfRelevant(mostRecentSessionIdToClose);
     } else {
       Logger.getLogger().v("ANR feature disabled.");
     }
 
-    if (nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) {
+    // We only collect native crash info for finalize report during initialization process
+    if (isInitProcess && nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) {
       // We only finalize the current session if it's a Java crash, so only finalize native crash
       // data when we aren't including current.
       finalizePreviousNativeSession(mostRecentSessionIdToClose);

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java

@@ -354,12 +354,16 @@ private boolean onReportSendComplete(@NonNull Task<CrashlyticsReportWithSessionI
       CrashlyticsReportWithSessionId report = task.getResult();
       Logger.getLogger()
           .d("Crashlytics report successfully enqueued to DataTransport: " + report.getSessionId());
-      File reportFile = report.getReportFile();
-      if (reportFile.delete()) {
-        Logger.getLogger().d("Deleted report file: " + reportFile.getPath());
-      } else {
-        Logger.getLogger().w("Crashlytics could not delete report file: " + reportFile.getPath());
-      }
+      crashlyticsWorkers.diskWrite.submit(
+          () -> {
+            File reportFile = report.getReportFile();
+            if (reportFile.delete()) {
+              Logger.getLogger().d("Deleted report file: " + reportFile.getPath());
+            } else {
+              Logger.getLogger()
+                  .w("Crashlytics could not delete report file: " + reportFile.getPath());
+            }
+          });
       return true;
     }
     Logger.getLogger()

firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java

@@ -43,6 +43,7 @@
 import com.google.firebase.crashlytics.internal.settings.SettingsProvider;
 import com.google.firebase.inject.Deferred;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.TreeSet;
@@ -65,6 +66,8 @@ public class CrashlyticsControllerRobolectricTest {
   @Mock private SessionReportingCoordinator mockSessionReportingCoordinator;
   @Mock private DataCollectionArbiter mockDataCollectionArbiter;
 
+  private CrashlyticsWorkers crashlyticsWorkers;
+
   private static final CrashlyticsNativeComponent MISSING_NATIVE_COMPONENT =
       new CrashlyticsNativeComponentDeferredProxy(
           new Deferred<CrashlyticsNativeComponent>() {
@@ -80,17 +83,24 @@ public void setUp() {
     MockitoAnnotations.openMocks(this);
     testContext = getApplicationContext();
     testFileStore = new FileStore(testContext);
+    crashlyticsWorkers =
+        new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());
   }
 
   @Test
-  public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist() {
+  public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist()
+      throws Exception {
     final String sessionId = "sessionId";
     final CrashlyticsController controller = createController();
 
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
         .thenReturn(new TreeSet<>(Collections.singletonList(sessionId)));
     mockSettingsProvider(true, false);
-    controller.doCloseSessions(mockSettingsProvider);
+
+    crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider));
+    // cannot use await since it check preconditions if blocking main thread
+    Thread.sleep(10);
+
     // Since we haven't added any app exit info to the shadow activity manager, there won't exist a
     // single app exit info, and so this method won't be called.
     verify(mockSessionReportingCoordinator, never())
@@ -99,7 +109,8 @@ public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntE
   }
 
   @Test
-  public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() {
+  public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() throws Exception {
+    final String sessionIdPrevious = "sessionIdPrevious";
     final String sessionId = "sessionId";
     final CrashlyticsController controller = createController();
     // Adds multiple AppExitInfos to confirm that Crashlytics loops through
@@ -109,26 +120,30 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() {
     List<ApplicationExitInfo> testApplicationExitInfo = getApplicationExitInfoList();
 
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
-        .thenReturn(new TreeSet<>(Collections.singletonList(sessionId)));
+        .thenReturn(new TreeSet<>(Arrays.asList(sessionId, sessionIdPrevious)));
     mockSettingsProvider(true, false);
-    controller.doCloseSessions(mockSettingsProvider);
+    crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(mockSettingsProvider));
+    // cannot use await since it check preconditions if blocking main thread
+    Thread.sleep(100);
     verify(mockSessionReportingCoordinator)
         .persistRelevantAppExitInfoEvent(
-            eq(sessionId),
+            eq(sessionIdPrevious),
             eq(testApplicationExitInfo),
             any(LogFileManager.class),
             any(UserMetadata.class));
   }
 
   @Test
-  public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() {
+  public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() throws Exception {
     final String sessionId = "sessionId";
     final CrashlyticsController controller = createController();
 
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
         .thenReturn(new TreeSet<>(Collections.singletonList(sessionId)));
     mockSettingsProvider(false, false);
-    controller.doCloseSessions(mockSettingsProvider);
+    crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider));
+    // cannot use await since it check preconditions if blocking main thread
+    Thread.sleep(10);
     verify(mockSessionReportingCoordinator, never())
         .persistRelevantAppExitInfoEvent(
             eq(sessionId), any(), any(LogFileManager.class), any(UserMetadata.class));
@@ -177,7 +192,7 @@ private CrashlyticsController createController() {
             MISSING_NATIVE_COMPONENT,
             mock(AnalyticsEventLogger.class),
             mock(CrashlyticsAppQualitySessionsSubscriber.class),
-            new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()));
+            crashlyticsWorkers);
     controller.openSession(SESSION_ID);
     return controller;
   }