diff --git a/firebase-crashlytics/firebase-crashlytics.gradle b/firebase-crashlytics/firebase-crashlytics.gradle index 3c450c0d68a..11656334809 100644 --- a/firebase-crashlytics/firebase-crashlytics.gradle +++ b/firebase-crashlytics/firebase-crashlytics.gradle @@ -98,6 +98,7 @@ dependencies { testImplementation(libs.mockito.core) testImplementation(libs.robolectric) testImplementation(libs.truth) + testImplementation(project(":integ-testing")) androidTestImplementation(libs.androidx.test.core) androidTestImplementation(libs.androidx.test.runner) 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 904ad8bb97b..70c946f08ca 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 @@ -34,8 +34,10 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.NativeSessionFileProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; @@ -55,13 +57,15 @@ import java.util.TreeSet; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; -import org.junit.Test; import org.mockito.ArgumentCaptor; public class CrashlyticsControllerTest extends CrashlyticsTestCase { private static final String GOOGLE_APP_ID = "google:app:id"; private static final String SESSION_ID = "session_id"; + private final CrashlyticsWorker commonWorker = + new CrashlyticsWorker(TestOnlyExecutors.background()); + private Context testContext; private IdManager idManager; private SettingsProvider testSettingsProvider; @@ -99,6 +103,12 @@ protected void setUp() throws Exception { when(testSettingsProvider.getSettingsAsync()).thenReturn(Tasks.forResult(testSettings)); } + @Override + protected void tearDown() throws Exception { + super.tearDown(); + commonWorker.await(); + } + /** A convenience class for building CrashlyticsController instances for testing. */ private class ControllerBuilder { private DataCollectionArbiter dataCollectionArbiter; @@ -106,7 +116,6 @@ private class ControllerBuilder { private AnalyticsEventLogger analyticsEventLogger; private SessionReportingCoordinator sessionReportingCoordinator; - private CrashlyticsBackgroundWorker backgroundWorker; private LogFileManager logFileManager = null; private UserMetadata userMetadata = null; @@ -118,8 +127,6 @@ private class ControllerBuilder { analyticsEventLogger = mock(AnalyticsEventLogger.class); sessionReportingCoordinator = mockSessionReportingCoordinator; - - backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService()); } ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) { @@ -168,7 +175,7 @@ public CrashlyticsController build() { final CrashlyticsController controller = new CrashlyticsController( testContext.getApplicationContext(), - backgroundWorker, + commonWorker, idManager, dataCollectionArbiter, testFileStore, @@ -208,6 +215,8 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal() controller.writeNonFatalException(thread, nonFatal); controller.doCloseSessions(testSettingsProvider); + commonWorker.await(); + verify(mockSessionReportingCoordinator) .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong()); } @@ -228,9 +237,8 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th .persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong()); } - @Test @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testOnDemandFatal_callLogFatalException() { + public void testOnDemandFatal_callLogFatalException() throws Exception { Thread thread = Thread.currentThread(); Exception fatal = new RuntimeException("Fatal"); Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class); @@ -246,6 +254,8 @@ public void testOnDemandFatal_callLogFatalException() { controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider); controller.logFatalException(thread, fatal); + commonWorker.await(); + verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID))); } @@ -323,7 +333,9 @@ public File getOsFile() { final CrashlyticsController controller = builder().setNativeComponent(mockNativeComponent).setLogFileManager(logFileManager).build(); - controller.finalizeSessions(testSettingsProvider); + commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider)); + commonWorker.await(); + verify(mockSessionReportingCoordinator) .finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any()); verify(mockSessionReportingCoordinator, never()) @@ -331,9 +343,10 @@ public File getOsFile() { } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testMissingNativeComponentCausesNoReports() { + public void testMissingNativeComponentCausesNoReports() throws Exception { final CrashlyticsController controller = createController(); - controller.finalizeSessions(testSettingsProvider); + commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider)); + commonWorker.await(); List sessions = testFileStore.getAllOpenSessionIds(); for (String sessionId : sessions) { @@ -384,7 +397,8 @@ public void testFinalizeSessionAfterCrashOk() throws Exception { testSettingsProvider, Thread.currentThread(), new RuntimeException()); // This should not throw. - controller.finalizeSessions(testSettingsProvider); + commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider)); + commonWorker.await(); } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo @@ -535,7 +549,7 @@ public void testUploadDisabledThenEnabled() throws Exception { } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testFatalEvent_sendsAppExceptionEvent() { + public void testFatalEvent_sendsAppExceptionEvent() throws Exception { final String sessionId = "sessionId"; final LogFileManager logFileManager = new LogFileManager(testFileStore); final AnalyticsEventLogger mockFirebaseAnalyticsLogger = mock(AnalyticsEventLogger.class); @@ -548,10 +562,14 @@ public void testFatalEvent_sendsAppExceptionEvent() { when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singleton(sessionId))); - controller.openSession(SESSION_ID); - controller.handleUncaughtException( - testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); - controller.finalizeSessions(testSettingsProvider); + commonWorker.submit( + () -> { + controller.openSession(SESSION_ID); + controller.handleUncaughtException( + testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); + controller.finalizeSessions(testSettingsProvider); + }); + commonWorker.await(); assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger); } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java index 9b579a66a62..e73bad6ba20 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java @@ -27,9 +27,11 @@ import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.UnavailableAnalyticsEventLogger; @@ -44,7 +46,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutorService; public class CrashlyticsCoreInitializationTest extends CrashlyticsTestCase { @@ -89,8 +90,9 @@ private static final class CoreBuilder { private IdManager idManager; private CrashlyticsNativeComponent nativeComponent; private DataCollectionArbiter arbiter; - private ExecutorService crashHandlerExecutor; private FileStore fileStore; + private CrashlyticsWorker commonWorker; + private CrashlyticsWorker diskWriteWorker; public CoreBuilder(Context context, FirebaseOptions firebaseOptions) { app = mock(FirebaseApp.class); @@ -119,7 +121,8 @@ public void whenAvailable( arbiter = mock(DataCollectionArbiter.class); when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(true); - crashHandlerExecutor = new SameThreadExecutorService(); + commonWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); fileStore = new FileStore(context); } @@ -142,9 +145,10 @@ public CrashlyticsCore build() { new DisabledBreadcrumbSource(), new UnavailableAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + commonWorker, + diskWriteWorker); } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java index 17064da9f37..a12bcabceae 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java @@ -28,10 +28,12 @@ import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.BuildConfig; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.UnavailableAnalyticsEventLogger; @@ -427,9 +429,10 @@ CrashlyticsCore build(Context context) { breadcrumbSource, new UnavailableAnalyticsEventLogger(), new FileStore(context), - new SameThreadExecutorService(), mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + new CrashlyticsWorker(TestOnlyExecutors.background()), + new CrashlyticsWorker(TestOnlyExecutors.background())); return crashlyticsCore; } } 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 db3d6d8a1cc..122134b8602 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 @@ -16,8 +16,9 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.io.File; import java.io.IOException; @@ -57,8 +58,8 @@ public class MetaDataStoreTest extends CrashlyticsTestCase { } private FileStore fileStore; - private final CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run); + private CrashlyticsWorker diskWriteWorker; private MetaDataStore storeUnderTest; @Override @@ -66,6 +67,12 @@ public void setUp() throws Exception { super.setUp(); fileStore = new FileStore(getContext()); storeUnderTest = new MetaDataStore(fileStore); + diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + } + + @Override + public void tearDown() throws Exception { + fileStore.deleteAllCrashlyticsFiles(); } private UserMetadata metadataWithUserId(String sessionId) { @@ -73,113 +80,179 @@ private UserMetadata metadataWithUserId(String sessionId) { } private UserMetadata metadataWithUserId(String sessionId, String userId) { - UserMetadata metadata = new UserMetadata(sessionId, fileStore, worker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, diskWriteWorker); metadata.setUserId(userId); return metadata; } - public void testWriteUserData_allFields() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_allFields() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_noFields() { - storeUnderTest.writeUserData( - SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_noFields() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertNull(userData.getUserId()); } - public void testWriteUserData_singleField() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_singleField() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_null() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_null() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertNull(userData.getUserId()); } - public void testWriteUserData_emptyString() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_emptyString() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertEquals("", userData.getUserId()); } - public void testWriteUserData_unicode() { + @Test + public void testWriteUserData_unicode() throws Exception { storeUnderTest.writeUserData( SESSION_ID_1, metadataWithUserId(SESSION_ID_1, UNICODE).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertEquals(UNICODE, userData.getUserId()); } - public void testWriteUserData_escaped() { - storeUnderTest.writeUserData( - SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_escaped() throws Exception { + diskWriteWorker.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); + }); + diskWriteWorker.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertEquals(ESCAPED.trim(), userData.getUserId()); } + @Test public void testWriteUserData_readDifferentSession() { storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, diskWriteWorker); assertNull(userData.getUserId()); } + @Test public void testReadUserData_corruptData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); try (PrintWriter printWriter = new PrintWriter(file)) { printWriter.println("Matt says hi!"); } - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_emptyData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); file.createNewFile(); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_noStoredData() { - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, diskWriteWorker); assertNull(userData.getUserId()); } @Test - public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isFalse(); + diskWriteWorker.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isFalse(); + }); + diskWriteWorker.await(); } @Test - public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() + throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isFalse(); + diskWriteWorker.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isFalse(); + }); + diskWriteWorker.await(); } @Test - public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isFalse(); + + diskWriteWorker.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isFalse(); + }); + diskWriteWorker.await(); } @Test - public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); final Map keys = new HashMap() { { @@ -190,34 +263,74 @@ public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { }; userMetadata.setCustomKeys(keys); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isTrue(); + diskWriteWorker.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isTrue(); + }); + diskWriteWorker.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readKeyData(SESSION_ID_2)).isEqualTo(keys); } @Test - public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() { + public void testSetSameKeysRaceCondition_preserveLastEntryValue() throws Exception { + final Map keys = + new HashMap() { + { + put(KEY_1, "10000"); + put(KEY_2, "20000"); + } + }; + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); + for (int index = 0; index <= 10000; index++) { + userMetadata.setCustomKey(KEY_1, String.valueOf(index)); + userMetadata.setCustomKey(KEY_2, String.valueOf(index * 2)); + } + diskWriteWorker.submit( + () -> { + final Map readKeys = storeUnderTest.readKeyData(SESSION_ID_1); + assertThat(readKeys.get(KEY_1)).isEqualTo("10000"); + assertThat(readKeys.get(KEY_2)).isEqualTo("20000"); + assertEqualMaps(keys, readKeys); + }); + diskWriteWorker.await(); + } + + @Test + public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() throws Exception { String userId = "ThemisWang"; - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); userMetadata.setUserId(userId); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isTrue(); + + diskWriteWorker.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isTrue(); + }); + diskWriteWorker.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readUserId(SESSION_ID_2)).isEqualTo(userId); } @Test - public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, diskWriteWorker); userMetadata.updateRolloutsState(ROLLOUTS_STATE); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isTrue(); + diskWriteWorker.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isTrue(); + }); + diskWriteWorker.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readRolloutsState(SESSION_ID_2)).isEqualTo(ROLLOUTS_STATE); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java index 3d488d924e0..696e61576c2 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java @@ -25,14 +25,16 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.common.CurrentTimeProvider; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; import com.google.firebase.crashlytics.internal.common.DeliveryMechanism; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.InstallIdProvider; import com.google.firebase.crashlytics.internal.common.InstallIdProvider.InstallIds; import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import org.json.JSONObject; @@ -57,7 +59,9 @@ public class DefaultSettingsControllerTest extends CrashlyticsTestCase { private SettingsSpiCall mockSettingsSpiCall; private DataCollectionArbiter mockDataCollectionArbiter; - private Executor networkExecutor = ExecutorUtils.buildSingleThreadExecutorService("network"); + private final CrashlyticsWorker commonWorker = + new CrashlyticsWorker(TestOnlyExecutors.background()); + private final ExecutorService networkExecutor = TestOnlyExecutors.blocking(); public DefaultSettingsControllerTest() {} @@ -121,7 +125,7 @@ public void testCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - await(controller.loadSettingsData(networkExecutor)); + await(controller.loadSettingsData(commonWorker, networkExecutor)); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -153,7 +157,8 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception { mockDataCollectionArbiter, true); - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData( + SettingsCacheBehavior.SKIP_CACHE_LOOKUP, commonWorker, networkExecutor); assertNotNull(controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -177,8 +182,7 @@ public void testExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings); @@ -198,7 +202,7 @@ public void testExpiredCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(commonWorker, networkExecutor); assertEquals(cachedSettings, controller.getSettingsSync()); assertEquals(cachedSettings, await(controller.getSettingsAsync())); @@ -220,8 +224,7 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings cachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); @@ -236,7 +239,8 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { mockSettingsSpiCall, mockDataCollectionArbiter, false); - controller.loadSettingsData(SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, networkExecutor); + controller.loadSettingsData( + SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, commonWorker, networkExecutor); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -256,8 +260,7 @@ public void testSkipCachedSettingsLoad() throws Exception { JSONObject expiredCachedSettingsJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(expiredCachedSettingsJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings expiredCachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(expiredCachedSettingsJson)) @@ -279,7 +282,8 @@ public void testSkipCachedSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData( + SettingsCacheBehavior.SKIP_CACHE_LOOKUP, commonWorker, networkExecutor); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -330,7 +334,8 @@ public void testLastDitchSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData( + SettingsCacheBehavior.SKIP_CACHE_LOOKUP, commonWorker, networkExecutor); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -364,7 +369,7 @@ public void testNoAvailableSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(commonWorker, networkExecutor); assertNotNull(controller.getSettingsSync()); assertFalse(controller.getSettingsAsync().isComplete()); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java index 7131df8c8a4..9db2fb9f0ee 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.*; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CommonUtils; @@ -29,6 +30,7 @@ import java.io.InputStream; import java.net.HttpURLConnection; import java.util.Map; +import java.util.concurrent.Future; public class DefaultSettingsSpiCallTest extends CrashlyticsTestCase { @@ -83,7 +85,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); @@ -128,7 +131,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java index f358cee6ac7..6238324fc95 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java @@ -19,14 +19,12 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import com.google.android.gms.tasks.Continuation; import com.google.android.gms.tasks.Task; import com.google.firebase.FirebaseApp; import com.google.firebase.analytics.connector.AnalyticsConnector; -import com.google.firebase.annotations.concurrent.Background; -import com.google.firebase.annotations.concurrent.Blocking; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; @@ -36,7 +34,6 @@ import com.google.firebase.crashlytics.internal.common.CrashlyticsAppQualitySessionsSubscriber; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.IdManager; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; @@ -46,7 +43,6 @@ import com.google.firebase.remoteconfig.interop.FirebaseRemoteConfigInterop; import com.google.firebase.sessions.api.FirebaseSessionsDependencies; import java.util.List; -import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; /** @@ -70,8 +66,8 @@ public class FirebaseCrashlytics { @NonNull Deferred nativeComponent, @NonNull Deferred analyticsConnector, @NonNull Deferred remoteConfigInteropDeferred, - @Background ExecutorService backgroundExecutorService, - @Blocking ExecutorService blockingExecutorService) { + ExecutorService backgroundExecutorService, + ExecutorService blockingExecutorService) { Context context = app.getApplicationContext(); final String appIdentifier = context.getPackageName(); @@ -93,9 +89,6 @@ public class FirebaseCrashlytics { final AnalyticsDeferredProxy analyticsDeferredProxy = new AnalyticsDeferredProxy(analyticsConnector); - final ExecutorService crashHandlerExecutor = - ExecutorUtils.buildSingleThreadExecutorService("Crashlytics Exception Handler"); - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber = new CrashlyticsAppQualitySessionsSubscriber(arbiter, fileStore); FirebaseSessionsDependencies.register(sessionsSubscriber); @@ -103,6 +96,9 @@ public class FirebaseCrashlytics { RemoteConfigDeferredProxy remoteConfigDeferredProxy = new RemoteConfigDeferredProxy(remoteConfigInteropDeferred); + CrashlyticsWorker commonWorker = new CrashlyticsWorker(backgroundExecutorService); + CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(backgroundExecutorService); + final CrashlyticsCore core = new CrashlyticsCore( app, @@ -112,9 +108,10 @@ public class FirebaseCrashlytics { analyticsDeferredProxy.getDeferredBreadcrumbSource(), analyticsDeferredProxy.getAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, sessionsSubscriber, - remoteConfigDeferredProxy); + remoteConfigDeferredProxy, + commonWorker, + diskWriteWorker); final String googleAppId = app.getOptions().getApplicationId(); final String mappingFileId = CommonUtils.getMappingFileId(context); @@ -150,10 +147,7 @@ public class FirebaseCrashlytics { Logger.getLogger().v("Installer package name is: " + appData.installerPackageName); - final Executor threadPoolExecutor = - ExecutorUtils.buildSequentialExecutor(backgroundExecutorService); - - final SettingsController settingsController = + SettingsController settingsController = SettingsController.create( context, googleAppId, @@ -166,18 +160,8 @@ public class FirebaseCrashlytics { // Kick off actually fetching the settings. settingsController - .loadSettingsData(threadPoolExecutor) - .continueWith( - threadPoolExecutor, - new Continuation() { - @Override - public Object then(@NonNull Task task) throws Exception { - if (!task.isSuccessful()) { - Logger.getLogger().e("Error fetching settings.", task.getException()); - } - return null; - } - }); + .loadSettingsData(commonWorker, blockingExecutorService) + .addOnFailureListener(ex -> Logger.getLogger().e("Error fetching settings.", ex)); final boolean finishCoreInBackground = core.onPreExecute(appData, settingsController); @@ -456,11 +440,11 @@ public boolean didCrashOnPreviousExecution() { * Indicates whether or not automatic data collection is enabled * * @return In order of priority: - *

If {@link #setCrashlyticsCollectionEnabled(boolean)} is called with a value, use it

- * - *

If the firebase_crashlytics_collection_enabled key is in your app’s AndroidManifest.xml, use it

- * - *

Otherwise, use the default {@link FirebaseApp#isDataCollectionDefaultEnabled()} in FirebaseApp

+ *

If {@link #setCrashlyticsCollectionEnabled(boolean)} is called with a value, use it + *

If the firebase_crashlytics_collection_enabled key is in your app’s + * AndroidManifest.xml, use it + *

Otherwise, use the default {@link FirebaseApp#isDataCollectionDefaultEnabled()} in + * FirebaseApp */ public boolean isCrashlyticsCollectionEnabled() { return core.isCrashlyticsCollectionEnabled(); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt index 256dd724339..d142ff1b283 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt @@ -60,10 +60,7 @@ internal object CrashlyticsPreconditions { private fun isBlockingThread() = threadName.contains("Firebase Blocking Thread #") - // TODO(mrober): Remove the Crashlytics thread when fully migrated to Firebase common threads. - private fun isBackgroundThread() = - threadName.contains("Firebase Background Thread #") || - threadName.contains("Crashlytics Exception Handler") + private fun isBackgroundThread() = threadName.contains("Firebase Background Thread #") private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { if (strictLevel.level >= WARN.level && !isCorrectThread()) { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java index 876f018fc95..e48fba16c48 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java @@ -22,6 +22,8 @@ import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; /** * Helper for executing tasks sequentially on the given executor service. @@ -116,13 +118,13 @@ public Task submitTask(Callable> callable) { } /** - * Blocks until all current pending tasks have completed. + * Blocks until all current pending tasks have completed, up to 30 seconds. Useful for testing. * *

This is not a shutdown, this does not stop new tasks from being submitted to the queue. */ @VisibleForTesting - public void await() throws ExecutionException, InterruptedException { - // Submit an empty runnable, and await on it. - Tasks.await(submit(() -> {})); + public void await() throws ExecutionException, InterruptedException, TimeoutException { + // Submit an empty runnable, and await on it for 30 sec so deadlocked tests fail faster. + Tasks.await(submit(() -> {}), 30, TimeUnit.SECONDS); } } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java index 33369524b2c..b29863f66c5 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java @@ -114,7 +114,9 @@ enum Architecture { matcher.put("x86", X86_32); } - /** @Return {@link CommonUtils.Architecture} enum based on @param String */ + /** + * @Return {@link CommonUtils.Architecture} enum based on @param String + */ static Architecture getValue() { String arch = Build.CPU_ABI; @@ -248,7 +250,9 @@ public static boolean getProximitySensorEnabled(Context context) { } } - /** @deprecated This method will now always return false. It should not be used. */ + /** + * @deprecated This method will now always return false. It should not be used. + */ @Deprecated public static boolean isLoggingEnabled(Context context) { return false; @@ -532,7 +536,9 @@ public static void closeQuietly(Closeable closeable) { } } - /** @return if the given permission is granted */ + /** + * @return if the given permission is granted + */ public static boolean checkPermission(Context context, String permission) { final int res = context.checkCallingOrSelfPermission(permission); return (res == PackageManager.PERMISSION_GRANTED); @@ -556,7 +562,9 @@ public static boolean canTryConnection(Context context) { } } - /** @return true if s1.equals(s2), or if both are null. */ + /** + * @return true if s1.equals(s2), or if both are null. + */ public static boolean nullSafeEquals(@Nullable String s1, @Nullable String s2) { // :TODO: replace calls to this method with Objects.equals(...) when minSdkVersion is 19+ if (s1 == null) { 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 321e03f5654..dd10a0e9bc5 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 @@ -30,6 +30,8 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; +import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.NativeSessionFileProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; @@ -82,7 +84,7 @@ class CrashlyticsController { private final CrashlyticsFileMarker crashMarker; private final UserMetadata userMetadata; - private final CrashlyticsBackgroundWorker backgroundWorker; + private final CrashlyticsWorker backgroundWorker; private final IdManager idManager; private final FileStore fileStore; @@ -116,7 +118,7 @@ class CrashlyticsController { CrashlyticsController( Context context, - CrashlyticsBackgroundWorker backgroundWorker, + CrashlyticsWorker commonWorker, IdManager idManager, DataCollectionArbiter dataCollectionArbiter, FileStore fileStore, @@ -129,7 +131,7 @@ class CrashlyticsController { AnalyticsEventLogger analyticsEventLogger, CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) { this.context = context; - this.backgroundWorker = backgroundWorker; + this.backgroundWorker = commonWorker; this.idManager = idManager; this.dataCollectionArbiter = dataCollectionArbiter; this.fileStore = fileStore; @@ -308,6 +310,7 @@ public Task then(@Nullable Void aVoid) throws Exception { /** This function must be called before opening the first session * */ boolean didCrashOnPreviousExecution() { + CrashlyticsPreconditions.checkBackgroundThread(); // To not violate strict mode. if (!crashMarker.isPresent()) { // Before the first session of this execution is opened, the current session ID still refers // to the previous execution's last session, which is what we want. @@ -531,7 +534,7 @@ private String getCurrentSessionId() { * @param settingsProvider */ boolean finalizeSessions(SettingsProvider settingsProvider) { - backgroundWorker.checkRunningOnThread(); + CrashlyticsPreconditions.checkBackgroundThread(); if (isHandlingException()) { Logger.getLogger().w("Skipping session finalization because a crash has already occurred."); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java index 03789c2c3fa..c86595afc87 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java @@ -26,6 +26,8 @@ import com.google.firebase.FirebaseApp; import com.google.firebase.crashlytics.BuildConfig; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; +import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; @@ -39,9 +41,7 @@ import com.google.firebase.crashlytics.internal.stacktrace.RemoveRepeatsStrategy; import com.google.firebase.crashlytics.internal.stacktrace.StackTraceTrimmingStrategy; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -90,14 +90,15 @@ public class CrashlyticsCore { @VisibleForTesting public final BreadcrumbSource breadcrumbSource; private final AnalyticsEventLogger analyticsEventLogger; - private final ExecutorService crashHandlerExecutor; - private final CrashlyticsBackgroundWorker backgroundWorker; private final CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber; private final CrashlyticsNativeComponent nativeComponent; private final RemoteConfigDeferredProxy remoteConfigDeferredProxy; + private final CrashlyticsWorker commonWorker; + private final CrashlyticsWorker diskWriteWorker; + // region Constructors public CrashlyticsCore( @@ -108,9 +109,10 @@ public CrashlyticsCore( BreadcrumbSource breadcrumbSource, AnalyticsEventLogger analyticsEventLogger, FileStore fileStore, - ExecutorService crashHandlerExecutor, CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, - RemoteConfigDeferredProxy remoteConfigDeferredProxy) { + RemoteConfigDeferredProxy remoteConfigDeferredProxy, + CrashlyticsWorker commonWorker, + CrashlyticsWorker diskWriteWorker) { this.app = app; this.dataCollectionArbiter = dataCollectionArbiter; this.context = app.getApplicationContext(); @@ -118,11 +120,11 @@ public CrashlyticsCore( this.nativeComponent = nativeComponent; this.breadcrumbSource = breadcrumbSource; this.analyticsEventLogger = analyticsEventLogger; - this.crashHandlerExecutor = crashHandlerExecutor; this.fileStore = fileStore; - this.backgroundWorker = new CrashlyticsBackgroundWorker(crashHandlerExecutor); this.sessionsSubscriber = sessionsSubscriber; this.remoteConfigDeferredProxy = remoteConfigDeferredProxy; + this.commonWorker = commonWorker; + this.diskWriteWorker = diskWriteWorker; startTime = System.currentTimeMillis(); onDemandCounter = new OnDemandCounter(); @@ -151,7 +153,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) initializationMarker = new CrashlyticsFileMarker(INITIALIZATION_MARKER_FILE_NAME, fileStore); final UserMetadata userMetadata = - new UserMetadata(sessionIdentifier, fileStore, backgroundWorker); + new UserMetadata(sessionIdentifier, fileStore, commonWorker); final LogFileManager logFileManager = new LogFileManager(fileStore); final StackTraceTrimmingStrategy stackTraceTrimmingStrategy = new MiddleOutFallbackStrategy( @@ -175,7 +177,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) controller = new CrashlyticsController( context, - backgroundWorker, + commonWorker, idManager, dataCollectionArbiter, fileStore, @@ -220,22 +222,16 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) return true; } - /** Performs background initialization asynchronously on the background worker's thread. */ + /** Performs background initialization asynchronously on the common worker. */ @CanIgnoreReturnValue public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { - return Utils.callTask( - crashHandlerExecutor, - new Callable>() { - @Override - public Task call() throws Exception { - return doBackgroundInitialization(settingsProvider); - } - }); + return commonWorker.submitTask(() -> doBackgroundInitialization(settingsProvider)); } /** Performs background initialization synchronously on the calling thread. */ @CanIgnoreReturnValue private Task doBackgroundInitialization(SettingsProvider settingsProvider) { + CrashlyticsPreconditions.checkBackgroundThread(); // create the marker for this run markInitializationStarted(); @@ -424,7 +420,7 @@ CrashlyticsController getController() { /** * When a startup crash occurs, Crashlytics must lock on the main thread and complete - * initializaiton to upload crash result. 4 seconds is chosen for the lock to prevent ANR + * initialization to upload crash result. 4 seconds is chosen for the lock to prevent ANR */ private void finishInitSynchronously(SettingsProvider settingsProvider) { @@ -436,7 +432,8 @@ public void run() { } }; - final Future future = crashHandlerExecutor.submit(runnable); + // TODO(mrober): Refactor to Tasks. Maybe just re-use async task and awaitEvenIfOnMain? + final Future future = commonWorker.getExecutor().submit(runnable); Logger.getLogger() .d( @@ -456,7 +453,7 @@ public void run() { /** Synchronous call to mark start of initialization */ void markInitializationStarted() { - backgroundWorker.checkRunningOnThread(); + CrashlyticsPreconditions.checkBackgroundThread(); // Create the Crashlytics initialization marker file, which is used to determine // whether the app crashed before initialization could complete. @@ -466,21 +463,18 @@ void markInitializationStarted() { /** Enqueues a job to remove the Crashlytics initialization marker file */ void markInitializationComplete() { - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - try { - final boolean removed = initializationMarker.remove(); - if (!removed) { - Logger.getLogger().w("Initialization marker file was not properly removed."); - } - return removed; - } catch (Exception e) { - Logger.getLogger() - .e("Problem encountered deleting Crashlytics initialization marker.", e); - return false; + commonWorker.submit( + () -> { + try { + boolean removed = initializationMarker.remove(); + if (!removed) { + Logger.getLogger().w("Initialization marker file was not properly removed."); } + return removed; + } catch (Exception e) { + Logger.getLogger() + .e("Problem encountered deleting Crashlytics initialization marker.", e); + return false; } }); } @@ -492,14 +486,7 @@ boolean didPreviousInitializationFail() { // region Previous crash handling private void checkForPreviousCrash() { - Task task = - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - return controller.didCrashOnPreviousExecution(); - } - }); + Task task = commonWorker.submit(() -> controller.didCrashOnPreviousExecution()); Boolean result; try { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java index 8e8f19d056a..79d43a9017c 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java @@ -20,7 +20,6 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.errorprone.annotations.CanIgnoreReturnValue; -import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; @@ -77,31 +76,6 @@ public static Task race(Executor executor, Task t1, Task t2) { return result.getTask(); } - /** Similar to Tasks.call, but takes a Callable that returns a Task. */ - public static Task callTask(Executor executor, Callable> callable) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - executor.execute( - () -> { - try { - callable - .call() - .continueWith( - executor, - task -> { - if (task.isSuccessful()) { - result.setResult(task.getResult()); - } else if (task.getException() != null) { - result.setException(task.getException()); - } - return null; - }); - } catch (Exception e) { - result.setException(e); - } - }); - return result.getTask(); - } - /** * Blocks until the given Task completes, and then returns the value the Task was resolved with, * if successful. If the Task fails, an exception will be thrown, wrapping the Exception of the diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java index e6af8a9bea3..1590d113d23 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java @@ -17,8 +17,8 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.common.CommonUtils; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.List; @@ -47,7 +47,7 @@ public class UserMetadata { @VisibleForTesting public static final int MAX_ROLLOUT_ASSIGNMENTS = 128; private final MetaDataStore metaDataStore; - private final CrashlyticsBackgroundWorker backgroundWorker; + private final CrashlyticsWorker backgroundWorker; private String sessionIdentifier; // The following references contain a marker bit, which is true if the data maintained in the @@ -69,9 +69,9 @@ public static String readUserId(String sessionId, FileStore fileStore) { } public static UserMetadata loadFromExistingSession( - String sessionId, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionId, FileStore fileStore, CrashlyticsWorker commonWorker) { MetaDataStore store = new MetaDataStore(fileStore); - UserMetadata metadata = new UserMetadata(sessionId, fileStore, backgroundWorker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, commonWorker); // We don't use the set methods in this class, because they will attempt to re-serialize the // data, which is unnecessary because we just read them from disk. metadata.customKeys.map.getReference().setKeys(store.readKeyData(sessionId, false)); @@ -82,10 +82,10 @@ public static UserMetadata loadFromExistingSession( } public UserMetadata( - String sessionIdentifier, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionIdentifier, FileStore fileStore, CrashlyticsWorker commonWorker) { this.sessionIdentifier = sessionIdentifier; this.metaDataStore = new MetaDataStore(fileStore); - this.backgroundWorker = backgroundWorker; + this.backgroundWorker = commonWorker; } /** diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java index 205e10fb4a5..ed9a423b4ce 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java @@ -14,6 +14,7 @@ package com.google.firebase.crashlytics.internal.network; +import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; import com.google.firebase.crashlytics.internal.Logger; import java.io.BufferedReader; import java.io.IOException; @@ -56,6 +57,7 @@ public HttpGetRequest header(Map.Entry entry) { } public HttpResponse execute() throws IOException { + CrashlyticsPreconditions.checkBlockingThread(); // Network calls must be on a blocking thread. InputStream stream = null; HttpsURLConnection connection = null; String body = null; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java index dcfbdf8d0fd..e54cadfc651 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java @@ -15,6 +15,7 @@ package com.google.firebase.crashlytics.internal.settings; import android.text.TextUtils; +import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; import com.google.firebase.crashlytics.internal.network.HttpGetRequest; @@ -96,6 +97,7 @@ protected HttpGetRequest createHttpGetRequest(Map queryParams) { @Override public JSONObject invoke(SettingsRequest requestData, boolean dataCollectionToken) { + CrashlyticsPreconditions.checkBlockingThread(); if (!dataCollectionToken) { throw new RuntimeException("An invalid data collection token was used."); } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java index d44c37257e3..e86f6e4d55e 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java @@ -23,6 +23,7 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CommonUtils; import com.google.firebase.crashlytics.internal.common.CurrentTimeProvider; @@ -33,7 +34,8 @@ import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.Locale; -import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; import org.json.JSONException; import org.json.JSONObject; @@ -149,8 +151,9 @@ public Settings getSettingsSync() { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(Executor executor) { - return loadSettingsData(SettingsCacheBehavior.USE_CACHE, executor); + public Task loadSettingsData( + CrashlyticsWorker commonWorker, ExecutorService networkExecutor) { + return loadSettingsData(SettingsCacheBehavior.USE_CACHE, commonWorker, networkExecutor); } /** @@ -158,7 +161,10 @@ public Task loadSettingsData(Executor executor) { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor executor) { + public Task loadSettingsData( + SettingsCacheBehavior cacheBehavior, + CrashlyticsWorker commonWorker, + ExecutorService networkExecutor) { // TODO: Refactor this so that it doesn't do the cache lookup twice when settings are // expired. @@ -186,18 +192,22 @@ public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor } // Kick off fetching fresh settings. + // TODO(mrober): Refactor to call worker directly, not expose executor. return dataCollectionArbiter - .waitForDataCollectionPermission(executor) + .waitForDataCollectionPermission(commonWorker.getExecutor()) .onSuccessTask( - executor, + commonWorker.getExecutor(), new SuccessContinuation() { @NonNull @Override public Task then(@Nullable Void aVoid) throws Exception { // Waited for data collection permission, so this is safe. final boolean dataCollectionToken = true; - final JSONObject settingsJson = - settingsSpiCall.invoke(settingsRequest, dataCollectionToken); + Future settingsFuture = + networkExecutor.submit( + () -> settingsSpiCall.invoke(settingsRequest, dataCollectionToken)); + // TODO(mrober): Should we add a timeout here, or let the entire init timeout? + JSONObject settingsJson = settingsFuture.get(); if (settingsJson != null) { final Settings fetchedSettings = 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 24ce37dd4b7..68c0a379529 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 @@ -28,8 +28,10 @@ import android.content.Context; import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; @@ -164,7 +166,7 @@ private CrashlyticsController createController() { final CrashlyticsController controller = new CrashlyticsController( testContext, - new CrashlyticsBackgroundWorker(Runnable::run), + new CrashlyticsWorker(TestOnlyExecutors.background()), idManager, mockDataCollectionArbiter, testFileStore,