From 39199d0f437314dda83b8a9dd40e219e4362962d Mon Sep 17 00:00:00 2001 From: Themis wang Date: Mon, 5 Aug 2024 19:59:16 -0700 Subject: [PATCH 1/4] move more jobs to disk write --- .../common/CrashlyticsController.java | 25 +++++++++++-------- .../common/SessionReportingCoordinator.java | 16 +++++++----- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index dfbd2e9b628..c3dee1b3801 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -205,7 +205,6 @@ public Task 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 then(@Nullable Boolean send) throws Exception { public Task call() throws Exception { if (!send) { Logger.getLogger().v("Deleting cached crash reports..."); - deleteFiles(listAppExceptionMarkerFiles()); - reportingCoordinator.removeAllReports(); + diskWriteWorker.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; @@ -558,14 +560,15 @@ private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) { } 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) { final int offset = skipCurrentSession ? 1 : 0; // :TODO HW2021 this implementation can be cleaned up. @@ -579,13 +582,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); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java index 5e35c87ed67..00e7af33e63 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java @@ -354,12 +354,16 @@ private boolean onReportSendComplete(@NonNull Task { + 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() From 7036db5b1bd4966b8a68483df0abf3722f074506 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 6 Aug 2024 12:32:09 -0700 Subject: [PATCH 2/4] add precondition to reinforce the executrion on background thread --- .../crashlytics/internal/common/CrashlyticsController.java | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index c3dee1b3801..8247406ba3a 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -569,6 +569,7 @@ void doCloseSessions(SettingsProvider settingsProvider) { */ private void doCloseSessions( boolean skipCurrentSession, SettingsProvider settingsProvider, boolean isInitProcess) { + CrashlyticsPreconditions.checkBackgroundThread(); final int offset = skipCurrentSession ? 1 : 0; // :TODO HW2021 this implementation can be cleaned up. From 23bcd866ea42f3344e15711b30257e85f7830447 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 6 Aug 2024 16:38:00 -0700 Subject: [PATCH 3/4] fix unit tests --- .../internal/common/CrashlyticsController.java | 2 ++ .../common/CrashlyticsControllerRobolectricTest.java | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index 8247406ba3a..c18441353aa 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -559,6 +559,8 @@ 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, false); } diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java index 026d5ce2280..1068deb81f6 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java +++ b/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; @@ -100,6 +101,7 @@ public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntE @Test public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { + final String sessionIdPrevious = "sessionIdPrevious"; final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); // Adds multiple AppExitInfos to confirm that Crashlytics loops through @@ -109,12 +111,12 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { List testApplicationExitInfo = getApplicationExitInfoList(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) - .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); + .thenReturn(new TreeSet<>(Arrays.asList(sessionId, sessionIdPrevious))); mockSettingsProvider(true, false); - controller.doCloseSessions(mockSettingsProvider); + controller.finalizeSessions(mockSettingsProvider); verify(mockSessionReportingCoordinator) .persistRelevantAppExitInfoEvent( - eq(sessionId), + eq(sessionIdPrevious), eq(testApplicationExitInfo), any(LogFileManager.class), any(UserMetadata.class)); From 54275a241db80b361c61f668c3ef8fcfecf9cd59 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 13 Aug 2024 15:01:41 -0400 Subject: [PATCH 4/4] fix merge conflict --- .../common/CrashlyticsControllerTest.java | 5 +++- .../internal/metadata/MetaDataStoreTest.java | 1 + .../common/CrashlyticsController.java | 4 +-- .../common/SessionReportingCoordinator.java | 2 +- .../CrashlyticsControllerRobolectricTest.java | 27 ++++++++++++++----- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java index 7eaacded569..34a1d72a47d 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java +++ b/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); } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java index ff7f853adff..4a1d399c137 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java +++ b/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()); } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index c18441353aa..5d60f93a9c8 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -362,7 +362,7 @@ public Task then(@Nullable Boolean send) throws Exception { public Task call() throws Exception { if (!send) { Logger.getLogger().v("Deleting cached crash reports..."); - diskWriteWorker.submit( + crashlyticsWorkers.diskWrite.submit( () -> { deleteFiles(listAppExceptionMarkerFiles()); reportingCoordinator.removeAllReports(); @@ -571,7 +571,7 @@ void doCloseSessions(SettingsProvider settingsProvider) { */ private void doCloseSessions( boolean skipCurrentSession, SettingsProvider settingsProvider, boolean isInitProcess) { - CrashlyticsPreconditions.checkBackgroundThread(); + CrashlyticsWorkers.checkBackgroundThread(); final int offset = skipCurrentSession ? 1 : 0; // :TODO HW2021 this implementation can be cleaned up. diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java index 00e7af33e63..56ff1244439 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java @@ -354,7 +354,7 @@ private boolean onReportSendComplete(@NonNull Task { File reportFile = report.getReportFile(); if (reportFile.delete()) { diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java index 1068deb81f6..ed21115c075 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java +++ b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java @@ -66,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() { @@ -81,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()) @@ -100,7 +109,7 @@ 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(); @@ -113,7 +122,9 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Arrays.asList(sessionId, sessionIdPrevious))); mockSettingsProvider(true, false); - controller.finalizeSessions(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(sessionIdPrevious), @@ -123,14 +134,16 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { } @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)); @@ -179,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; }