diff --git a/firebase-crashlytics-ndk/gradle.properties b/firebase-crashlytics-ndk/gradle.properties index 212c6add578..5fc86eb4491 100644 --- a/firebase-crashlytics-ndk/gradle.properties +++ b/firebase-crashlytics-ndk/gradle.properties @@ -1,2 +1,2 @@ -version=19.0.4 +version=19.1.0 latestReleasedVersion=19.0.3 diff --git a/firebase-crashlytics/CHANGELOG.md b/firebase-crashlytics/CHANGELOG.md index 937ec9a3752..39cc4022d1c 100644 --- a/firebase-crashlytics/CHANGELOG.md +++ b/firebase-crashlytics/CHANGELOG.md @@ -1,8 +1,14 @@ # Unreleased -* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is enabled. - ([Github #5919](//github.com/firebase/firebase-android-sdk/issues/5919)) +* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is + enabled. + (GitHub [#5919](https://github.com/firebase/firebase-android-sdk/issues/5919){: .external}) * [fixed] Ensure that on-demand fatal events are never processed on the main thread. + (GitHub [#4345](https://github.com/firebase/firebase-android-sdk/issues/4345){: .external}) +* [fixed] Improved data consistency for rapid user actions. +* [changed] Internal changes to improve startup time. * [changed] Internal changes to the way session IDs are generated. +* [changed] Internal changes to the way background tasks are scheduled. +* [changed] Migrated SDK to use standard Firebase executors. # 19.0.3 diff --git a/firebase-crashlytics/gradle.properties b/firebase-crashlytics/gradle.properties index 212c6add578..5fc86eb4491 100644 --- a/firebase-crashlytics/gradle.properties +++ b/firebase-crashlytics/gradle.properties @@ -1,2 +1,2 @@ -version=19.0.4 +version=19.1.0 latestReleasedVersion=19.0.3 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 34a1d72a47d..fc46a35090c 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 @@ -410,9 +410,9 @@ public void testUploadWithNoReports() throws Exception { final CrashlyticsController controller = createController(); - Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verifyNoMoreInteractions(mockSessionReportingCoordinator); @@ -426,9 +426,9 @@ public void testUploadWithDataCollectionAlwaysEnabled() throws Exception { final CrashlyticsController controller = createController(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockDataCollectionArbiter).isAutomaticDataCollectionEnabled(); @@ -452,15 +452,13 @@ public void testUploadDisabledThenOptIn() throws Exception { final ControllerBuilder builder = builder(); builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(arbiter).isAutomaticDataCollectionEnabled(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class)); await(controller.sendUnsentReports()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).sendReports(any(Executor.class)); verifyNoMoreInteractions(mockSessionReportingCoordinator); @@ -481,14 +479,14 @@ public void testUploadDisabledThenOptOut() throws Exception { builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(arbiter).isAutomaticDataCollectionEnabled(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).removeAllReports(); await(controller.deleteUnsentReports()); - await(task); + crashlyticsWorkers.common.await(); crashlyticsWorkers.diskWrite.await(); @@ -525,7 +523,7 @@ public void testUploadDisabledThenEnabled() throws Exception { builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class)); @@ -547,7 +545,7 @@ public void testUploadDisabledThenEnabled() throws Exception { when(app.isDataCollectionDefaultEnabled()).thenReturn(false); assertFalse(arbiter.isAutomaticDataCollectionEnabled()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).sendReports(any(Executor.class)); verifyNoMoreInteractions(mockSessionReportingCoordinator); 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 728757d30e8..33046b3f74c 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 @@ -22,8 +22,6 @@ import android.content.Context; import android.text.TextUtils; import androidx.annotation.NonNull; -import androidx.annotation.Nullable; -import com.google.android.gms.tasks.SuccessContinuation; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; @@ -379,14 +377,7 @@ private Task startCoreAsync(CrashlyticsCore crashlyticsCore) { return crashlyticsCore .doBackgroundInitializationAsync(mockSettingsController) - .onSuccessTask( - new SuccessContinuation() { - @NonNull - @Override - public Task then(@Nullable Void aVoid) throws Exception { - return Tasks.forResult(crashlyticsCore); - } - }); + .onSuccessTask(unused -> Tasks.forResult(crashlyticsCore)); } /** Helper class for building CrashlyticsCore instances. */ diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java index 80904908632..56eae6b3f42 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java @@ -37,8 +37,11 @@ import android.os.BatteryManager; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.Event.Application.Execution; import com.google.firebase.crashlytics.internal.settings.Settings; @@ -66,6 +69,8 @@ public class CrashlyticsReportDataCaptureTest { private int eventThreadImportance; private int maxChainedExceptions; private SettingsProvider testSettingsProvider; + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Mock private DevelopmentPlatformProvider developmentPlatformProvider; @@ -118,7 +123,9 @@ public void testCaptureReport_containsUnityVersionInDeveloperPlatformFieldsWhenA .thenReturn(expectedUnityVersion); initDataCapture(); - final CrashlyticsReport report = dataCapture.captureReportData("sessionId", 0); + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData("sessionId", 0)); + CrashlyticsReport report = Tasks.await(task); assertNotNull(report.getSession()); assertEquals(UNITY_PLATFORM, report.getSession().getApp().getDevelopmentPlatform()); @@ -132,7 +139,9 @@ public void testCaptureReport_containsNoDeveloperPlatformFieldsWhenUnityIsMissin when(developmentPlatformProvider.getDevelopmentPlatform()).thenReturn(null); initDataCapture(); - final CrashlyticsReport report = dataCapture.captureReportData("sessionId", 0); + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData("sessionId", 0)); + CrashlyticsReport report = Tasks.await(task); assertNotNull(report.getSession()); assertNull(report.getSession().getApp().getDevelopmentPlatform()); @@ -307,10 +316,13 @@ public void testCaptureAnrEvent_noBuildIdInAppData_buildIdsNull() throws Excepti } @Test - public void testCaptureReportSessionFields() { + public void testCaptureReportSessionFields() throws Exception { final String sessionId = "sessionId"; final long timestamp = System.currentTimeMillis(); - final CrashlyticsReport report = dataCapture.captureReportData(sessionId, timestamp); + + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData(sessionId, timestamp)); + CrashlyticsReport report = Tasks.await(task); assertEquals(sessionId, report.getSession().getIdentifier()); assertEquals(timestamp, report.getSession().getStartedAt()); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java index 93a3e334fe3..d51a07fbf7f 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java @@ -21,14 +21,20 @@ import android.content.SharedPreferences; 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.common.InstallIdProvider.InstallIds; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import java.util.concurrent.TimeoutException; public class IdManagerTest extends CrashlyticsTestCase { - SharedPreferences prefs; - SharedPreferences legacyPrefs; + private SharedPreferences prefs; + private SharedPreferences legacyPrefs; + + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Override public void setUp() throws Exception { @@ -66,10 +72,10 @@ private IdManager createIdManager(String instanceId, DataCollectionArbiter arbit return new IdManager(getContext(), getContext().getPackageName(), iid, arbiter); } - public void testCreateUUID() { + public void testCreateUUID() throws Exception { final String fid = "test_fid"; final IdManager idManager = createIdManager(fid, MOCK_ARBITER_ENABLED); - final String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + final String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); assertEquals(installId, prefs.getString(IdManager.PREFKEY_INSTALLATION_UUID, null)); @@ -77,10 +83,10 @@ public void testCreateUUID() { assertEquals(fid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(installId, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(installId, getInstallIds(idManager).getCrashlyticsInstallId()); } - public void testGetIdExceptionalCase_doesNotRotateInstallId() { + public void testGetIdExceptionalCase_doesNotRotateInstallId() throws Exception { FirebaseInstallationsApi fis = mock(FirebaseInstallationsApi.class); final String expectedInstallId = "expectedInstallId"; when(fis.getId()) @@ -93,12 +99,12 @@ public void testGetIdExceptionalCase_doesNotRotateInstallId() { final IdManager idManager = new IdManager(getContext(), getContext().getPackageName(), fis, MOCK_ARBITER_ENABLED); - final String actualInstallId = idManager.getInstallIds().getCrashlyticsInstallId(); + final String actualInstallId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(actualInstallId); assertEquals(expectedInstallId, actualInstallId); } - public void testInstanceIdChanges_dataCollectionEnabled() { + public void testInstanceIdChanges_dataCollectionEnabled() throws Exception { // Set up the initial state with a valid iid and uuid. final String oldUuid = "old_uuid"; final String newFid = "new_test_fid"; @@ -111,7 +117,7 @@ public void testInstanceIdChanges_dataCollectionEnabled() { // Initialize the manager with a different FID. IdManager idManager = createIdManager(newFid, MOCK_ARBITER_ENABLED); - String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); assertFalse(installId.equals(oldUuid)); @@ -119,10 +125,10 @@ public void testInstanceIdChanges_dataCollectionEnabled() { assertEquals(newFid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(installId, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(installId, getInstallIds(idManager).getCrashlyticsInstallId()); } - void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { + void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) throws Exception { final String oldUuid = "test_uuid"; final String fid = dataCollectionEnabled ? "test_fid" : IdManager.createSyntheticFid(); // Set up the initial state with a valid iid and uuid. @@ -136,7 +142,7 @@ void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { IdManager idManager = createIdManager(fid, dataCollectionEnabled ? MOCK_ARBITER_ENABLED : MOCK_ARBITER_DISABLED); - String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); // Test that the UUID didn't change. @@ -146,18 +152,18 @@ void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { assertEquals(fid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(oldUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(oldUuid, getInstallIds(idManager).getCrashlyticsInstallId()); } - public void testInstanceIdDoesntChange_dataCollectionEnabled() { + public void testInstanceIdDoesntChange_dataCollectionEnabled() throws Exception { validateInstanceIdDoesntChange(/* dataCollectionEnabled= */ true); } - public void testInstanceIdDoesntChange_dataCollectionDisabled() { + public void testInstanceIdDoesntChange_dataCollectionDisabled() throws Exception { validateInstanceIdDoesntChange(/* dataCollectionEnabled= */ false); } - public void testInstanceIdRotatesWithDataCollectionFlag() { + public void testInstanceIdRotatesWithDataCollectionFlag() throws Exception { final String originalUuid = "test_uuid"; final String originalFid = "test_fid"; // Set up the initial state with a valid iid and uuid. @@ -169,26 +175,25 @@ public void testInstanceIdRotatesWithDataCollectionFlag() { // Initialize the manager with the same FID. IdManager idManager = createIdManager(originalFid, MOCK_ARBITER_ENABLED); - String firstUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String firstUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(firstUuid); assertEquals(originalUuid, firstUuid); // subsequent calls should return the same id - assertEquals(firstUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(firstUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( firstUuid, - createIdManager(originalFid, MOCK_ARBITER_ENABLED) - .getInstallIds() + getInstallIds(createIdManager(originalFid, MOCK_ARBITER_ENABLED)) .getCrashlyticsInstallId()); // Disable data collection manager and confirm we get a different id idManager = createIdManager(originalFid, MOCK_ARBITER_DISABLED); - String secondUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String secondUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotSame(secondUuid, firstUuid); - assertEquals(secondUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(secondUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( secondUuid, - createIdManager(null, MOCK_ARBITER_DISABLED).getInstallIds().getCrashlyticsInstallId()); + getInstallIds(createIdManager(null, MOCK_ARBITER_DISABLED)).getCrashlyticsInstallId()); // Check that we cached an synthetic FID final SharedPreferences prefs = CommonUtils.getSharedPrefs(getContext()); String cachedFid = prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null); @@ -196,17 +201,21 @@ public void testInstanceIdRotatesWithDataCollectionFlag() { // re-enable data collection idManager = createIdManager(originalFid, MOCK_ARBITER_ENABLED); - String thirdUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String thirdUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotSame(thirdUuid, firstUuid); assertNotSame(thirdUuid, secondUuid); - assertEquals(thirdUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(thirdUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( thirdUuid, - createIdManager(originalFid, MOCK_ARBITER_ENABLED) - .getInstallIds() + getInstallIds(createIdManager(originalFid, MOCK_ARBITER_ENABLED)) .getCrashlyticsInstallId()); // The cached ID should be back to the original cachedFid = prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null); assertEquals(cachedFid, originalFid); } + + /** Get the install ids on the common worker. */ + private InstallIds getInstallIds(IdManager idManager) throws Exception { + return Tasks.await(crashlyticsWorkers.common.submit(idManager::getInstallIds)); + } } 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 4a1d399c137..f66c3e0d2d6 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 @@ -145,6 +145,7 @@ public void testWriteUserData_emptyString() throws Exception { SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); }); crashlyticsWorkers.diskWrite.await(); + Thread.sleep(3); UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals("", 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 5d60f93a9c8..7726d9e6924 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 @@ -248,6 +248,7 @@ public Task then(@Nullable Settings settings) throws Exception { try { // Never block on ODFs, in case they get triggered from the main thread. if (!isOnDemand) { + //noinspection deprecation drain the worker instead. Utils.awaitEvenIfOnMainThread(handleUncaughtExceptionTask); } } catch (TimeoutException e) { @@ -338,68 +339,56 @@ Task deleteUnsentReports() { return unsentReportsHandled.getTask(); } - // TODO(b/261014167): Use an explicit executor in continuations. - @SuppressLint("TaskMainThread") - Task submitAllReports(Task settingsDataTask) { + void submitAllReports(Task settingsDataTask) { if (!reportingCoordinator.hasReportsToSend()) { // Just notify the user that there are no reports and stop. Logger.getLogger().v("No crash reports are available to be sent."); unsentReportsAvailable.trySetResult(false); - return Tasks.forResult(null); + return; } Logger.getLogger().v("Crash reports are available to be sent."); - return waitForReportAction() + waitForReportAction() .onSuccessTask( + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override public Task then(@Nullable Boolean send) throws Exception { + if (!send) { + Logger.getLogger().v("Deleting cached crash reports..."); + deleteFiles(listAppExceptionMarkerFiles()); + reportingCoordinator.removeAllReports(); + unsentReportsHandled.trySetResult(null); + + return Tasks.forResult(null); + } + + Logger.getLogger().d("Sending cached crash reports..."); - return crashlyticsWorkers.common.submitTask( - new Callable>() { + // waitForReportAction guarantees we got user permission. + boolean dataCollectionToken = send; + + // Signal to the settings fetch and onboarding that we have explicit + // permission. + dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); + return settingsDataTask.onSuccessTask( + crashlyticsWorkers.common, + new SuccessContinuation() { + @NonNull @Override - public Task call() throws Exception { - if (!send) { - Logger.getLogger().v("Deleting cached crash reports..."); - crashlyticsWorkers.diskWrite.submit( - () -> { - deleteFiles(listAppExceptionMarkerFiles()); - reportingCoordinator.removeAllReports(); - }); - unsentReportsHandled.trySetResult(null); + public Task then(@Nullable Settings appSettingsData) throws Exception { + if (appSettingsData == null) { + Logger.getLogger() + .w( + "Received null app settings at app startup. Cannot send cached reports"); return Tasks.forResult(null); } + logAnalyticsAppExceptionEvents(); + reportingCoordinator.sendReports(crashlyticsWorkers.common); + unsentReportsHandled.trySetResult(null); - Logger.getLogger().d("Sending cached crash reports..."); - - // waitForReportAction guarantees we got user permission. - boolean dataCollectionToken = send; - - // Signal to the settings fetch and onboarding that we have explicit - // permission. - dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); - - return settingsDataTask.onSuccessTask( - crashlyticsWorkers.common, - new SuccessContinuation() { - @NonNull - @Override - public Task then(@Nullable Settings appSettingsData) - throws Exception { - if (appSettingsData == null) { - Logger.getLogger() - .w( - "Received null app settings at app startup. Cannot send cached reports"); - return Tasks.forResult(null); - } - logAnalyticsAppExceptionEvents(); - reportingCoordinator.sendReports(crashlyticsWorkers.common); - unsentReportsHandled.trySetResult(null); - - return Tasks.forResult(null); - } - }); + return Tasks.forResult(null); } }); } @@ -566,7 +555,6 @@ void doCloseSessions(SettingsProvider settingsProvider) { } /** - * * Not synchronized/locked. Must be executed from the executor service runs tasks in serial order */ private void doCloseSessions( 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 3214e525390..8e451762642 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 @@ -21,7 +21,6 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.firebase.FirebaseApp; import com.google.firebase.crashlytics.BuildConfig; @@ -225,12 +224,11 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) /** Performs background initialization asynchronously on the common worker. */ @CanIgnoreReturnValue public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { - return crashlyticsWorkers.common.submitTask(() -> doBackgroundInitialization(settingsProvider)); + return crashlyticsWorkers.common.submit(() -> doBackgroundInitialization(settingsProvider)); } /** Performs background initialization synchronously on the calling thread. */ - @CanIgnoreReturnValue - private Task doBackgroundInitialization(SettingsProvider settingsProvider) { + private void doBackgroundInitialization(SettingsProvider settingsProvider) { CrashlyticsWorkers.checkBackgroundThread(); // create the marker for this run markInitializationStarted(); @@ -246,22 +244,17 @@ private Task doBackgroundInitialization(SettingsProvider settingsProvider) Logger.getLogger().d("Collection of crash reports disabled in Crashlytics settings."); // TODO: This isn't actually an error condition, so figure out the right way to // handle this case. - return Tasks.forException( - new RuntimeException("Collection of crash reports disabled in Crashlytics settings.")); + throw new RuntimeException("Collection of crash reports disabled in Crashlytics settings."); } if (!controller.finalizeSessions(settingsProvider)) { Logger.getLogger().w("Previous sessions could not be finalized."); } - // TODO: Move this call out of this method, so that the return value merely indicates - // initialization is complete. Callers that want to know when report sending is complete can - // handle that as a separate call. - return controller.submitAllReports(settingsProvider.getSettingsAsync()); + controller.submitAllReports(settingsProvider.getSettingsAsync()); } catch (Exception e) { Logger.getLogger() .e("Crashlytics encountered a problem during asynchronous initialization.", e); - return Tasks.forException(e); } finally { // The only thing that compels us to leave the marker and start synchronously next time // is not executing all the way through this method. That would indicate that we perhaps @@ -433,17 +426,11 @@ CrashlyticsController getController() { * initialization to upload crash result. 4 seconds is chosen for the lock to prevent ANR */ private void finishInitSynchronously(SettingsProvider settingsProvider) { - - final Runnable runnable = - new Runnable() { - @Override - public void run() { - doBackgroundInitialization(settingsProvider); - } - }; - - // TODO(mrober): Refactor to Tasks. Maybe just re-use async task and awaitEvenIfOnMain? - final Future future = crashlyticsWorkers.common.getExecutor().submit(runnable); + Future future = + crashlyticsWorkers + .common + .getExecutor() + .submit(() -> doBackgroundInitialization(settingsProvider)); Logger.getLogger() .d( @@ -452,12 +439,13 @@ public void run() { try { future.get(DEFAULT_MAIN_HANDLER_TIMEOUT_SEC, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Logger.getLogger().e("Crashlytics was interrupted during initialization.", e); - } catch (ExecutionException e) { - Logger.getLogger().e("Crashlytics encountered a problem during initialization.", e); - } catch (TimeoutException e) { - Logger.getLogger().e("Crashlytics timed out during initialization.", e); + } catch (InterruptedException ex) { + Logger.getLogger().e("Crashlytics was interrupted during initialization.", ex); + Thread.currentThread().interrupt(); + } catch (ExecutionException ex) { + Logger.getLogger().e("Crashlytics encountered a problem during initialization.", ex); + } catch (TimeoutException ex) { + Logger.getLogger().e("Crashlytics timed out during initialization.", ex); } } @@ -473,20 +461,15 @@ void markInitializationStarted() { /** Enqueues a job to remove the Crashlytics initialization marker file */ void markInitializationComplete() { - crashlyticsWorkers.common.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; - } - }); + CrashlyticsWorkers.checkBackgroundThread(); + try { + boolean removed = initializationMarker.remove(); + if (!removed) { + Logger.getLogger().w("Initialization marker file was not properly removed."); + } + } catch (Exception ex) { + Logger.getLogger().e("Problem encountered deleting Crashlytics initialization marker.", ex); + } } boolean didPreviousInitializationFail() { @@ -496,13 +479,16 @@ boolean didPreviousInitializationFail() { // region Previous crash handling private void checkForPreviousCrash() { - Task task = - crashlyticsWorkers.common.submit(() -> controller.didCrashOnPreviousExecution()); + Future future = + crashlyticsWorkers + .common + .getExecutor() + .submit(() -> controller.didCrashOnPreviousExecution()); Boolean result; try { - result = Utils.awaitEvenIfOnMainThread(task); - } catch (Exception e) { + result = future.get(DEFAULT_MAIN_HANDLER_TIMEOUT_SEC, TimeUnit.SECONDS); + } catch (Exception ignored) { didCrashOnPreviousExecution = false; return; } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java index 8a985e2f3c5..25877d8d773 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java @@ -14,13 +14,15 @@ package com.google.firebase.crashlytics.internal.common; -import static com.google.firebase.crashlytics.internal.common.Utils.awaitEvenIfOnMainThread; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; import android.content.SharedPreferences; import android.os.Build; import androidx.annotation.NonNull; +import com.google.android.gms.tasks.Tasks; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import java.util.Locale; import java.util.Objects; @@ -28,6 +30,7 @@ import java.util.regex.Pattern; public class IdManager implements InstallIdProvider { + private static final int TIMEOUT_MILLIS = 10_000; public static final String DEFAULT_VERSION_NAME = "0.0"; @@ -179,19 +182,22 @@ private String readCachedCrashlyticsInstallId(SharedPreferences prefs) { */ @NonNull public FirebaseInstallationId fetchTrueFid(boolean validate) { + CrashlyticsWorkers.checkNotMainThread(); // This fetch blocks, never do it on main. String fid = null; String authToken = null; if (validate) { // Fetch the auth token first when requested, so the fid will be validated. try { - authToken = awaitEvenIfOnMainThread(firebaseInstallations.getToken(false)).getToken(); + authToken = + Tasks.await(firebaseInstallations.getToken(false), TIMEOUT_MILLIS, MILLISECONDS) + .getToken(); } catch (Exception ex) { Logger.getLogger().w("Error getting Firebase authentication token.", ex); } } try { - fid = awaitEvenIfOnMainThread(firebaseInstallations.getId()); + fid = Tasks.await(firebaseInstallations.getId(), TIMEOUT_MILLIS, MILLISECONDS); } catch (Exception ex) { Logger.getLogger().w("Error getting Firebase installation id.", ex); } @@ -213,7 +219,9 @@ private synchronized String createAndCacheCrashlyticsInstallId( return iid; } - /** @return the package name that identifies this App. */ + /** + * @return the package name that identifies this App. + */ public String getAppIdentifier() { return appIdentifier; } 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 56ff1244439..29e2301591a 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 @@ -341,6 +341,7 @@ private void persistEvent( if (!isFatal) { crashlyticsWorkers.diskWrite.submit( () -> { + Logger.getLogger().d("disk worker: log non-fatal event to persistence"); reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); }); return; @@ -354,16 +355,12 @@ 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()); - } - }); + 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() 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 d224cae38b5..c4ca0c90274 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 @@ -30,7 +30,7 @@ public final class Utils { private static final int BACKGROUND_TIMEOUT_MILLIS = 4_000; /** Timeout in milliseconds for blocking on the main thread. Be careful about ANRs. */ - private static final int MAIN_TIMEOUT_MILLIS = 2_750; + private static final int MAIN_TIMEOUT_MILLIS = 3_000; /** * Blocks until the given Task completes, and then returns the value the Task was resolved with, @@ -44,7 +44,9 @@ public final class Utils { * @return the value that was returned by the task, if successful. * @throws InterruptedException if the method was interrupted * @throws TimeoutException if the method timed out while waiting for the task. + * @deprecated Don't use this. Drain the worker instead. */ + @Deprecated public static T awaitEvenIfOnMainThread(Task task) throws InterruptedException, TimeoutException { CountDownLatch latch = new CountDownLatch(1); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt index dcb4cb6e7b1..4d6234fbc14 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt @@ -16,6 +16,8 @@ package com.google.firebase.crashlytics.internal.concurrency +import android.os.Build +import android.os.Looper import com.google.firebase.crashlytics.internal.Logger import java.util.concurrent.ExecutorService @@ -59,6 +61,12 @@ class CrashlyticsWorkers( /** When enabled, failed preconditions will cause assertion errors for debugging. */ @JvmStatic var enforcement: Boolean = false + @JvmStatic + fun checkNotMainThread() = + checkThread(::isNotMainThread) { + "Must not be called on a main thread, was called on $threadName." + } + @JvmStatic fun checkBlockingThread() = checkThread(::isBlockingThread) { @@ -71,6 +79,13 @@ class CrashlyticsWorkers( "Must be called on a background thread, was called on $threadName." } + private fun isNotMainThread() = + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + !Looper.getMainLooper().isCurrentThread + } else { + Looper.getMainLooper() != Looper.myLooper() + } + private fun isBlockingThread() = threadName.contains("Firebase Blocking Thread #") private fun isBackgroundThread() = threadName.contains("Firebase Background Thread #") @@ -78,7 +93,7 @@ class CrashlyticsWorkers( private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { if (!isCorrectThread()) { Logger.getLogger().d(failureMessage()) - assert(enforcement, failureMessage) + assert(!enforcement, failureMessage) } } } 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 3d87b78eafe..e3e4357e1e2 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 @@ -162,7 +162,6 @@ public Task loadSettingsData( SettingsCacheBehavior cacheBehavior, CrashlyticsWorkers crashlyticsWorkers) { // TODO: Refactor this so that it doesn't do the cache lookup twice when settings are // expired. - // We need to bypass the cache if this is the first time a new build has run so the // backend will know about it. if (!buildInstanceIdentifierChanged()) {