From f51fa9c76e183cbf6ba58cedf97f1ad1a2b49bf8 Mon Sep 17 00:00:00 2001 From: themiswang Date: Mon, 19 Aug 2024 14:20:31 -0400 Subject: [PATCH 01/10] fix finalize report behaviour (#6193) we are sending a task within a task for common worker which cause the async behavior, remove the re-submitting task for report finalization --- .../common/CrashlyticsController.java | 73 +++++++++---------- 1 file changed, 33 insertions(+), 40 deletions(-) diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index 5d60f93a9c8..c915c6e9ea8 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 @@ -351,55 +351,48 @@ Task submitAllReports(Task settingsDataTask) { return 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..."); + crashlyticsWorkers.diskWrite.submit( + () -> { + deleteFiles(listAppExceptionMarkerFiles()); + reportingCoordinator.removeAllReports(); + }); + unsentReportsHandled.trySetResult(null); + return Tasks.forResult(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 crashlyticsWorkers.common.submitTask( - new Callable>() { + 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); } }); } From ad7b5918667d6864a82a6d5540172766725f996f Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Mon, 19 Aug 2024 16:53:31 -0400 Subject: [PATCH 02/10] Avoid Tasks in checkForPreviousCrash (#6194) Avoid `Task`s in `checkForPreviousCrash`. This is a step towards removing the nasty `awaitEvenIfOnMainThread`. --- .../internal/common/CrashlyticsCore.java | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) 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..7c64bd7aaba 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 @@ -433,17 +433,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 +446,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); } } @@ -496,13 +491,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; } From 17b7c2b3d7e0fbbae60af9a43260422c0d5e30cc Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Mon, 19 Aug 2024 17:09:06 -0400 Subject: [PATCH 03/10] Deprecate awaitEvenIfOnMain (#6195) Deprecate `awaitEvenIfOnMain`. Next PR will be to drain the worker and remove this util. --- .../internal/common/CrashlyticsController.java | 2 +- .../crashlytics/internal/common/IdManager.java | 16 ++++++++++++---- .../crashlytics/internal/common/Utils.java | 6 ++++-- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index c915c6e9ea8..201890769d1 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) { @@ -559,7 +560,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/IdManager.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java index 8a985e2f3c5..369781ece61 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.checkBackgroundThread(); // 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/Utils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java index d224cae38b5..76439b0eb0f 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 @@ -27,10 +27,10 @@ @SuppressWarnings({"ResultOfMethodCallIgnored", "UnusedReturnValue"}) public final class Utils { /** Timeout in milliseconds for blocking on background threads. */ - private static final int BACKGROUND_TIMEOUT_MILLIS = 4_000; + private static final int BACKGROUND_TIMEOUT_MILLIS = 5_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 = 4_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); From af0f42a1ff429138c0adaf1d53cd9c7faf1627cb Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Wed, 21 Aug 2024 14:16:58 -0400 Subject: [PATCH 04/10] Adjust time out and disk worker (#6199) Adjust time out and disk worker as discussed. This moves some things off the disk worker, but we plan to put them back after more refactoring. --- .../CrashlyticsReportDataCaptureTest.java | 20 ++++-- .../internal/common/IdManagerTest.java | 63 +++++++++++-------- .../internal/metadata/MetaDataStoreTest.java | 1 + .../common/CrashlyticsController.java | 7 +-- .../internal/common/CrashlyticsCore.java | 23 +++---- .../internal/common/IdManager.java | 1 + .../common/SessionReportingCoordinator.java | 16 ++--- .../crashlytics/internal/common/Utils.java | 4 +- 8 files changed, 73 insertions(+), 62 deletions(-) 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 201890769d1..08f6ee94dc8 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 @@ -359,11 +359,8 @@ Task submitAllReports(Task settingsDataTask) { public Task then(@Nullable Boolean send) throws Exception { if (!send) { Logger.getLogger().v("Deleting cached crash reports..."); - crashlyticsWorkers.diskWrite.submit( - () -> { - deleteFiles(listAppExceptionMarkerFiles()); - reportingCoordinator.removeAllReports(); - }); + deleteFiles(listAppExceptionMarkerFiles()); + reportingCoordinator.removeAllReports(); unsentReportsHandled.trySetResult(null); return Tasks.forResult(null); } 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 7c64bd7aaba..1a7be95b987 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 @@ -468,20 +468,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() { 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 369781ece61..1117ecdd046 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 @@ -104,6 +104,7 @@ private static String formatId(@NonNull String id) { @Override @NonNull public synchronized InstallIds getInstallIds() { + CrashlyticsWorkers.checkBackgroundThread(); if (!shouldRefresh()) { return installIds; } 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..5e35c87ed67 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java @@ -354,16 +354,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 76439b0eb0f..da14b1060c6 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 @@ -27,10 +27,10 @@ @SuppressWarnings({"ResultOfMethodCallIgnored", "UnusedReturnValue"}) public final class Utils { /** Timeout in milliseconds for blocking on background threads. */ - private static final int BACKGROUND_TIMEOUT_MILLIS = 5_000; + private static final int BACKGROUND_TIMEOUT_MILLIS = 3_000; /** Timeout in milliseconds for blocking on the main thread. Be careful about ANRs. */ - private static final int MAIN_TIMEOUT_MILLIS = 4_000; + private static final int MAIN_TIMEOUT_MILLIS = 2_000; /** * Blocks until the given Task completes, and then returns the value the Task was resolved with, From f584b6a0b760aaef5851b7ec5574571361b6d430 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Wed, 21 Aug 2024 16:48:43 -0400 Subject: [PATCH 05/10] Make init suspend on settings only (#6202) Make init suspend on settings only, not on race. This allows us to avoid having a separate queue just for init and settings. If the app has unsent reports, but data collection and send unsent reports both have not been triggered, still let user actions queue up. --- .../internal/common/CrashlyticsCoreTest.java | 11 +---------- .../internal/common/CrashlyticsController.java | 2 -- .../crashlytics/internal/common/CrashlyticsCore.java | 11 +++++------ 3 files changed, 6 insertions(+), 18 deletions(-) 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/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 08f6ee94dc8..68e7c3b9402 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 @@ -339,8 +339,6 @@ Task deleteUnsentReports() { return unsentReportsHandled.getTask(); } - // TODO(b/261014167): Use an explicit executor in continuations. - @SuppressLint("TaskMainThread") Task submitAllReports(Task settingsDataTask) { if (!reportingCoordinator.hasReportsToSend()) { // Just notify the user that there are no reports and stop. 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 1a7be95b987..9f1e824f458 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 @@ -224,13 +224,13 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) /** Performs background initialization asynchronously on the common worker. */ @CanIgnoreReturnValue - public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { + public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { return crashlyticsWorkers.common.submitTask(() -> doBackgroundInitialization(settingsProvider)); } /** Performs background initialization synchronously on the calling thread. */ @CanIgnoreReturnValue - private Task doBackgroundInitialization(SettingsProvider settingsProvider) { + private Task doBackgroundInitialization(SettingsProvider settingsProvider) { CrashlyticsWorkers.checkBackgroundThread(); // create the marker for this run markInitializationStarted(); @@ -254,10 +254,9 @@ private Task doBackgroundInitialization(SettingsProvider 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()); + Task settings = settingsProvider.getSettingsAsync(); + controller.submitAllReports(settings); + return settings; } catch (Exception e) { Logger.getLogger() .e("Crashlytics encountered a problem during asynchronous initialization.", e); From a2261d863d9520e6206d0818c6fbd3da2c41a9b8 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 22 Aug 2024 11:27:50 -0400 Subject: [PATCH 06/10] Fix assert --- .../crashlytics/internal/concurrency/CrashlyticsWorkers.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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..531a477c862 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 @@ -78,7 +78,7 @@ class CrashlyticsWorkers( private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { if (!isCorrectThread()) { Logger.getLogger().d(failureMessage()) - assert(enforcement, failureMessage) + assert(!enforcement, failureMessage) } } } From e896ac41d97ba6f860ce258a5a9f342851965a75 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 22 Aug 2024 11:44:24 -0400 Subject: [PATCH 07/10] Add a precondition check for not main thread (#6204) Add a precondition check for not main thread. This is more useful when we just want to enforce not main thread, not a specific thread. --- .../crashlytics/internal/common/IdManager.java | 3 +-- .../internal/concurrency/CrashlyticsWorkers.kt | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) 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 1117ecdd046..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 @@ -104,7 +104,6 @@ private static String formatId(@NonNull String id) { @Override @NonNull public synchronized InstallIds getInstallIds() { - CrashlyticsWorkers.checkBackgroundThread(); if (!shouldRefresh()) { return installIds; } @@ -183,7 +182,7 @@ private String readCachedCrashlyticsInstallId(SharedPreferences prefs) { */ @NonNull public FirebaseInstallationId fetchTrueFid(boolean validate) { - CrashlyticsWorkers.checkBackgroundThread(); // This fetch blocks, never do it on main. + CrashlyticsWorkers.checkNotMainThread(); // This fetch blocks, never do it on main. String fid = null; String authToken = null; 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 531a477c862..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 #") From 079548f3b578117f473d03285a526202d2839a2b Mon Sep 17 00:00:00 2001 From: themiswang Date: Tue, 27 Aug 2024 09:57:23 -0400 Subject: [PATCH 08/10] Fix user action blocking behaviour when data collection disabled (#6218) Make doBackgroundInitializationAsync submit a callable instead of submit task Submit task results in a blocking behaviour when `SettingsTask` cannot be resolved during initialization Remove un-necessary task return --- firebase-crashlytics-ndk/gradle.properties | 2 +- firebase-crashlytics/gradle.properties | 2 +- .../common/CrashlyticsControllerTest.java | 22 +++++++++---------- .../common/CrashlyticsController.java | 8 +++---- .../internal/common/CrashlyticsCore.java | 16 +++++--------- .../common/SessionReportingCoordinator.java | 1 + .../internal/settings/SettingsController.java | 1 - 7 files changed, 22 insertions(+), 30 deletions(-) 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/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/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 68e7c3b9402..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 @@ -339,16 +339,16 @@ Task deleteUnsentReports() { return unsentReportsHandled.getTask(); } - 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() { @@ -360,6 +360,7 @@ public Task then(@Nullable Boolean send) throws Exception { deleteFiles(listAppExceptionMarkerFiles()); reportingCoordinator.removeAllReports(); unsentReportsHandled.trySetResult(null); + return Tasks.forResult(null); } @@ -371,7 +372,6 @@ public Task then(@Nullable Boolean send) throws Exception { // Signal to the settings fetch and onboarding that we have explicit // permission. dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); - return settingsDataTask.onSuccessTask( crashlyticsWorkers.common, new SuccessContinuation() { 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 9f1e824f458..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; @@ -224,13 +223,12 @@ 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)); + public Task doBackgroundInitializationAsync(SettingsProvider 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,21 +244,17 @@ private Task doBackgroundInitialization(SettingsProvider settingsProvi 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."); } - Task settings = settingsProvider.getSettingsAsync(); - controller.submitAllReports(settings); - return settings; + 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 diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java index 5e35c87ed67..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; 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()) { From 91397814d317ba374629445301607ac09d06f9e4 Mon Sep 17 00:00:00 2001 From: themiswang Date: Tue, 27 Aug 2024 13:38:03 -0400 Subject: [PATCH 09/10] increase timeout interval (#6219) --- .../google/firebase/crashlytics/internal/common/Utils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 da14b1060c6..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 @@ -27,10 +27,10 @@ @SuppressWarnings({"ResultOfMethodCallIgnored", "UnusedReturnValue"}) public final class Utils { /** Timeout in milliseconds for blocking on background threads. */ - private static final int BACKGROUND_TIMEOUT_MILLIS = 3_000; + 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_000; + 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, From d70c119744006adab846b1d7264e79db79136bb2 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 5 Sep 2024 11:53:09 -0400 Subject: [PATCH 10/10] Update CHANGELOG (#6231) --- firebase-crashlytics/CHANGELOG.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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