From cac0af966207e7154aa45a153151dd5347aae1a6 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 17 Sep 2024 14:56:21 -0400 Subject: [PATCH 01/11] Revert "Remove changes related to persistence issue (#6191)" This reverts commit 4d6e112267814327da70b403113654d945436b90. --- .../firebase-crashlytics.gradle | 1 + .../internal/CrashlyticsWorkerTest.java | 429 -------- .../common/CrashlyticsControllerTest.java | 65 +- .../CrashlyticsCoreInitializationTest.java | 12 +- .../internal/common/CrashlyticsCoreTest.java | 22 +- .../SessionReportingCoordinatorTest.java | 62 +- .../concurrency/ConcurrencyTesting.java | 45 + .../concurrency/CrashlyticsTasksTest.java | 240 +++++ .../concurrency/CrashlyticsWorkerTest.java | 918 ++++++++++++++++++ .../internal/metadata/MetaDataStoreTest.java | 226 +++-- .../DefaultSettingsControllerTest.java | 40 +- .../settings/DefaultSettingsSpiCallTest.java | 8 +- .../crashlytics/CrashlyticsRegistrar.java | 12 +- .../crashlytics/FirebaseCrashlytics.java | 39 +- .../internal/CrashlyticsPreconditions.kt | 86 -- .../internal/CrashlyticsWorker.java | 128 --- .../internal/common/CommonUtils.java | 16 +- .../common/CrashlyticsBackgroundWorker.java | 166 ---- .../common/CrashlyticsController.java | 118 +-- .../internal/common/CrashlyticsCore.java | 107 +- .../common/CrashlyticsLifecycleEvents.java | 51 - .../common/DataCollectionArbiter.java | 10 +- .../common/SessionReportingCoordinator.java | 72 +- .../crashlytics/internal/common/Utils.java | 70 -- .../concurrency/CrashlyticsTasks.java | 69 ++ .../concurrency/CrashlyticsWorker.java | 212 ++++ .../concurrency/CrashlyticsWorkers.kt | 85 ++ .../internal/concurrency/package-info.java | 18 + .../internal/metadata/UserMetadata.java | 66 +- .../internal/network/HttpGetRequest.java | 2 + .../settings/DefaultSettingsSpiCall.java | 2 + .../internal/settings/SettingsController.java | 27 +- .../CrashlyticsControllerRobolectricTest.java | 37 +- ...onReportingCoordinatorRobolectricTest.java | 14 +- 34 files changed, 2143 insertions(+), 1332 deletions(-) delete mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java diff --git a/firebase-crashlytics/firebase-crashlytics.gradle b/firebase-crashlytics/firebase-crashlytics.gradle index 3c450c0d68a..11656334809 100644 --- a/firebase-crashlytics/firebase-crashlytics.gradle +++ b/firebase-crashlytics/firebase-crashlytics.gradle @@ -98,6 +98,7 @@ dependencies { testImplementation(libs.mockito.core) testImplementation(libs.robolectric) testImplementation(libs.truth) + testImplementation(project(":integ-testing")) androidTestImplementation(libs.androidx.test.core) androidTestImplementation(libs.androidx.test.runner) diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java deleted file mode 100644 index 63c9a9ce6d2..00000000000 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java +++ /dev/null @@ -1,429 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal; - -import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; - -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import com.google.firebase.concurrent.TestOnlyExecutors; -import java.io.IOException; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.concurrent.CancellationException; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executors; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -public class CrashlyticsWorkerTest { - private CrashlyticsWorker crashlyticsWorker; - - @Before - public void setUp() { - crashlyticsWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); - } - - @After - public void tearDown() throws Exception { - // Drain the worker, just in case any test cases would fail but didn't await. - crashlyticsWorker.await(); - } - - @Test - public void executesTasksOnThreadPool() throws Exception { - Set threads = new HashSet<>(); - - // Find thread names by adding the names we touch to the set. - for (int i = 0; i < 100; i++) { - crashlyticsWorker.submit(() -> threads.add(Thread.currentThread().getName())); - } - - crashlyticsWorker.await(); - - // Verify that we touched at lease some of the expected background threads. - assertThat(threads) - .containsAnyOf( - "Firebase Background Thread #0", - "Firebase Background Thread #1", - "Firebase Background Thread #2", - "Firebase Background Thread #3"); - } - - @Test - public void executesTasksInOrder() throws Exception { - List list = new ArrayList<>(); - - // Add sequential numbers to the list to validate tasks execute in order. - for (int i = 0; i < 100; i++) { - int sequential = i; - crashlyticsWorker.submit(() -> list.add(sequential)); - } - - crashlyticsWorker.await(); - - // Verify that the tasks executed in order. - assertThat(list).isInOrder(); - } - - @Test - public void executesTasksSequentially() throws Exception { - List list = new ArrayList<>(); - AtomicBoolean reentrant = new AtomicBoolean(false); - - for (int i = 0; i < 100; i++) { - int sequential = i; - crashlyticsWorker.submit( - () -> { - if (reentrant.get()) { - // Return early if two runnables ran at the same time. - return; - } - - reentrant.set(true); - // Sleep a bit to simulate some work. - sleep(5); - list.add(sequential); - reentrant.set(false); - }); - } - - crashlyticsWorker.await(); - - // Verify that all the runnable tasks executed, one at a time, and in order. - assertThat(list).hasSize(100); - assertThat(list).isInOrder(); - } - - @Test - public void submitCallableThatReturns() throws Exception { - String ender = "Remember, the enemy's gate is down."; - Task task = crashlyticsWorker.submit(() -> ender); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(ender); - } - - @Test - public void submitCallableThatReturnsNull() throws Exception { - Task task = crashlyticsWorker.submit(() -> null); - - String result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitCallableThatThrows() { - Task task = - crashlyticsWorker.submit( - () -> { - throw new Exception("I threw in the callable"); - }); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the callable"); - } - - @Test - public void submitCallableThatThrowsThenReturns() throws Exception { - Task throwingTask = - crashlyticsWorker.submit( - () -> { - throw new IOException(); - }); - - assertThrows(ExecutionException.class, () -> Tasks.await(throwingTask)); - - String hiro = - "When you are wrestling for possession of a sword, the man with the handle always wins."; - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(hiro)); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(hiro); - } - - @Test - public void submitRunnable() throws Exception { - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - // A Runnable does not return, so the task evaluates to null. - assertThat(result).isNull(); - } - - @Test - public void submitRunnableThatThrows() { - Task task = - crashlyticsWorker.submit( - (Runnable) - () -> { - throw new RuntimeException("I threw in the runnable"); - }); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the runnable"); - } - - @Test - public void submitRunnableThatThrowsThenReturns() throws Exception { - Task thowingTask = - crashlyticsWorker.submit( - (Runnable) - () -> { - throw new IllegalArgumentException(); - }); - - assertThrows(ExecutionException.class, () -> Tasks.await(thowingTask)); - - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitTaskThatReturns() throws Exception { - String skippy = "Think of the problem as an enemy, and defeat them in detail."; - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(skippy)); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(skippy); - } - - @Test - public void submitTaskThatReturnsNull() throws Exception { - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(null)); - - String result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitTaskThatThrows() { - Task task = - crashlyticsWorker.submitTask( - () -> Tasks.forException(new Exception("Thrown from a task."))); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Thrown from a task."); - } - - @Test - public void submitTaskThatThrowsThenReturns() throws Exception { - crashlyticsWorker.submitTask(() -> Tasks.forException(new IllegalStateException())); - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("The Hail Mary")); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo("The Hail Mary"); - } - - @Test - public void submitTaskThatCancels() { - Task task = crashlyticsWorker.submitTask(Tasks::forCanceled); - - CancellationException thrown = - assertThrows(CancellationException.class, () -> Tasks.await(task)); - - assertThat(task.isCanceled()).isTrue(); - assertThat(thrown).hasMessageThat().contains("Task is already canceled"); - } - - @Test - public void submitTaskThatCancelsThenReturns() throws Exception { - crashlyticsWorker.submitTask(Tasks::forCanceled); - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Flying Dutchman")); - - String result = Tasks.await(task); - - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isEqualTo("Flying Dutchman"); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenReturns() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit another task. - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Valkyrie")); - - String result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isEqualTo("Valkyrie"); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenCallable() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit a simple callable. - Task task = crashlyticsWorker.submit(() -> true); - - boolean result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isTrue(); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenRunnable() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit an empty runnable. - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isNull(); - } - - @Test - public void submitTaskFromAnotherWorker() throws Exception { - Task otherTask = - new CrashlyticsWorker(TestOnlyExecutors.blocking()) - .submit(() -> "Dog's fine. Just sleeping."); - - // This will not use a background thread while waiting for the task on blocking thread. - Task task = crashlyticsWorker.submitTask(() -> otherTask); - - String result = Tasks.await(task); - assertThat(result).isEqualTo("Dog's fine. Just sleeping."); - } - - @Test - public void submitTaskFromAnotherWorkerThatThrows() throws Exception { - Task otherTask = - new CrashlyticsWorker(TestOnlyExecutors.blocking()) - .submitTask(() -> Tasks.forException(new IndexOutOfBoundsException())); - - // Await on the throwing task to force the exception to propagate threw the local worker. - Task task = crashlyticsWorker.submitTask(() -> otherTask); - assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - // Submit another task to local worker to verify the chain did not break. - Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5f375a86)); - - int localResult = Tasks.await(localTask); - - assertThat(otherTask.isSuccessful()).isFalse(); - assertThat(localTask.isSuccessful()).isTrue(); - assertThat(localResult).isEqualTo(0x5f375a86); - } - - @Test - public void submitTaskFromAnotherWorkerThatCancels() throws Exception { - Task otherCancelled = - new CrashlyticsWorker(TestOnlyExecutors.blocking()).submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate threw the local worker. - Task task = crashlyticsWorker.submitTask(() -> otherCancelled); - assertThrows(CancellationException.class, () -> Tasks.await(task)); - - // Submit another task to local worker to verify the chain did not break. - Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5fe6eb50c7b537a9L)); - - long localResult = Tasks.await(localTask); - - assertThat(otherCancelled.isCanceled()).isTrue(); - assertThat(localTask.isCanceled()).isFalse(); - assertThat(localResult).isEqualTo(0x5fe6eb50c7b537a9L); - } - - @Test - public void submitTaskFromAnotherWorkerDoesNotUseLocalThreads() throws Exception { - // Setup a "local" worker. - ThreadPoolExecutor localExecutor = (ThreadPoolExecutor) Executors.newFixedThreadPool(4); - CrashlyticsWorker localWorker = new CrashlyticsWorker(localExecutor); - - // Use a task off crashlyticsWorker to represent an other task. - Task otherTask = - crashlyticsWorker.submit( - () -> { - sleep(30); - return localExecutor.getActiveCount(); - }); - - // No active threads yet. - assertThat(localExecutor.getActiveCount()).isEqualTo(0); - - // 1 active thread when doing a local task. - assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); - - // 0 active local threads when waiting for other task. - // Waiting for a task from another worker does not block a local thread. - assertThat(Tasks.await(localWorker.submitTask(() -> otherTask))).isEqualTo(0); - - // 1 active thread when doing a task. - assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); - - // No active threads after. - assertThat(localExecutor.getActiveCount()).isEqualTo(0); - } - - @Test - public void submitTaskWhenThreadPoolFull() { - // Fill the underlying executor thread pool. - for (int i = 0; i < 10; i++) { - crashlyticsWorker.getExecutor().execute(() -> sleep(40)); - } - - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(42)); - - // The underlying thread pool is full with tasks that will take longer than this timeout. - assertThrows(TimeoutException.class, () -> Tasks.await(task, 30, TimeUnit.MILLISECONDS)); - } - - private static void sleep(long millis) { - try { - Thread.sleep(millis); - } catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - } -} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java index 904ad8bb97b..34a1d72a47d 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java @@ -34,11 +34,13 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.NativeSessionFileProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; @@ -55,13 +57,15 @@ import java.util.TreeSet; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; -import org.junit.Test; import org.mockito.ArgumentCaptor; public class CrashlyticsControllerTest extends CrashlyticsTestCase { private static final String GOOGLE_APP_ID = "google:app:id"; private static final String SESSION_ID = "session_id"; + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + private Context testContext; private IdManager idManager; private SettingsProvider testSettingsProvider; @@ -99,6 +103,12 @@ protected void setUp() throws Exception { when(testSettingsProvider.getSettingsAsync()).thenReturn(Tasks.forResult(testSettings)); } + @Override + protected void tearDown() throws Exception { + super.tearDown(); + crashlyticsWorkers.common.await(); + } + /** A convenience class for building CrashlyticsController instances for testing. */ private class ControllerBuilder { private DataCollectionArbiter dataCollectionArbiter; @@ -106,7 +116,6 @@ private class ControllerBuilder { private AnalyticsEventLogger analyticsEventLogger; private SessionReportingCoordinator sessionReportingCoordinator; - private CrashlyticsBackgroundWorker backgroundWorker; private LogFileManager logFileManager = null; private UserMetadata userMetadata = null; @@ -118,8 +127,6 @@ private class ControllerBuilder { analyticsEventLogger = mock(AnalyticsEventLogger.class); sessionReportingCoordinator = mockSessionReportingCoordinator; - - backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService()); } ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) { @@ -168,7 +175,6 @@ public CrashlyticsController build() { final CrashlyticsController controller = new CrashlyticsController( testContext.getApplicationContext(), - backgroundWorker, idManager, dataCollectionArbiter, testFileStore, @@ -179,7 +185,8 @@ public CrashlyticsController build() { sessionReportingCoordinator, nativeComponent, analyticsEventLogger, - mock(CrashlyticsAppQualitySessionsSubscriber.class)); + mock(CrashlyticsAppQualitySessionsSubscriber.class), + crashlyticsWorkers); return controller; } } @@ -206,7 +213,10 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal() .thenReturn(new TreeSet<>(Collections.singleton(sessionId))); controller.writeNonFatalException(thread, nonFatal); - controller.doCloseSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(testSettingsProvider)); + + crashlyticsWorkers.common.await(); + crashlyticsWorkers.diskWrite.await(); verify(mockSessionReportingCoordinator) .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong()); @@ -228,9 +238,8 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th .persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong()); } - @Test @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testOnDemandFatal_callLogFatalException() { + public void testOnDemandFatal_callLogFatalException() throws Exception { Thread thread = Thread.currentThread(); Exception fatal = new RuntimeException("Fatal"); Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class); @@ -246,6 +255,8 @@ public void testOnDemandFatal_callLogFatalException() { controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider); controller.logFatalException(thread, fatal); + crashlyticsWorkers.common.await(); + verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID))); } @@ -323,7 +334,9 @@ public File getOsFile() { final CrashlyticsController controller = builder().setNativeComponent(mockNativeComponent).setLogFileManager(logFileManager).build(); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); + verify(mockSessionReportingCoordinator) .finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any()); verify(mockSessionReportingCoordinator, never()) @@ -331,9 +344,10 @@ public File getOsFile() { } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testMissingNativeComponentCausesNoReports() { + public void testMissingNativeComponentCausesNoReports() throws Exception { final CrashlyticsController controller = createController(); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); List sessions = testFileStore.getAllOpenSessionIds(); for (String sessionId : sessions) { @@ -363,13 +377,15 @@ public void testLoggedExceptionsAfterCrashOk() { */ // FIXME: Validate this test works as intended @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testLogStringAfterCrashOk() { + public void testLogStringAfterCrashOk() throws Exception { final CrashlyticsController controller = builder().build(); controller.handleUncaughtException( testSettingsProvider, Thread.currentThread(), new RuntimeException()); // This should not throw. - controller.writeToLog(System.currentTimeMillis(), "Hi"); + crashlyticsWorkers.diskWrite.submit( + () -> controller.writeToLog(System.currentTimeMillis(), "Hi")); + crashlyticsWorkers.diskWrite.await(); } /** @@ -384,7 +400,8 @@ public void testFinalizeSessionAfterCrashOk() throws Exception { testSettingsProvider, Thread.currentThread(), new RuntimeException()); // This should not throw. - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo @@ -427,7 +444,7 @@ public void testUploadDisabledThenOptIn() throws Exception { final DataCollectionArbiter arbiter = mock(DataCollectionArbiter.class); when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(false); - when(arbiter.waitForDataCollectionPermission(any(Executor.class))) + when(arbiter.waitForDataCollectionPermission()) .thenReturn(new TaskCompletionSource().getTask()); when(arbiter.waitForAutomaticDataCollectionEnabled()) .thenReturn(new TaskCompletionSource().getTask()); @@ -473,6 +490,8 @@ public void testUploadDisabledThenOptOut() throws Exception { await(controller.deleteUnsentReports()); await(task); + crashlyticsWorkers.diskWrite.await(); + verify(mockSessionReportingCoordinator).removeAllReports(); verifyNoMoreInteractions(mockSessionReportingCoordinator); } @@ -535,7 +554,7 @@ public void testUploadDisabledThenEnabled() throws Exception { } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testFatalEvent_sendsAppExceptionEvent() { + public void testFatalEvent_sendsAppExceptionEvent() throws Exception { final String sessionId = "sessionId"; final LogFileManager logFileManager = new LogFileManager(testFileStore); final AnalyticsEventLogger mockFirebaseAnalyticsLogger = mock(AnalyticsEventLogger.class); @@ -548,10 +567,14 @@ public void testFatalEvent_sendsAppExceptionEvent() { when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singleton(sessionId))); - controller.openSession(SESSION_ID); - controller.handleUncaughtException( - testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit( + () -> { + controller.openSession(SESSION_ID); + controller.handleUncaughtException( + testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); + controller.finalizeSessions(testSettingsProvider); + }); + crashlyticsWorkers.common.await(); assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger); } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java index 9b579a66a62..1bdb3fc183d 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java @@ -27,6 +27,7 @@ import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; @@ -34,6 +35,7 @@ import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.UnavailableAnalyticsEventLogger; import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.persistence.FileStore; import com.google.firebase.crashlytics.internal.settings.Settings; import com.google.firebase.crashlytics.internal.settings.SettingsController; @@ -44,7 +46,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutorService; public class CrashlyticsCoreInitializationTest extends CrashlyticsTestCase { @@ -89,8 +90,8 @@ private static final class CoreBuilder { private IdManager idManager; private CrashlyticsNativeComponent nativeComponent; private DataCollectionArbiter arbiter; - private ExecutorService crashHandlerExecutor; private FileStore fileStore; + private CrashlyticsWorkers crashlyticsWorkers; public CoreBuilder(Context context, FirebaseOptions firebaseOptions) { app = mock(FirebaseApp.class); @@ -119,7 +120,8 @@ public void whenAvailable( arbiter = mock(DataCollectionArbiter.class); when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(true); - crashHandlerExecutor = new SameThreadExecutorService(); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); fileStore = new FileStore(context); } @@ -142,9 +144,9 @@ public CrashlyticsCore build() { new DisabledBreadcrumbSource(), new UnavailableAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + crashlyticsWorkers); } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java index 17064da9f37..728757d30e8 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java @@ -28,6 +28,7 @@ import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.BuildConfig; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; @@ -38,6 +39,7 @@ import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbHandler; import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbSource; import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.persistence.FileStore; import com.google.firebase.crashlytics.internal.settings.Settings; @@ -68,6 +70,8 @@ public void whenAvailable( private CrashlyticsCore crashlyticsCore; private BreadcrumbSource mockBreadcrumbSource; + private static final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Override protected void setUp() throws Exception { @@ -91,7 +95,7 @@ public void testCustomAttributes() throws Exception { final String id = "id012345"; crashlyticsCore.setUserId(id); - + crashlyticsWorkers.common.await(); assertEquals(id, metadata.getUserId()); final StringBuffer idBuffer = new StringBuffer(id); @@ -102,11 +106,13 @@ public void testCustomAttributes() throws Exception { final String superLongId = longId + "more chars"; crashlyticsCore.setUserId(superLongId); + crashlyticsWorkers.common.await(); assertEquals(longId, metadata.getUserId()); final String key1 = "key1"; final String value1 = "value1"; crashlyticsCore.setCustomKey(key1, value1); + crashlyticsWorkers.common.await(); assertEquals(value1, metadata.getCustomKeys().get(key1)); // Adding an existing key with the same value should return false @@ -120,6 +126,7 @@ public void testCustomAttributes() throws Exception { // test truncation of custom keys and attributes crashlyticsCore.setCustomKey(superLongId, superLongValue); + crashlyticsWorkers.common.await(); assertNull(metadata.getCustomKeys().get(superLongId)); assertEquals(longValue, metadata.getCustomKeys().get(longId)); @@ -128,23 +135,28 @@ public void testCustomAttributes() throws Exception { final String key = "key" + i; final String value = "value" + i; crashlyticsCore.setCustomKey(key, value); + crashlyticsWorkers.common.await(); assertEquals(value, metadata.getCustomKeys().get(key)); } // should be full now, extra key, value pairs will be dropped. final String key = "new key"; crashlyticsCore.setCustomKey(key, "some value"); + crashlyticsWorkers.common.await(); assertFalse(metadata.getCustomKeys().containsKey(key)); // should be able to update existing keys crashlyticsCore.setCustomKey(key1, longValue); + crashlyticsWorkers.common.await(); assertEquals(longValue, metadata.getCustomKeys().get(key1)); // when we set a key to null, it should still exist with an empty value crashlyticsCore.setCustomKey(key1, null); + crashlyticsWorkers.common.await(); assertEquals("", metadata.getCustomKeys().get(key1)); // keys and values are trimmed. crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " "); + crashlyticsWorkers.common.await(); assertTrue(metadata.getCustomKeys().containsKey(key1)); assertEquals(longValue, metadata.getCustomKeys().get(key1)); } @@ -195,6 +207,7 @@ public void testBulkCustomKeys() throws Exception { keysAndValues.put(intKey, String.valueOf(intValue)); crashlyticsCore.setCustomKeys(keysAndValues); + crashlyticsWorkers.common.await(); assertEquals(stringValue, metadata.getCustomKeys().get(stringKey)); assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey)); @@ -215,6 +228,7 @@ public void testBulkCustomKeys() throws Exception { addlKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(addlKeysAndValues); + crashlyticsWorkers.common.await(); // Ensure all keys have been set assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA); @@ -232,6 +246,7 @@ public void testBulkCustomKeys() throws Exception { extraKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(extraKeysAndValues); + crashlyticsWorkers.common.await(); // Make sure the extra keys were not added for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) { @@ -257,6 +272,7 @@ public void testBulkCustomKeys() throws Exception { updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue)); crashlyticsCore.setCustomKeys(updatedKeysAndValues); + crashlyticsWorkers.common.await(); assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey)); assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey))); @@ -427,9 +443,9 @@ CrashlyticsCore build(Context context) { breadcrumbSource, new UnavailableAnalyticsEventLogger(), new FileStore(context), - new SameThreadExecutorService(), mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + crashlyticsWorkers); return crashlyticsCore; } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java index 4aeb055eda5..4c42bdc8069 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java @@ -33,6 +33,8 @@ 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.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; @@ -69,6 +71,9 @@ public class SessionReportingCoordinatorTest { private SessionReportingCoordinator reportingCoordinator; + private CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -80,7 +85,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + crashlyticsWorkers); } @Test @@ -116,7 +122,8 @@ public void testFatalEvent_persistsHighPriorityEventWithAllThreadsForSessionId() } @Test - public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() { + public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() + throws Exception { final String eventType = "error"; final String sessionId = "testSessionId"; final long timestamp = System.currentTimeMillis(); @@ -126,6 +133,8 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + final boolean expectedAllThreads = false; final boolean expectedHighPriority = false; @@ -136,7 +145,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes } @Test - public void testNonFatalEvent_addsLogsToEvent() { + public void testNonFatalEvent_addsLogsToEvent() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -149,6 +158,8 @@ public void testNonFatalEvent_addsLogsToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventBuilder) .setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build()); verify(mockEventBuilder).build(); @@ -156,7 +167,7 @@ public void testNonFatalEvent_addsLogsToEvent() { } @Test - public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -168,6 +179,8 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class)); verify(mockEventBuilder).build(); verify(logFileManager, never()).clearLog(); @@ -212,7 +225,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() { } @Test - public void testNonFatalEvent_addsSortedKeysToEvent() { + public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -243,6 +256,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes); verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes); verify(mockEventAppBuilder).build(); @@ -252,7 +267,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { } @Test - public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -266,6 +281,8 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -274,7 +291,7 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { } @Test - public void testNonFatalEvent_addRolloutsEvent() { + public void testNonFatalEvent_addRolloutsEvent() throws Exception { long timestamp = System.currentTimeMillis(); String sessionId = "testSessionId"; mockEventInteractions(); @@ -287,6 +304,8 @@ public void testNonFatalEvent_addRolloutsEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -417,35 +436,6 @@ public void testFatalEvent_addRolloutsToEvent() { verify(mockEventBuilder, times(2)).build(); } - @Test - public void onLog_writesToLogFileManager() { - long timestamp = System.currentTimeMillis(); - String log = "this is a log"; - - reportingCoordinator.onLog(timestamp, log); - - verify(logFileManager).writeToLog(timestamp, log); - } - - @Test - public void onCustomKey_writesToReportMetadata() { - final String key = "key"; - final String value = "value"; - - reportingCoordinator.onCustomKey(key, value); - - verify(reportMetadata).setCustomKey(key, value); - } - - @Test - public void onUserId_writesUserToReportMetadata() { - final String userId = "testUser"; - - reportingCoordinator.onUserId(userId); - - verify(reportMetadata).setUserId(userId); - } - @Test public void testFinalizeSessionWithNativeEvent_createsCrashlyticsReportWithNativePayload() { byte[] testBytes = {0, 2, 20, 10}; diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java new file mode 100644 index 00000000000..bb086f920cd --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java @@ -0,0 +1,45 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +/** Convenience methods for use in Crashlytics concurrency tests. */ +class ConcurrencyTesting { + + /** Returns the current thread's name. */ + static String getThreadName() { + return Thread.currentThread().getName(); + } + + /** Creates a simple executor that runs on a single named thread. */ + static ExecutorService newNamedSingleThreadExecutor(String name) { + return Executors.newSingleThreadExecutor(runnable -> new Thread(runnable, name)); + } + + /** Convenient sleep method that propagates the interruption, but does not throw. */ + static void sleep(long millis) { + try { + Thread.sleep(millis); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } + + private ConcurrencyTesting() {} +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java new file mode 100644 index 00000000000..f789172d39a --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java @@ -0,0 +1,240 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.sleep; +import static org.junit.Assert.assertThrows; + +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import org.junit.Test; + +public class CrashlyticsTasksTest { + + @Test + public void raceReturnsFirstResult() throws Exception { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submit( + () -> { + sleep(20); + return "first"; + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submit( + () -> { + sleep(40); + return "slow"; + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceReturnsFirstException() { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forException(new ArithmeticException()); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forException(new IllegalStateException()); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + // The first task throws an ArithmeticException. + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void raceFirstCancelsReturnsSecondResult() throws Exception { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forCanceled(); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forResult("I am slow but didn't cancel."); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("I am slow but didn't cancel."); + } + + @Test + public void raceBothCancel() { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forCanceled(); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forCanceled(); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + + // Both cancelled, so cancel the race result. + assertThrows(CancellationException.class, () -> Tasks.await(task)); + } + + @Test + public void raceTasksOnSameWorker() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(TestOnlyExecutors.background()); + + // Create 2 tasks on the same worker to race. + Task task1 = + worker.submit( + () -> { + sleep(20); + return "first"; + }); + Task task2 = + worker.submit( + () -> { + sleep(30); + return "second"; + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTasksOnSameSingleThreadWorker() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(Executors.newSingleThreadExecutor()); + + // Create 2 tasks on the same worker to race. + Task task1 = worker.submit(() -> "first"); + Task task2 = worker.submit(() -> "second"); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + // The first task is submitted to this single thread worker first, so will always be first. + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTaskOneOnWorkerAnotherNeverCompletes() throws Exception { + // Create a task on a worker, and another that never completes, to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()).submit(() -> "first"); + Task task2 = new TaskCompletionSource().getTask(); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTaskOneOnWorkerAnotherOtherThatCompletesFirst() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(TestOnlyExecutors.background()); + + // Add a decoy task to the worker to take up some time. + worker.submitTask( + () -> { + sleep(20); + return Tasks.forResult(null); + }); + + // Create a task on this worker, and another, to race. + Task task1 = worker.submit(() -> "worker"); + TaskCompletionSource task2 = new TaskCompletionSource<>(); + task2.trySetResult("other"); + + Task task = CrashlyticsTasks.race(task1, task2.getTask()); + String result = Tasks.await(task); + + // The other tasks completes first because the first task is queued up later on the worker. + assertThat(result).isEqualTo("other"); + } + + @Test + public void raceNoExecutor() throws Exception { + // Create tasks with no explicit executor. + TaskCompletionSource task1 = new TaskCompletionSource<>(); + TaskCompletionSource task2 = new TaskCompletionSource<>(); + + Task task = CrashlyticsTasks.race(task1.getTask(), task2.getTask()); + + // Set a task result from another thread. + new Thread( + () -> { + sleep(30); + task1.trySetResult("yes"); + }) + .start(); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("yes"); + } + + @Test + public void raceTasksThatNeverResolve() { + // Create tasks that will never resolve. + Task task1 = new TaskCompletionSource().getTask(); + Task task2 = new TaskCompletionSource().getTask(); + + Task task = CrashlyticsTasks.race(task1, task2); + + // Since the tasks never resolve, the await will timeout. + assertThrows(TimeoutException.class, () -> Tasks.await(task, 300, TimeUnit.MILLISECONDS)); + } +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java new file mode 100644 index 00000000000..96c78bb4abc --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java @@ -0,0 +1,918 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.getThreadName; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.newNamedSingleThreadExecutor; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.sleep; +import static org.junit.Assert.assertThrows; + +import com.google.android.gms.tasks.CancellationToken; +import com.google.android.gms.tasks.CancellationTokenSource; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; +import java.io.IOException; +import java.util.Collections; +import java.util.HashSet; +import java.util.Queue; +import java.util.Set; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class CrashlyticsWorkerTest { + private CrashlyticsWorker crashlyticsWorker; + + @Before + public void setUp() { + crashlyticsWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + } + + @After + public void tearDown() throws Exception { + // Drain the worker, just in case any test cases would fail but didn't await. + crashlyticsWorker.await(); + } + + @Test + public void executesTasksOnThreadPool() throws Exception { + Set threads = Collections.synchronizedSet(new HashSet<>()); + + // Find thread names by adding the names we touch to the set. + for (int i = 0; i < 100; i++) { + crashlyticsWorker.submit(() -> threads.add(getThreadName())); + } + + crashlyticsWorker.await(); + + // Verify that we touched at lease some of the expected background threads. + assertThat(threads) + .containsAnyOf( + "Firebase Background Thread #0", + "Firebase Background Thread #1", + "Firebase Background Thread #2", + "Firebase Background Thread #3"); + } + + @Test + public void executesTasksInOrder() throws Exception { + Queue list = new ConcurrentLinkedQueue<>(); + + // Add sequential numbers to the list to validate tasks execute in order. + for (int i = 0; i < 100; i++) { + int sequential = i; + crashlyticsWorker.submit(() -> list.add(sequential)); + } + + crashlyticsWorker.await(); + + // Verify that the tasks executed in order. + assertThat(list).isInOrder(); + } + + @Test + public void executesTasksSequentially() throws Exception { + Queue list = new ConcurrentLinkedQueue<>(); + AtomicBoolean reentrant = new AtomicBoolean(false); + + for (int i = 0; i < 100; i++) { + int sequential = i; + crashlyticsWorker.submit( + () -> { + if (reentrant.get()) { + // Return early if two runnables ran at the same time. + return; + } + + reentrant.set(true); + // Sleep a bit to simulate some work. + sleep(5); + list.add(sequential); + reentrant.set(false); + }); + } + + crashlyticsWorker.await(); + + // Verify that all the runnable tasks executed, one at a time, and in order. + assertThat(list).hasSize(100); + assertThat(list).isInOrder(); + } + + @Test + public void submitCallableThatReturns() throws Exception { + String ender = "Remember, the enemy's gate is down."; + Task task = crashlyticsWorker.submit(() -> ender); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(ender); + } + + @Test + public void submitCallableThatReturnsNull() throws Exception { + Task task = crashlyticsWorker.submit(() -> null); + + String result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitCallableThatThrows() { + Task task = + crashlyticsWorker.submit( + () -> { + throw new Exception("I threw in the callable"); + }); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the callable"); + } + + @Test + public void submitCallableThatThrowsThenReturns() throws Exception { + Task throwingTask = + crashlyticsWorker.submit( + () -> { + throw new IOException(); + }); + + assertThrows(ExecutionException.class, () -> Tasks.await(throwingTask)); + + String hiro = + "When you are wrestling for possession of a sword, the man with the handle always wins."; + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(hiro)); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(hiro); + } + + @Test + public void submitRunnable() throws Exception { + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + // A Runnable does not return, so the task evaluates to null. + assertThat(result).isNull(); + } + + @Test + public void submitRunnableThatThrows() { + Task task = + crashlyticsWorker.submit( + (Runnable) + () -> { + throw new RuntimeException("I threw in the runnable"); + }); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the runnable"); + } + + @Test + public void submitRunnableThatThrowsThenReturns() throws Exception { + Task thowingTask = + crashlyticsWorker.submit( + (Runnable) + () -> { + throw new IllegalArgumentException(); + }); + + assertThrows(ExecutionException.class, () -> Tasks.await(thowingTask)); + + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitTaskThatReturns() throws Exception { + String skippy = "Think of the problem as an enemy, and defeat them in detail."; + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(skippy)); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(skippy); + } + + @Test + public void submitTaskThatReturnsNull() throws Exception { + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(null)); + + String result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitTaskThatThrows() { + Task task = + crashlyticsWorker.submitTask( + () -> Tasks.forException(new Exception("Thrown from a task."))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Thrown from a task."); + } + + @Test + public void submitTaskThatThrowsThenReturns() throws Exception { + crashlyticsWorker.submitTask(() -> Tasks.forException(new IllegalStateException())); + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("The Hail Mary")); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("The Hail Mary"); + } + + @Test + public void submitTaskThatCancels() { + Task task = crashlyticsWorker.submitTask(Tasks::forCanceled); + + CancellationException thrown = + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + assertThat(task.isCanceled()).isTrue(); + assertThat(thrown).hasMessageThat().contains("Task is already canceled"); + } + + @Test + public void submitTaskThatCancelsThenReturns() throws Exception { + crashlyticsWorker.submitTask(Tasks::forCanceled); + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Flying Dutchman")); + + String result = Tasks.await(task); + + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isEqualTo("Flying Dutchman"); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenReturns() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit another task. + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Valkyrie")); + + String result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isEqualTo("Valkyrie"); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenCallable() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit a simple callable. + Task task = crashlyticsWorker.submit(() -> true); + + boolean result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isTrue(); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenRunnable() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit an empty runnable. + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isNull(); + } + + @Test + public void submitTaskFromAnotherWorker() throws Exception { + Task otherTask = + new CrashlyticsWorker(TestOnlyExecutors.blocking()) + .submit(() -> "Dog's fine. Just sleeping."); + + // This will not use a background thread while waiting for the task on blocking thread. + Task task = crashlyticsWorker.submitTask(() -> otherTask); + + String result = Tasks.await(task); + assertThat(result).isEqualTo("Dog's fine. Just sleeping."); + } + + @Test + public void submitTaskFromAnotherWorkerThatThrows() throws Exception { + Task otherTask = + new CrashlyticsWorker(TestOnlyExecutors.blocking()) + .submitTask(() -> Tasks.forException(new IndexOutOfBoundsException())); + + // Await on the throwing task to force the exception to propagate threw the local worker. + Task task = crashlyticsWorker.submitTask(() -> otherTask); + assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + // Submit another task to local worker to verify the chain did not break. + Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5f375a86)); + + int localResult = Tasks.await(localTask); + + assertThat(otherTask.isSuccessful()).isFalse(); + assertThat(localTask.isSuccessful()).isTrue(); + assertThat(localResult).isEqualTo(0x5f375a86); + } + + @Test + public void submitTaskFromAnotherWorkerThatCancels() throws Exception { + Task otherCancelled = + new CrashlyticsWorker(TestOnlyExecutors.blocking()).submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate threw the local worker. + Task task = crashlyticsWorker.submitTask(() -> otherCancelled); + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Submit another task to local worker to verify the chain did not break. + Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5fe6eb50c7b537a9L)); + + long localResult = Tasks.await(localTask); + + assertThat(otherCancelled.isCanceled()).isTrue(); + assertThat(localTask.isCanceled()).isFalse(); + assertThat(localResult).isEqualTo(0x5fe6eb50c7b537a9L); + } + + @Test + public void submitTaskFromAnotherWorkerDoesNotUseLocalThreads() throws Exception { + // Setup a "local" worker. + ThreadPoolExecutor localExecutor = (ThreadPoolExecutor) Executors.newFixedThreadPool(4); + CrashlyticsWorker localWorker = new CrashlyticsWorker(localExecutor); + + // Use a task off crashlyticsWorker to represent an other task. + Task otherTask = + crashlyticsWorker.submit( + () -> { + sleep(30); + return localExecutor.getActiveCount(); + }); + + // No active threads yet. + assertThat(localExecutor.getActiveCount()).isEqualTo(0); + + // 1 active thread when doing a local task. + assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); + + // 0 active local threads when waiting for other task. + // Waiting for a task from another worker does not block a local thread. + assertThat(Tasks.await(localWorker.submitTask(() -> otherTask))).isEqualTo(0); + + // 1 active thread when doing a task. + assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); + + // No active threads after. + assertThat(localExecutor.getActiveCount()).isEqualTo(0); + } + + @Test + public void submitTaskWhenThreadPoolFull() { + // Fill the underlying executor thread pool. + for (int i = 0; i < 10; i++) { + crashlyticsWorker.execute(() -> sleep(40)); + } + + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(42)); + + // The underlying thread pool is full with tasks that will take longer than this timeout. + assertThrows(TimeoutException.class, () -> Tasks.await(task, 30, TimeUnit.MILLISECONDS)); + } + + @Test + public void submitTaskThatReturnsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forResult(1337), + task -> Tasks.forResult(Integer.toString(task.getResult()))); + + assertThat(Tasks.await(result)).isEqualTo("1337"); + } + + @Test + public void submitTaskThatThrowsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forException(new IndexOutOfBoundsException("Sometimes we look too far.")), + task -> { + if (task.getException() != null) { + return Tasks.forResult("Task threw."); + } + return Tasks.forResult("I dunno how I got here?"); + }); + + assertThat(Tasks.await(result)).isEqualTo("Task threw."); + } + + @Test + public void submitTaskWithContinuationThatThrows() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forResult(7), task -> Tasks.forException(new IOException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(result)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IOException.class); + + // Verify the worker still executes tasks after the continuation threw. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskThatCancelsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + Tasks::forCanceled, + task -> Tasks.forResult(task.isCanceled() ? "Task cancelled." : "What?")); + + assertThat(Tasks.await(result)).isEqualTo("Task cancelled."); + } + + @Test + public void submitTaskWithContinuationThatCancels() throws Exception { + Task result = + crashlyticsWorker.submitTask(() -> Tasks.forResult(7), task -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(result)); + + // Verify the worker still executes tasks after the continuation was cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> "jk"))).isEqualTo("jk"); + } + + @Test + public void submitTaskOnSuccess() throws Exception { + TaskCompletionSource waitingSource = new TaskCompletionSource<>(); + Task waitingTask = waitingSource.getTask(); + + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> waitingTask, + integerResult -> { + // This gets called with the result when the waiting task resolves successfully. + return Tasks.forResult(integerResult + " Success!"); + }); + + waitingSource.trySetResult(1337); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("1337 Success!"); + } + + @Test + public void submitTaskThatReturnsWithSuccessContinuation() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(1337), integer -> Tasks.forResult(Integer.toString(integer))); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("1337"); + } + + @Test + public void submitTaskThatThrowsWithSuccessContinuation() { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forException(new IndexOutOfBoundsException()), + object -> Tasks.forResult("Still you don't believe.")); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IndexOutOfBoundsException.class); + } + + @Test + public void submitTaskWithSuccessContinuationThatThrows() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(7), integer -> Tasks.forException(new IOException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IOException.class); + + // Verify the worker still executes tasks after the success continuation threw. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskThatCancelsWithSuccessContinuation() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + Tasks::forCanceled, object -> Tasks.forResult("Will set you free")); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the worker still executes tasks after the task cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskWithSuccessContinuationThatCancels() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(7), integer -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the worker still executes tasks after the success continuation was cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> "jk"))).isEqualTo("jk"); + } + + @Test + public void submitTaskWithContinuationExecutesInOrder() throws Exception { + // The integers added to the list represent the order they should be executed in. + Queue list = new ConcurrentLinkedQueue<>(); + + // Start the chain which adds 1, then kicks off tasks to add 6 & 7 later, but adds 2 before + // executing the newly added tasks in the continuation. + crashlyticsWorker.submitTask( + () -> { + list.add(1); + + // Sleep to give time for the tasks 3, 4, 5, 6 to be submitted. + sleep(3); + + // We added the 1 and will add 2 in the continuation. And 3, 4, 5, 6 have been submitted. + crashlyticsWorker.submit(() -> list.add(7)); + crashlyticsWorker.submit(() -> list.add(8)); + + return Tasks.forResult(1); + }, + task -> { + // When the task 1 completes the next number to add is 2. Because all the other tasks + // are just submitted, not executed yet. + list.add(2); + return Tasks.forResult("a"); + }); + + // Submit task to add 3 since we just added 1 and know a continuation will add the 2. + crashlyticsWorker.submit(() -> list.add(3)); + + // Submit a waiting task that blocks adding 4 and 5 so we can kick it off later. + TaskCompletionSource waitingSource = new TaskCompletionSource<>(); + Task waitingTask = waitingSource.getTask(); + + crashlyticsWorker.submitTask( + () -> + waitingTask + .continueWith(crashlyticsWorker, task -> list.add(4)) + .continueWith(crashlyticsWorker, task -> list.add(5))); + + // Submit task to add 6 after the waiting continuations to add 4, 5. + crashlyticsWorker.submit(() -> list.add(6)); + + // Kick off the waiting task to add 4, 5 now that 6 is queued up. + waitingSource.trySetResult(null); + + crashlyticsWorker.await(); + + // Verify the list is complete and in order. + assertThat(list).isInOrder(); + assertThat(list).hasSize(8); + } + + @Test + public void tasksRunOnCorrectThreads() throws Exception { + ExecutorService executor = newNamedSingleThreadExecutor("workerThread"); + CrashlyticsWorker worker = new CrashlyticsWorker(executor); + + ExecutorService otherExecutor = newNamedSingleThreadExecutor("otherThread"); + CrashlyticsWorker otherWorker = new CrashlyticsWorker(otherExecutor); + + // Submit a Runnable. + worker.submit( + () -> { + // The runnable blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + }); + + // Submit a Callable. + worker.submit( + () -> { + // The callable blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return null; + }); + + // Submit a Callable. + worker.submitTask( + () -> { + // The callable itself blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return otherWorker.submit( + () -> { + // The called task blocks an underlying thread in its own executor. + assertThat(getThreadName()).isEqualTo("otherThread"); + }); + }); + + // Submit a Callable with a Continuation. + worker.submitTask( + () -> { + // The callable itself blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return otherWorker.submitTask( + () -> { + // The called task blocks an underlying thread in its own executor. + assertThat(getThreadName()).isEqualTo("otherThread"); + return Tasks.forResult(null); + }); + }, + task -> { + // The continuation blocks an underlying thread of the original worker. + assertThat(getThreadName()).isEqualTo("workerThread"); + return Tasks.forResult(null); + }); + + // Await on the worker to force all the tasks to run their assertions. + worker.await(); + } + + @Test + public void executeContinuationOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Hello!") + .continueWith(crashlyticsWorker, greetingTask -> getThreadName()); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeContinuationInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Hello!") + .continueWith(crashlyticsWorker, greetingTask -> getThreadName())); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Ahoy-hoy!") + .onSuccessTask(crashlyticsWorker, greeting -> Tasks.forResult(getThreadName())); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Ahoy-hoy!") + .onSuccessTask( + crashlyticsWorker, greeting -> Tasks.forResult(getThreadName()))); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationOnExceptionOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submitTask(() -> Tasks.forException(new IllegalStateException())) + .onSuccessTask(crashlyticsWorker, greeting -> Tasks.forResult(getThreadName())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(IllegalStateException.class); + + // Verify the chain did not break by adding the on success to a failed task. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeSuccessContinuationOnExceptionInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forException(new IllegalStateException()) + .onSuccessTask( + crashlyticsWorker, greeting -> Tasks.forResult(getThreadName()))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(IllegalStateException.class); + + // Verify the chain did not break by adding the on success to a failed task. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationThatThrowsOnWorker() { + Task task = + crashlyticsWorker + .submit(() -> "Aloha") + .continueWithTask( + crashlyticsWorker, greeting -> Tasks.forException(new ArithmeticException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void executeContinuationThatThrowsInsideWorker() { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Aloha") + .continueWithTask( + crashlyticsWorker, + greeting -> Tasks.forException(new ArithmeticException()))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void executeContinuationThatCancelsOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Aloha") + .continueWithTask(crashlyticsWorker, greeting -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the chain did not break by the continuation cancelling. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationThatCancelsInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Aloha") + .continueWithTask(crashlyticsWorker, greeting -> Tasks.forCanceled())); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the chain did not break by the continuation cancelling. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationsOnOrInsideWorkerDoesNotDeadlock() throws Exception { + // Create a single thread worker to catch potential deadlocks. + CrashlyticsWorker worker = new CrashlyticsWorker(newNamedSingleThreadExecutor("single")); + + // Create a waiting task so we can queue up stuff before executing it. + TaskCompletionSource waiting = new TaskCompletionSource<>(); + + Task task = + worker + .submitTask( + () -> waiting.getTask().continueWith(worker, greetingTask -> getThreadName())) + .continueWith(worker, insideTask -> insideTask.getResult() + "-" + getThreadName()); + + // Kick off the waiting task. + waiting.trySetResult("Howdy!"); + + String result = Tasks.await(task); + + // Verify both on and inside continuations ran on the worker's underlying executor. + assertThat(result).contains("single-single"); + } + + @Test + public void executeContinuationTasksOnOrInsideWorkerDoesNotDeadlock() throws Exception { + // Create a single thread worker to catch potential deadlocks. + CrashlyticsWorker worker = new CrashlyticsWorker(newNamedSingleThreadExecutor("single")); + + // Create a waiting task so we can queue up stuff before executing it. + TaskCompletionSource waiting = new TaskCompletionSource<>(); + + Task task = + worker + .submitTask( + () -> + waiting + .getTask() + .continueWithTask(worker, greetingTask -> Tasks.forResult(getThreadName()))) + .continueWithTask( + worker, + insideTask -> Tasks.forResult(insideTask.getResult() + "-" + getThreadName())); + + // Kick off the waiting task. + waiting.trySetResult("Howdy!"); + + String result = Tasks.await(task); + + // Verify both on and inside continuations ran on the worker's underlying executor. + assertThat(result).contains("single-single"); + } + + @Test + public void cancelledTaskInMiddleDoesNotBreakChain() throws Exception { + // List to keep track of tasks that successfully executed. + Queue list = new ConcurrentLinkedQueue<>(); + + // Create a waiting task to block execution on the worker until more tasks are queued up. + TaskCompletionSource taskSource = new TaskCompletionSource<>(); + crashlyticsWorker.submitTask(taskSource::getTask); + + // Setup a waiting cancellation, so the task does not know it's cancelled when submitting more. + CancellationTokenSource cancellationSource = new CancellationTokenSource(); + CancellationToken cancellationToken = cancellationSource.getToken(); + + // Submit the first task that will cancel. + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Runnable + crashlyticsWorker.submit( + () -> { + list.add("runnable"); + }); + + // Submit another task that will cancel. + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable + crashlyticsWorker.submit(() -> list.add("callable")); + + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable + crashlyticsWorker.submitTask( + () -> { + list.add("callable task"); + return Tasks.forResult(null); + }); + + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable with a Continuation. + crashlyticsWorker.submitTask( + () -> Tasks.forResult(null), + task -> { + list.add("continuation"); + return Tasks.forResult(null); + }); + + // Trigger the cancellations. + cancellationSource.cancel(); + taskSource.trySetResult("go!"); + + crashlyticsWorker.await(); + + // Verify that all types of tasks executed. + assertThat(list).containsExactly("runnable", "callable", "callable task", "continuation"); + } +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java index db3d6d8a1cc..4a1d399c137 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java @@ -16,8 +16,9 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.io.File; import java.io.IOException; @@ -57,8 +58,8 @@ public class MetaDataStoreTest extends CrashlyticsTestCase { } private FileStore fileStore; - private final CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run); + private CrashlyticsWorkers crashlyticsWorkers; private MetaDataStore storeUnderTest; @Override @@ -66,6 +67,13 @@ public void setUp() throws Exception { super.setUp(); fileStore = new FileStore(getContext()); storeUnderTest = new MetaDataStore(fileStore); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + } + + @Override + public void tearDown() throws Exception { + fileStore.deleteAllCrashlyticsFiles(); } private UserMetadata metadataWithUserId(String sessionId) { @@ -73,113 +81,181 @@ private UserMetadata metadataWithUserId(String sessionId) { } private UserMetadata metadataWithUserId(String sessionId, String userId) { - UserMetadata metadata = new UserMetadata(sessionId, fileStore, worker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, crashlyticsWorkers); metadata.setUserId(userId); return metadata; } - public void testWriteUserData_allFields() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_allFields() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + Thread.sleep(5); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_noFields() { - storeUnderTest.writeUserData( - SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_noFields() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } - public void testWriteUserData_singleField() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_singleField() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_null() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_null() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } - public void testWriteUserData_emptyString() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_emptyString() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals("", userData.getUserId()); } - public void testWriteUserData_unicode() { + @Test + public void testWriteUserData_unicode() throws Exception { storeUnderTest.writeUserData( SESSION_ID_1, metadataWithUserId(SESSION_ID_1, UNICODE).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(UNICODE, userData.getUserId()); } - public void testWriteUserData_escaped() { - storeUnderTest.writeUserData( - SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_escaped() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); + Thread.sleep(10); assertEquals(ESCAPED.trim(), userData.getUserId()); } + @Test public void testWriteUserData_readDifferentSession() { storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } + @Test public void testReadUserData_corruptData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); try (PrintWriter printWriter = new PrintWriter(file)) { printWriter.println("Matt says hi!"); } - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_emptyData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); file.createNewFile(); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_noStoredData() { - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } @Test - public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isFalse(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() + throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isFalse(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isFalse(); + + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); final Map keys = new HashMap() { { @@ -190,34 +266,74 @@ public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { }; userMetadata.setCustomKeys(keys); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isTrue(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readKeyData(SESSION_ID_2)).isEqualTo(keys); } @Test - public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() { + public void testSetSameKeysRaceCondition_preserveLastEntryValue() throws Exception { + final Map keys = + new HashMap() { + { + put(KEY_1, "10000"); + put(KEY_2, "20000"); + } + }; + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); + for (int index = 0; index <= 10000; index++) { + userMetadata.setCustomKey(KEY_1, String.valueOf(index)); + userMetadata.setCustomKey(KEY_2, String.valueOf(index * 2)); + } + crashlyticsWorkers.diskWrite.submit( + () -> { + final Map readKeys = storeUnderTest.readKeyData(SESSION_ID_1); + assertThat(readKeys.get(KEY_1)).isEqualTo("10000"); + assertThat(readKeys.get(KEY_2)).isEqualTo("20000"); + assertEqualMaps(keys, readKeys); + }); + crashlyticsWorkers.diskWrite.await(); + } + + @Test + public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() throws Exception { String userId = "ThemisWang"; - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setUserId(userId); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isTrue(); + + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readUserId(SESSION_ID_2)).isEqualTo(userId); } @Test - public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.updateRolloutsState(ROLLOUTS_STATE); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isTrue(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readRolloutsState(SESSION_ID_2)).isEqualTo(ROLLOUTS_STATE); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java index 3d488d924e0..83f104d9649 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java @@ -25,14 +25,14 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.common.CurrentTimeProvider; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; import com.google.firebase.crashlytics.internal.common.DeliveryMechanism; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.InstallIdProvider; import com.google.firebase.crashlytics.internal.common.InstallIdProvider.InstallIds; -import java.util.concurrent.Executor; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import java.util.concurrent.TimeUnit; import org.json.JSONObject; @@ -57,7 +57,8 @@ public class DefaultSettingsControllerTest extends CrashlyticsTestCase { private SettingsSpiCall mockSettingsSpiCall; private DataCollectionArbiter mockDataCollectionArbiter; - private Executor networkExecutor = ExecutorUtils.buildSingleThreadExecutorService("network"); + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); public DefaultSettingsControllerTest() {} @@ -121,7 +122,7 @@ public void testCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - await(controller.loadSettingsData(networkExecutor)); + await(controller.loadSettingsData(crashlyticsWorkers)); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -139,7 +140,7 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception { when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -153,7 +154,7 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception { mockDataCollectionArbiter, true); - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertNotNull(controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -177,14 +178,13 @@ public void testExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -198,7 +198,7 @@ public void testExpiredCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(crashlyticsWorkers); assertEquals(cachedSettings, controller.getSettingsSync()); assertEquals(cachedSettings, await(controller.getSettingsAsync())); @@ -220,8 +220,7 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings cachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); @@ -236,7 +235,7 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { mockSettingsSpiCall, mockDataCollectionArbiter, false); - controller.loadSettingsData(SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, crashlyticsWorkers); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -256,15 +255,14 @@ public void testSkipCachedSettingsLoad() throws Exception { JSONObject expiredCachedSettingsJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(expiredCachedSettingsJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings expiredCachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(expiredCachedSettingsJson)) .thenReturn(expiredCachedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -279,7 +277,7 @@ public void testSkipCachedSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -315,7 +313,7 @@ public void testLastDitchSettingsLoad() throws Exception { .thenReturn(expiredCachedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -330,7 +328,7 @@ public void testLastDitchSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -350,7 +348,7 @@ public void testNoAvailableSettingsLoad() throws Exception { when(mockCachedSettingsIo.readCachedSettings()).thenReturn(null); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -364,7 +362,7 @@ public void testNoAvailableSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(crashlyticsWorkers); assertNotNull(controller.getSettingsSync()); assertFalse(controller.getSettingsAsync().isComplete()); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java index 7131df8c8a4..9db2fb9f0ee 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.*; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CommonUtils; @@ -29,6 +30,7 @@ import java.io.InputStream; import java.net.HttpURLConnection; import java.util.Map; +import java.util.concurrent.Future; public class DefaultSettingsSpiCallTest extends CrashlyticsTestCase { @@ -83,7 +85,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); @@ -128,7 +131,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java index 9051d22258f..b792c51784c 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java @@ -14,8 +14,6 @@ package com.google.firebase.crashlytics; -import static com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.ASSERT; - import com.google.firebase.FirebaseApp; import com.google.firebase.analytics.connector.AnalyticsConnector; import com.google.firebase.annotations.concurrent.Background; @@ -26,8 +24,8 @@ import com.google.firebase.components.Dependency; import com.google.firebase.components.Qualified; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import com.google.firebase.platforminfo.LibraryVersionComponent; import com.google.firebase.remoteconfig.interop.FirebaseRemoteConfigInterop; @@ -69,10 +67,8 @@ public List> getComponents() { } private FirebaseCrashlytics buildCrashlytics(ComponentContainer container) { - // TODO(mrober): Make this a build time configuration. Do not release like this. - CrashlyticsPreconditions.setStrictLevel(ASSERT); // Kill the process on violation for debugging. + CrashlyticsWorkers.setEnforcement(BuildConfig.DEBUG); - // CrashlyticsPreconditions.checkMainThread(); long startTime = System.currentTimeMillis(); FirebaseCrashlytics crashlytics = @@ -86,8 +82,8 @@ private FirebaseCrashlytics buildCrashlytics(ComponentContainer container) { container.get(blockingExecutorService)); long duration = System.currentTimeMillis() - startTime; - if (duration > 30) { - Logger.getLogger().i("Initializing Crashlytics blocked main for " + duration + " ms"); + if (duration > 16) { + Logger.getLogger().d("Initializing Crashlytics blocked main for " + duration + " ms"); } return crashlytics; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java index 9b0d61cd0f6..d305eb4a868 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java @@ -19,12 +19,9 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import com.google.android.gms.tasks.Continuation; import com.google.android.gms.tasks.Task; import com.google.firebase.FirebaseApp; import com.google.firebase.analytics.connector.AnalyticsConnector; -import com.google.firebase.annotations.concurrent.Background; -import com.google.firebase.annotations.concurrent.Blocking; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; @@ -36,8 +33,8 @@ import com.google.firebase.crashlytics.internal.common.CrashlyticsAppQualitySessionsSubscriber; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.IdManager; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; import com.google.firebase.crashlytics.internal.settings.SettingsController; @@ -46,7 +43,6 @@ import com.google.firebase.remoteconfig.interop.FirebaseRemoteConfigInterop; import com.google.firebase.sessions.api.FirebaseSessionsDependencies; import java.util.List; -import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; /** @@ -70,8 +66,8 @@ public class FirebaseCrashlytics { @NonNull Deferred nativeComponent, @NonNull Deferred analyticsConnector, @NonNull Deferred remoteConfigInteropDeferred, - @Background ExecutorService backgroundExecutorService, - @Blocking ExecutorService blockingExecutorService) { + ExecutorService backgroundExecutorService, + ExecutorService blockingExecutorService) { Context context = app.getApplicationContext(); final String appIdentifier = context.getPackageName(); @@ -82,6 +78,9 @@ public class FirebaseCrashlytics { + " for " + appIdentifier); + CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(backgroundExecutorService, blockingExecutorService); + FileStore fileStore = new FileStore(context); final DataCollectionArbiter arbiter = new DataCollectionArbiter(app); final IdManager idManager = @@ -93,9 +92,6 @@ public class FirebaseCrashlytics { final AnalyticsDeferredProxy analyticsDeferredProxy = new AnalyticsDeferredProxy(analyticsConnector); - final ExecutorService crashHandlerExecutor = - ExecutorUtils.buildSingleThreadExecutorService("Crashlytics Exception Handler"); - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber = new CrashlyticsAppQualitySessionsSubscriber(arbiter, fileStore); FirebaseSessionsDependencies.register(sessionsSubscriber); @@ -112,9 +108,9 @@ public class FirebaseCrashlytics { analyticsDeferredProxy.getDeferredBreadcrumbSource(), analyticsDeferredProxy.getAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, sessionsSubscriber, - remoteConfigDeferredProxy); + remoteConfigDeferredProxy, + crashlyticsWorkers); final String googleAppId = app.getOptions().getApplicationId(); final String mappingFileId = CommonUtils.getMappingFileId(context); @@ -150,10 +146,7 @@ public class FirebaseCrashlytics { Logger.getLogger().v("Installer package name is: " + appData.installerPackageName); - final Executor threadPoolExecutor = - ExecutorUtils.buildSequentialExecutor(backgroundExecutorService); - - final SettingsController settingsController = + SettingsController settingsController = SettingsController.create( context, googleAppId, @@ -166,18 +159,8 @@ public class FirebaseCrashlytics { // Kick off actually fetching the settings. settingsController - .loadSettingsData(threadPoolExecutor) - .continueWith( - threadPoolExecutor, - new Continuation() { - @Override - public Object then(@NonNull Task task) throws Exception { - if (!task.isSuccessful()) { - Logger.getLogger().e("Error fetching settings.", task.getException()); - } - return null; - } - }); + .loadSettingsData(crashlyticsWorkers) + .addOnFailureListener(ex -> Logger.getLogger().e("Error fetching settings.", ex)); final boolean finishCoreInBackground = core.onPreExecute(appData, settingsController); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt deleted file mode 100644 index 256dd724339..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal - -import android.os.Build -import android.os.Looper -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.ASSERT -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.NONE -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.THROW -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.WARN - -/** - * Convenient preconditions specific for Crashlytics concurrency. - * - * Use GMS Core's [com.google.android.gms.common.internal.Preconditions] for general preconditions. - */ -internal object CrashlyticsPreconditions { - private val threadName - get() = Thread.currentThread().name - - // TODO(mrober): Make this a build time configuration. - @JvmStatic var strictLevel: StrictLevel = NONE - - @JvmStatic - fun checkMainThread() = - checkThread(::isMainThread) { "Must be called on the main thread, was called on $threadName." } - - @JvmStatic - fun checkBlockingThread() = - checkThread(::isBlockingThread) { - "Must be called on a blocking thread, was called on $threadName." - } - - @JvmStatic - fun checkBackgroundThread() = - checkThread(::isBackgroundThread) { - "Must be called on a background thread, was called on $threadName." - } - - private fun isMainThread() = - 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 #") - - // TODO(mrober): Remove the Crashlytics thread when fully migrated to Firebase common threads. - private fun isBackgroundThread() = - threadName.contains("Firebase Background Thread #") || - threadName.contains("Crashlytics Exception Handler") - - private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { - if (strictLevel.level >= WARN.level && !isCorrectThread()) { - Logger.getLogger().w(failureMessage()) - assert(strictLevel.level < ASSERT.level, failureMessage) - check(strictLevel.level < THROW.level, failureMessage) - } - } - - enum class StrictLevel(val level: Int) : Comparable { - /** Do not check for violations. */ - NONE(0), - /** Log violations as warnings. */ - WARN(1), - /** Throw an exception on violation. */ - THROW(2), - /** Kill the process on violation. Useful for debugging. */ - ASSERT(3), - } -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java deleted file mode 100644 index 876f018fc95..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java +++ /dev/null @@ -1,128 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal; - -import androidx.annotation.VisibleForTesting; -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; - -/** - * Helper for executing tasks sequentially on the given executor service. - * - *

Work on the queue may block, or it may return a Task, such that the underlying thread may be - * re-used while the worker queue is still blocked. - * - *

Work enqueued on this worker will be run serially, regardless of the underlying executor. - * Therefore, workers on the queue should not add new work to the queue and then block on it, as - * that would create a deadlock. In such a case, the worker can return a Task that depends on the - * future work, and run the future work on the executor's thread, but not put it in the queue as its - * own worker. - * - * @hide - */ -public class CrashlyticsWorker { - private final ExecutorService executor; - - private final Object tailLock = new Object(); - private Task tail = Tasks.forResult(null); - - public CrashlyticsWorker(ExecutorService executor) { - this.executor = executor; - } - - /** Returns the executor used by this worker. */ - public ExecutorService getExecutor() { - return executor; - } - - /** - * Submits a Callable task for asynchronous execution on the executor. - * - *

Returns a Task which will be resolved upon successful completion of the - * callable, or throws an ExecutionException if the callable throws an exception. - */ - public Task submit(Callable callable) { - synchronized (tailLock) { - // Do not propagate a cancellation. - if (tail.isCanceled()) { - tail = tail.continueWithTask(executor, task -> Tasks.forResult(null)); - } - // Chain the new callable onto the queue's tail. - Task result = tail.continueWith(executor, task -> callable.call()); - tail = result; - return result; - } - } - - /** - * Submits a Runnable task for asynchronous execution on the executor. - * - *

Returns a Task which will be resolved with null upon successful completion of - * the runnable, or throws an ExecutionException if the runnable throws an exception. - */ - public Task submit(Runnable runnable) { - synchronized (tailLock) { - // Do not propagate a cancellation. - if (tail.isCanceled()) { - tail = tail.continueWithTask(executor, task -> Tasks.forResult(null)); - } - // Chain the new runnable onto the queue's tail. - Task result = - tail.continueWith( - executor, - task -> { - runnable.run(); - return null; - }); - tail = result; - return result; - } - } - - /** - * Submits a Callable Task for asynchronous execution on the executor. - * - *

This is useful for making the worker block on an asynchronous operation, while letting the - * underlying threads be re-used. - * - *

Returns a Task which will be resolved upon successful completion of the Task - * returned by the callable, throws an ExecutionException if the callable throws an - * exception, or throws a CancellationException if the task is cancelled. - */ - public Task submitTask(Callable> callable) { - synchronized (tailLock) { - // Chain the new callable task onto the queue's tail, regardless of cancellation. - Task result = tail.continueWithTask(executor, task -> callable.call()); - tail = result; - return result; - } - } - - /** - * Blocks until all current pending tasks have completed. - * - *

This is not a shutdown, this does not stop new tasks from being submitted to the queue. - */ - @VisibleForTesting - public void await() throws ExecutionException, InterruptedException { - // Submit an empty runnable, and await on it. - Tasks.await(submit(() -> {})); - } -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java index 33369524b2c..b29863f66c5 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java @@ -114,7 +114,9 @@ enum Architecture { matcher.put("x86", X86_32); } - /** @Return {@link CommonUtils.Architecture} enum based on @param String */ + /** + * @Return {@link CommonUtils.Architecture} enum based on @param String + */ static Architecture getValue() { String arch = Build.CPU_ABI; @@ -248,7 +250,9 @@ public static boolean getProximitySensorEnabled(Context context) { } } - /** @deprecated This method will now always return false. It should not be used. */ + /** + * @deprecated This method will now always return false. It should not be used. + */ @Deprecated public static boolean isLoggingEnabled(Context context) { return false; @@ -532,7 +536,9 @@ public static void closeQuietly(Closeable closeable) { } } - /** @return if the given permission is granted */ + /** + * @return if the given permission is granted + */ public static boolean checkPermission(Context context, String permission) { final int res = context.checkCallingOrSelfPermission(permission); return (res == PackageManager.PERMISSION_GRANTED); @@ -556,7 +562,9 @@ public static boolean canTryConnection(Context context) { } } - /** @return true if s1.equals(s2), or if both are null. */ + /** + * @return true if s1.equals(s2), or if both are null. + */ public static boolean nullSafeEquals(@Nullable String s1, @Nullable String s2) { // :TODO: replace calls to this method with Objects.equals(...) when minSdkVersion is 19+ if (s1 == null) { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java deleted file mode 100644 index f5812bb6a4c..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java +++ /dev/null @@ -1,166 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.crashlytics.internal.common; - -import androidx.annotation.NonNull; -import com.google.android.gms.tasks.Continuation; -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import java.util.concurrent.Callable; -import java.util.concurrent.Executor; - -/** - * Helper for executing tasks on the Crashlytics background executor service. - * - *

Work on the queue may block, or it may return a Task, such that the underlying thread may be - * re-used while the worker queue is still blocked. - * - *

Work enqueued on this worker will be run serially, regardless of the underlying executor. - * Therefore, workers on the queue should not add new work to the queue and then block on it, as - * that would create a deadlock. In such a case, the worker can return a Task that depends on the - * future work, and run the future work on the executor's thread, but not put it in the queue as its - * own worker. - * - * @deprecated Use the generic CrashlyticsWorker instead. - */ -@Deprecated -public class CrashlyticsBackgroundWorker { - // TODO(mrober): Clean this up after moving everything to the generic CrashlyticsWorker. - private final Executor executor; - - private Task tail = Tasks.forResult(null); - - private final Object tailLock = new Object(); - - // A thread local to keep track of which thread belongs to this executor. - private final ThreadLocal isExecutorThread = new ThreadLocal<>(); - - public CrashlyticsBackgroundWorker(Executor executor) { - this.executor = executor; - // Queue up the first job as one that marks the thread so we can check it later. - executor.execute( - new Runnable() { - @Override - public void run() { - isExecutorThread.set(true); - } - }); - } - - /** Returns the executor used by this background worker. */ - public Executor getExecutor() { - return executor; - } - - /** Returns true if called on the thread owned by this background worker. */ - private boolean isRunningOnThread() { - return Boolean.TRUE.equals(isExecutorThread.get()); - } - - /** - * Throws an exception if called from any thread other than the background worker's. This helps - * guarantee code is being called on the intended thread. - */ - public void checkRunningOnThread() { - if (!isRunningOnThread()) { - throw new IllegalStateException("Not running on background worker thread as intended."); - } - } - - /** - * Submit a Runnable task for asynchronous execution on the Crashlytics background - * executor service. - * - *

If the runnable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved with null upon successful completion of the - * runnable, or null if the runnable is rejected from the background executor - * service. - */ - Task submit(final Runnable runnable) { - return submit( - new Callable() { - @Override - public Void call() throws Exception { - runnable.run(); - return null; - } - }); - } - - /** Convenience method that creates a new Continuation that wraps the given callable. */ - private Continuation newContinuation(Callable callable) { - return new Continuation() { - @Override - public T then(@NonNull Task task) throws Exception { - // We can ignore the task passed in, which is just the queue's tail. - return callable.call(); - } - }; - } - - /** Convenience method that tasks a Task and convert it to a Task. */ - private Task ignoreResult(Task task) { - return task.continueWith( - executor, - new Continuation() { - @Override - public Void then(@NonNull Task task) throws Exception { - // Ignore whether the task succeeded or failed. - return null; - } - }); - } - - /** - * Submit a Callable task for asynchronous execution on the Crashlytics background - * executor service. - * - *

If the callable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved upon successful completion of the callable. - */ - public Task submit(final Callable callable) { - synchronized (tailLock) { - // Chain the new callable onto the queue's tail. - Task toReturn = tail.continueWith(executor, newContinuation(callable)); - - // Add a new tail that swallows errors from the callable when it finishes. - tail = ignoreResult(toReturn); - return toReturn; - } - } - - /** - * Submit a Callable task for asynchronous execution on the Crashlytics background - * executor service. This method is useful for making the worker block on an asynchronous - * operation, while letting the underlying thread be re-used. - * - *

If the callable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved upon successful completion of the Task - * returns by the callable. - */ - public Task submitTask(final Callable> callable) { - synchronized (tailLock) { - // Chain the new callable onto the queue's tail. - Task toReturn = tail.continueWithTask(executor, newContinuation(callable)); - - // Add a new tail that swallows errors from the callable when it finishes. - tail = ignoreResult(toReturn); - return toReturn; - } - } -} 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 e26cdee31b9..5d60f93a9c8 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -33,6 +33,8 @@ import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.NativeSessionFileProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsTasks; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; @@ -51,7 +53,6 @@ import java.util.Map; import java.util.SortedSet; import java.util.concurrent.Callable; -import java.util.concurrent.Executor; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeoutException; @@ -67,8 +68,6 @@ class CrashlyticsController { static final FilenameFilter APP_EXCEPTION_MARKER_FILTER = (directory, filename) -> filename.startsWith(APP_EXCEPTION_MARKER_PREFIX); - static final String NATIVE_SESSION_DIR = "native-sessions"; - static final int FIREBASE_CRASH_TYPE_FATAL = 1; private static final String GENERATOR_FORMAT = "Crashlytics Android SDK/%s"; @@ -82,7 +81,7 @@ class CrashlyticsController { private final CrashlyticsFileMarker crashMarker; private final UserMetadata userMetadata; - private final CrashlyticsBackgroundWorker backgroundWorker; + private final CrashlyticsWorkers crashlyticsWorkers; private final IdManager idManager; private final FileStore fileStore; @@ -116,7 +115,6 @@ class CrashlyticsController { CrashlyticsController( Context context, - CrashlyticsBackgroundWorker backgroundWorker, IdManager idManager, DataCollectionArbiter dataCollectionArbiter, FileStore fileStore, @@ -127,9 +125,9 @@ class CrashlyticsController { SessionReportingCoordinator sessionReportingCoordinator, CrashlyticsNativeComponent nativeComponent, AnalyticsEventLogger analyticsEventLogger, - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) { + CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, + CrashlyticsWorkers crashlyticsWorkers) { this.context = context; - this.backgroundWorker = backgroundWorker; this.idManager = idManager; this.dataCollectionArbiter = dataCollectionArbiter; this.fileStore = fileStore; @@ -141,10 +139,7 @@ class CrashlyticsController { this.analyticsEventLogger = analyticsEventLogger; this.sessionsSubscriber = sessionsSubscriber; this.reportingCoordinator = sessionReportingCoordinator; - } - - private Context getContext() { - return context; + this.crashlyticsWorkers = crashlyticsWorkers; } // region Exception handling @@ -195,7 +190,7 @@ synchronized void handleUncaughtException( final long timestampMillis = System.currentTimeMillis(); final Task handleUncaughtExceptionTask = - backgroundWorker.submitTask( + crashlyticsWorkers.common.submitTask( new Callable>() { @Override public Task call() throws Exception { @@ -210,7 +205,6 @@ public Task call() throws Exception { // We've fatally crashed, so write the marker file that indicates a crash occurred. crashMarker.create(); - reportingCoordinator.persistFatalEvent( ex, thread, currentSessionId, timestampSeconds); @@ -224,12 +218,10 @@ public Task call() throws Exception { return Tasks.forResult(null); } - Executor executor = backgroundWorker.getExecutor(); - return settingsProvider .getSettingsAsync() .onSuccessTask( - executor, + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override @@ -244,7 +236,8 @@ public Task then(@Nullable Settings settings) throws Exception { return Tasks.whenAll( logAnalyticsAppExceptionEvents(), reportingCoordinator.sendReports( - executor, isOnDemand ? currentSessionId : null)); + crashlyticsWorkers.common, + isOnDemand ? currentSessionId : null)); } }); } @@ -303,11 +296,12 @@ public Task then(@Nullable Void aVoid) throws Exception { Logger.getLogger().d("Waiting for send/deleteUnsentReports to be called."); // Wait for either the processReports callback to be called, or data collection to be enabled. - return Utils.race(collectionEnabled, reportActionProvided.getTask()); + return CrashlyticsTasks.race(collectionEnabled, reportActionProvided.getTask()); } /** This function must be called before opening the first session * */ boolean didCrashOnPreviousExecution() { + CrashlyticsWorkers.checkBackgroundThread(); // To not violate strict mode. if (!crashMarker.isPresent()) { // Before the first session of this execution is opened, the current session ID still refers // to the previous execution's last session, which is what we want. @@ -362,14 +356,17 @@ Task submitAllReports(Task settingsDataTask) { @Override public Task then(@Nullable Boolean send) throws Exception { - return backgroundWorker.submitTask( + return crashlyticsWorkers.common.submitTask( new Callable>() { @Override public Task call() throws Exception { if (!send) { Logger.getLogger().v("Deleting cached crash reports..."); - deleteFiles(listAppExceptionMarkerFiles()); - reportingCoordinator.removeAllReports(); + crashlyticsWorkers.diskWrite.submit( + () -> { + deleteFiles(listAppExceptionMarkerFiles()); + reportingCoordinator.removeAllReports(); + }); unsentReportsHandled.trySetResult(null); return Tasks.forResult(null); } @@ -383,10 +380,8 @@ public Task call() throws Exception { // permission. dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); - Executor executor = backgroundWorker.getExecutor(); - return settingsDataTask.onSuccessTask( - executor, + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override @@ -399,7 +394,7 @@ public Task then(@Nullable Settings appSettingsData) return Tasks.forResult(null); } logAnalyticsAppExceptionEvents(); - reportingCoordinator.sendReports(executor); + reportingCoordinator.sendReports(crashlyticsWorkers.common); unsentReportsHandled.trySetResult(null); return Tasks.forResult(null); @@ -415,16 +410,9 @@ public Task then(@Nullable Settings appSettingsData) /** Log a timestamped string to the log file. */ void writeToLog(final long timestamp, final String msg) { - backgroundWorker.submit( - new Callable() { - @Override - public Void call() throws Exception { - if (!isHandlingException()) { - logFileManager.writeToLog(timestamp, msg); - } - return null; - } - }); + if (!isHandlingException()) { + logFileManager.writeToLog(timestamp, msg); + } } /** Log a caught exception - write out Throwable as event section of protobuf */ @@ -433,23 +421,15 @@ void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwab // rather than the time at which the task executes. final long timestampMillis = System.currentTimeMillis(); - backgroundWorker.submit( - new Runnable() { - @Override - public void run() { - if (!isHandlingException()) { - long timestampSeconds = getTimestampSeconds(timestampMillis); - final String currentSessionId = getCurrentSessionId(); - if (currentSessionId == null) { - Logger.getLogger() - .w("Tried to write a non-fatal exception while no session was open."); - return; - } - reportingCoordinator.persistNonFatalEvent( - ex, thread, currentSessionId, timestampSeconds); - } - } - }); + if (!isHandlingException()) { + long timestampSeconds = getTimestampSeconds(timestampMillis); + final String currentSessionId = getCurrentSessionId(); + if (currentSessionId == null) { + Logger.getLogger().w("Tried to write a non-fatal exception while no session was open."); + return; + } + reportingCoordinator.persistNonFatalEvent(ex, thread, currentSessionId, timestampSeconds); + } } void logFatalException(Thread thread, Throwable ex) { @@ -498,14 +478,8 @@ void setInternalKey(String key, String value) { /** Open a new session on the single-threaded executor. */ void openSession(String sessionIdentifier) { - backgroundWorker.submit( - new Callable() { - @Override - public Void call() throws Exception { - doOpenSession(sessionIdentifier, /* isOnDemand= */ false); - return null; - } - }); + crashlyticsWorkers.common.submit( + () -> doOpenSession(sessionIdentifier, /* isOnDemand= */ false)); } /** @@ -531,7 +505,7 @@ private String getCurrentSessionId() { * @param settingsProvider */ boolean finalizeSessions(SettingsProvider settingsProvider) { - backgroundWorker.checkRunningOnThread(); + CrashlyticsWorkers.checkBackgroundThread(); if (isHandlingException()) { Logger.getLogger().w("Skipping session finalization because a crash has already occurred."); @@ -540,7 +514,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) { Logger.getLogger().v("Finalizing previously open sessions."); try { - doCloseSessions(true, settingsProvider); + doCloseSessions(true, settingsProvider, true); } catch (Exception e) { Logger.getLogger().e("Unable to finalize previously open sessions.", e); return false; @@ -585,15 +559,19 @@ private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) { reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds); } + // This is only used for exception handler close session (we have another close session in + // background initialization) void doCloseSessions(SettingsProvider settingsProvider) { - doCloseSessions(false, settingsProvider); + doCloseSessions(false, settingsProvider, false); } /** - * Not synchronized/locked. Must be executed from the single thread executor service used by this - * class. + * + * Not synchronized/locked. Must be executed from the executor service runs tasks in serial order */ - private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settingsProvider) { + private void doCloseSessions( + boolean skipCurrentSession, SettingsProvider settingsProvider, boolean isInitProcess) { + CrashlyticsWorkers.checkBackgroundThread(); final int offset = skipCurrentSession ? 1 : 0; // :TODO HW2021 this implementation can be cleaned up. @@ -607,13 +585,15 @@ private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settin final String mostRecentSessionIdToClose = sortedOpenSessions.get(offset); - if (settingsProvider.getSettingsSync().featureFlagData.collectAnrs) { + // We only collect ANR info for finalize report during initialization process + if (isInitProcess && settingsProvider.getSettingsSync().featureFlagData.collectAnrs) { writeApplicationExitInfoEventIfRelevant(mostRecentSessionIdToClose); } else { Logger.getLogger().v("ANR feature disabled."); } - if (nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) { + // We only collect native crash info for finalize report during initialization process + if (isInitProcess && nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) { // We only finalize the current session if it's a Java crash, so only finalize native crash // data when we aren't including current. finalizePreviousNativeSession(mostRecentSessionIdToClose); @@ -938,7 +918,7 @@ private void writeApplicationExitInfoEventIfRelevant(String sessionId) { if (applicationExitInfoList.size() != 0) { final LogFileManager relevantSessionLogManager = new LogFileManager(fileStore, sessionId); final UserMetadata relevantUserMetadata = - UserMetadata.loadFromExistingSession(sessionId, fileStore, backgroundWorker); + UserMetadata.loadFromExistingSession(sessionId, fileStore, crashlyticsWorkers); reportingCoordinator.persistRelevantAppExitInfoEvent( sessionId, applicationExitInfoList, relevantSessionLogManager, relevantUserMetadata); } else { 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 9e8af2f4898..3214e525390 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 @@ -30,6 +30,7 @@ import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbSource; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.persistence.FileStore; @@ -39,9 +40,7 @@ import com.google.firebase.crashlytics.internal.stacktrace.RemoveRepeatsStrategy; import com.google.firebase.crashlytics.internal.stacktrace.StackTraceTrimmingStrategy; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -93,14 +92,14 @@ public class CrashlyticsCore { @VisibleForTesting public final BreadcrumbSource breadcrumbSource; private final AnalyticsEventLogger analyticsEventLogger; - private final ExecutorService crashHandlerExecutor; - private final CrashlyticsBackgroundWorker backgroundWorker; private final CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber; private final CrashlyticsNativeComponent nativeComponent; private final RemoteConfigDeferredProxy remoteConfigDeferredProxy; + private final CrashlyticsWorkers crashlyticsWorkers; + // region Constructors public CrashlyticsCore( @@ -111,9 +110,9 @@ public CrashlyticsCore( BreadcrumbSource breadcrumbSource, AnalyticsEventLogger analyticsEventLogger, FileStore fileStore, - ExecutorService crashHandlerExecutor, CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, - RemoteConfigDeferredProxy remoteConfigDeferredProxy) { + RemoteConfigDeferredProxy remoteConfigDeferredProxy, + CrashlyticsWorkers crashlyticsWorkers) { this.app = app; this.dataCollectionArbiter = dataCollectionArbiter; this.context = app.getApplicationContext(); @@ -121,11 +120,10 @@ public CrashlyticsCore( this.nativeComponent = nativeComponent; this.breadcrumbSource = breadcrumbSource; this.analyticsEventLogger = analyticsEventLogger; - this.crashHandlerExecutor = crashHandlerExecutor; this.fileStore = fileStore; - this.backgroundWorker = new CrashlyticsBackgroundWorker(crashHandlerExecutor); this.sessionsSubscriber = sessionsSubscriber; this.remoteConfigDeferredProxy = remoteConfigDeferredProxy; + this.crashlyticsWorkers = crashlyticsWorkers; startTime = System.currentTimeMillis(); onDemandCounter = new OnDemandCounter(); @@ -154,7 +152,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) initializationMarker = new CrashlyticsFileMarker(INITIALIZATION_MARKER_FILE_NAME, fileStore); final UserMetadata userMetadata = - new UserMetadata(sessionIdentifier, fileStore, backgroundWorker); + new UserMetadata(sessionIdentifier, fileStore, crashlyticsWorkers); final LogFileManager logFileManager = new LogFileManager(fileStore); final StackTraceTrimmingStrategy stackTraceTrimmingStrategy = new MiddleOutFallbackStrategy( @@ -173,12 +171,12 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) stackTraceTrimmingStrategy, settingsProvider, onDemandCounter, - sessionsSubscriber); + sessionsSubscriber, + crashlyticsWorkers); controller = new CrashlyticsController( context, - backgroundWorker, idManager, dataCollectionArbiter, fileStore, @@ -189,7 +187,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) sessionReportingCoordinator, nativeComponent, analyticsEventLogger, - sessionsSubscriber); + sessionsSubscriber, + crashlyticsWorkers); // If the file is present at this point, then the previous run's initialization // did not complete, and we want to perform initialization synchronously this time. @@ -223,22 +222,16 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) return true; } - /** Performs background initialization asynchronously on the background worker's thread. */ + /** Performs background initialization asynchronously on the common worker. */ @CanIgnoreReturnValue public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { - return Utils.callTask( - crashHandlerExecutor, - new Callable>() { - @Override - public Task call() throws Exception { - return doBackgroundInitialization(settingsProvider); - } - }); + return crashlyticsWorkers.common.submitTask(() -> doBackgroundInitialization(settingsProvider)); } /** Performs background initialization synchronously on the calling thread. */ @CanIgnoreReturnValue private Task doBackgroundInitialization(SettingsProvider settingsProvider) { + CrashlyticsWorkers.checkBackgroundThread(); // create the marker for this run markInitializationStarted(); @@ -328,7 +321,8 @@ public static String getVersion() { * safe to invoke this method from the main thread. */ public void logException(@NonNull Throwable throwable) { - controller.writeNonFatalException(Thread.currentThread(), throwable); + crashlyticsWorkers.common.submit( + () -> controller.writeNonFatalException(Thread.currentThread(), throwable)); } /** @@ -343,11 +337,15 @@ public void logException(@NonNull Throwable throwable) { */ public void log(final String msg) { final long timestamp = System.currentTimeMillis() - startTime; - controller.writeToLog(timestamp, msg); + // queuing up on common worker to maintain the order + crashlyticsWorkers.common.submit( + () -> { + crashlyticsWorkers.diskWrite.submit(() -> controller.writeToLog(timestamp, msg)); + }); } public void setUserId(String identifier) { - controller.setUserId(identifier); + crashlyticsWorkers.common.submit(() -> controller.setUserId(identifier)); } /** @@ -360,7 +358,7 @@ public void setUserId(String identifier) { * @throws NullPointerException if key is null. */ public void setCustomKey(String key, String value) { - controller.setCustomKey(key, value); + crashlyticsWorkers.common.submit(() -> controller.setCustomKey(key, value)); } /** @@ -377,7 +375,7 @@ public void setCustomKey(String key, String value) { * @throws NullPointerException if any key in keysAndValues is null. */ public void setCustomKeys(Map keysAndValues) { - controller.setCustomKeys(keysAndValues); + crashlyticsWorkers.common.submit(() -> controller.setCustomKeys(keysAndValues)); } // endregion @@ -397,7 +395,7 @@ public void setCustomKeys(Map keysAndValues) { * @throws NullPointerException if key is null. */ public void setInternalKey(String key, String value) { - controller.setInternalKey(key, value); + crashlyticsWorkers.common.submit(() -> controller.setInternalKey(key, value)); } /** Logs a fatal Throwable on the Crashlytics servers on-demand. */ @@ -406,11 +404,16 @@ public void logFatalException(Throwable throwable) { .d("Recorded on-demand fatal events: " + onDemandCounter.getRecordedOnDemandExceptions()); Logger.getLogger() .d("Dropped on-demand fatal events: " + onDemandCounter.getDroppedOnDemandExceptions()); - controller.setInternalKey( - ON_DEMAND_RECORDED_KEY, Integer.toString(onDemandCounter.getRecordedOnDemandExceptions())); - controller.setInternalKey( - ON_DEMAND_DROPPED_KEY, Integer.toString(onDemandCounter.getDroppedOnDemandExceptions())); - controller.logFatalException(Thread.currentThread(), throwable); + crashlyticsWorkers.common.submit( + () -> { + controller.setInternalKey( + ON_DEMAND_RECORDED_KEY, + Integer.toString(onDemandCounter.getRecordedOnDemandExceptions())); + controller.setInternalKey( + ON_DEMAND_DROPPED_KEY, + Integer.toString(onDemandCounter.getDroppedOnDemandExceptions())); + controller.logFatalException(Thread.currentThread(), throwable); + }); } // endregion @@ -427,7 +430,7 @@ CrashlyticsController getController() { /** * When a startup crash occurs, Crashlytics must lock on the main thread and complete - * initializaiton to upload crash result. 4 seconds is chosen for the lock to prevent ANR + * initialization to upload crash result. 4 seconds is chosen for the lock to prevent ANR */ private void finishInitSynchronously(SettingsProvider settingsProvider) { @@ -439,7 +442,8 @@ public void run() { } }; - final Future future = crashHandlerExecutor.submit(runnable); + // TODO(mrober): Refactor to Tasks. Maybe just re-use async task and awaitEvenIfOnMain? + final Future future = crashlyticsWorkers.common.getExecutor().submit(runnable); Logger.getLogger() .d( @@ -459,7 +463,7 @@ public void run() { /** Synchronous call to mark start of initialization */ void markInitializationStarted() { - backgroundWorker.checkRunningOnThread(); + CrashlyticsWorkers.checkBackgroundThread(); // Create the Crashlytics initialization marker file, which is used to determine // whether the app crashed before initialization could complete. @@ -469,21 +473,18 @@ void markInitializationStarted() { /** Enqueues a job to remove the Crashlytics initialization marker file */ void markInitializationComplete() { - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - try { - final boolean removed = initializationMarker.remove(); - if (!removed) { - Logger.getLogger().w("Initialization marker file was not properly removed."); - } - return removed; - } catch (Exception e) { - Logger.getLogger() - .e("Problem encountered deleting Crashlytics initialization marker.", e); - return false; + 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; } }); } @@ -496,13 +497,7 @@ boolean didPreviousInitializationFail() { private void checkForPreviousCrash() { Task task = - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - return controller.didCrashOnPreviousExecution(); - } - }); + crashlyticsWorkers.common.submit(() -> controller.didCrashOnPreviousExecution()); Boolean result; try { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java deleted file mode 100644 index 3ddc3d3ab6a..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.crashlytics.internal.common; - -import androidx.annotation.NonNull; - -/** This class defines Crashlytics lifecycle events */ -interface CrashlyticsLifecycleEvents { - - /** - * Called when a new session is opened. - * - * @param sessionId the identifier for the new session - */ - void onBeginSession(@NonNull String sessionId, long timestamp); - - /** - * Called when a message is logged by the user. - * - * @param timestamp the timestamp of the message (in milliseconds since app launch) - * @param log the log message - */ - void onLog(long timestamp, String log); - - /** - * Called when a custom key is set by the user. - * - * @param key - * @param value - */ - void onCustomKey(String key, String value); - - /** - * Called when a user ID is set by the user. - * - * @param userId - */ - void onUserId(String userId); -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java index 84493f7b355..157722753fd 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java @@ -24,7 +24,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.FirebaseApp; import com.google.firebase.crashlytics.internal.Logger; -import java.util.concurrent.Executor; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsTasks; // Determines whether automatic data collection is enabled. public class DataCollectionArbiter { @@ -129,11 +129,9 @@ public Task waitForAutomaticDataCollectionEnabled() { * Returns a task which will be resolved when either: 1) automatic data collection has been * enabled, or 2) grantDataCollectionPermission has been called. */ - public Task waitForDataCollectionPermission(Executor executor) { - return Utils.race( - executor, - dataCollectionExplicitlyApproved.getTask(), - waitForAutomaticDataCollectionEnabled()); + public Task waitForDataCollectionPermission() { + return CrashlyticsTasks.race( + dataCollectionExplicitlyApproved.getTask(), waitForAutomaticDataCollectionEnabled()); } /** 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 4018d3c0394..56ff1244439 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java @@ -24,6 +24,7 @@ import com.google.android.gms.tasks.Task; 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.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; @@ -47,10 +48,10 @@ import java.util.concurrent.Executor; /** - * This class handles Crashlytics lifecycle events and coordinates session data capture and - * persistence, as well as sending of reports to Firebase Crashlytics. + * This class coordinates Crashlytics session data capture and persistence, as well as sending of + * reports to Firebase Crashlytics. */ -public class SessionReportingCoordinator implements CrashlyticsLifecycleEvents { +public class SessionReportingCoordinator { private static final String EVENT_TYPE_CRASH = "crash"; private static final String EVENT_TYPE_LOGGED = "error"; @@ -68,7 +69,8 @@ public static SessionReportingCoordinator create( StackTraceTrimmingStrategy stackTraceTrimmingStrategy, SettingsProvider settingsProvider, OnDemandCounter onDemandCounter, - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) { + CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, + CrashlyticsWorkers crashlyticsWorkers) { final CrashlyticsReportDataCapture dataCapture = new CrashlyticsReportDataCapture( context, idManager, appData, stackTraceTrimmingStrategy, settingsProvider); @@ -77,7 +79,13 @@ public static SessionReportingCoordinator create( final DataTransportCrashlyticsReportSender reportSender = DataTransportCrashlyticsReportSender.create(context, settingsProvider, onDemandCounter); return new SessionReportingCoordinator( - dataCapture, reportPersistence, reportSender, logFileManager, userMetadata, idManager); + dataCapture, + reportPersistence, + reportSender, + logFileManager, + userMetadata, + idManager, + crashlyticsWorkers); } private final CrashlyticsReportDataCapture dataCapture; @@ -87,22 +95,25 @@ public static SessionReportingCoordinator create( private final UserMetadata reportMetadata; private final IdManager idManager; + private final CrashlyticsWorkers crashlyticsWorkers; + SessionReportingCoordinator( CrashlyticsReportDataCapture dataCapture, CrashlyticsReportPersistence reportPersistence, DataTransportCrashlyticsReportSender reportsSender, LogFileManager logFileManager, UserMetadata reportMetadata, - IdManager idManager) { + IdManager idManager, + CrashlyticsWorkers crashlyticsWorkers) { this.dataCapture = dataCapture; this.reportPersistence = reportPersistence; this.reportsSender = reportsSender; this.logFileManager = logFileManager; this.reportMetadata = reportMetadata; this.idManager = idManager; + this.crashlyticsWorkers = crashlyticsWorkers; } - @Override public void onBeginSession(@NonNull String sessionId, long timestampSeconds) { final CrashlyticsReport capturedReport = dataCapture.captureReportData(sessionId, timestampSeconds); @@ -110,21 +121,6 @@ public void onBeginSession(@NonNull String sessionId, long timestampSeconds) { reportPersistence.persistReport(capturedReport); } - @Override - public void onLog(long timestamp, String log) { - logFileManager.writeToLog(timestamp, log); - } - - @Override - public void onCustomKey(String key, String value) { - reportMetadata.setCustomKey(key, value); - } - - @Override - public void onUserId(String userId) { - reportMetadata.setUserId(userId); - } - public void persistFatalEvent( @NonNull Throwable event, @NonNull Thread thread, @NonNull String sessionId, long timestamp) { Logger.getLogger().v("Persisting fatal event for session " + sessionId); @@ -326,7 +322,7 @@ private void persistEvent( @NonNull String sessionId, @NonNull String eventType, long timestamp, - boolean includeAllThreads) { + boolean isFatal) { final boolean isHighPriority = eventType.equals(EVENT_TYPE_CRASH); @@ -338,9 +334,19 @@ private void persistEvent( timestamp, EVENT_THREAD_IMPORTANCE, MAX_CHAINED_EXCEPTION_DEPTH, - includeAllThreads); + isFatal); + CrashlyticsReport.Session.Event finallizedEvent = addMetaDataToEvent(capturedEvent); + + // Non-fatal, persistence write task we move to diskWriteWorker + if (!isFatal) { + crashlyticsWorkers.diskWrite.submit( + () -> { + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); + }); + return; + } - reportPersistence.persistEvent(addMetaDataToEvent(capturedEvent), sessionId, isHighPriority); + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); } private boolean onReportSendComplete(@NonNull Task task) { @@ -348,12 +354,16 @@ private boolean onReportSendComplete(@NonNull Task { + File reportFile = report.getReportFile(); + if (reportFile.delete()) { + Logger.getLogger().d("Deleted report file: " + reportFile.getPath()); + } else { + Logger.getLogger() + .w("Crashlytics could not delete report file: " + reportFile.getPath()); + } + }); return true; } Logger.getLogger() diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java index 8e8f19d056a..d224cae38b5 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 @@ -14,16 +14,11 @@ package com.google.firebase.crashlytics.internal.common; -import android.annotation.SuppressLint; import android.os.Looper; -import com.google.android.gms.tasks.Continuation; import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.TaskCompletionSource; import com.google.errorprone.annotations.CanIgnoreReturnValue; -import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -37,71 +32,6 @@ public final class Utils { /** Timeout in milliseconds for blocking on the main thread. Be careful about ANRs. */ private static final int MAIN_TIMEOUT_MILLIS = 2_750; - /** - * @return A tasks that is resolved when either of the given tasks is resolved. - */ - // TODO(b/261014167): Use an explicit executor in continuations. - @SuppressLint("TaskMainThread") - public static Task race(Task t1, Task t2) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - Continuation continuation = - task -> { - if (task.isSuccessful()) { - result.trySetResult(task.getResult()); - } else if (task.getException() != null) { - result.trySetException(task.getException()); - } - return null; - }; - t1.continueWith(continuation); - t2.continueWith(continuation); - return result.getTask(); - } - - /** - * @return A tasks that is resolved when either of the given tasks is resolved. - */ - public static Task race(Executor executor, Task t1, Task t2) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - Continuation continuation = - task -> { - if (task.isSuccessful()) { - result.trySetResult(task.getResult()); - } else if (task.getException() != null) { - result.trySetException(task.getException()); - } - return null; - }; - t1.continueWith(executor, continuation); - t2.continueWith(executor, continuation); - return result.getTask(); - } - - /** Similar to Tasks.call, but takes a Callable that returns a Task. */ - public static Task callTask(Executor executor, Callable> callable) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - executor.execute( - () -> { - try { - callable - .call() - .continueWith( - executor, - task -> { - if (task.isSuccessful()) { - result.setResult(task.getResult()); - } else if (task.getException() != null) { - result.setException(task.getException()); - } - return null; - }); - } catch (Exception e) { - result.setException(e); - } - }); - return result.getTask(); - } - /** * Blocks until the given Task completes, and then returns the value the Task was resolved with, * if successful. If the Task fails, an exception will be thrown, wrapping the Exception of the diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java new file mode 100644 index 00000000000..61b2766197f --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java @@ -0,0 +1,69 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import com.google.android.gms.tasks.CancellationTokenSource; +import com.google.android.gms.tasks.Continuation; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Crashlytics specific utilities for dealing with Tasks. + * + * @hide + */ +public final class CrashlyticsTasks { + /** An Executor that runs on the calling thread. */ + private static final Executor DIRECT = Runnable::run; + + /** + * Returns a task that is resolved when either of the given tasks is resolved. + * + *

If both tasks are cancelled, the returned task will be cancelled. + */ + public static Task race(Task task1, Task task2) { + CancellationTokenSource cancellation = new CancellationTokenSource(); + TaskCompletionSource result = new TaskCompletionSource<>(cancellation.getToken()); + + AtomicBoolean otherTaskCancelled = new AtomicBoolean(false); + + Continuation> continuation = + task -> { + if (task.isSuccessful()) { + // Task is complete and successful. + result.trySetResult(task.getResult()); + } else if (task.getException() != null) { + // Task is complete but unsuccessful. + result.trySetException(task.getException()); + } else if (otherTaskCancelled.getAndSet(true)) { + // Both tasks are cancelled. + cancellation.cancel(); + } + return Tasks.forResult(null); + }; + + task1.continueWithTask(DIRECT, continuation); + task2.continueWithTask(DIRECT, continuation); + + return result.getTask(); + } + + private CrashlyticsTasks() {} +} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java new file mode 100644 index 00000000000..615c52389a1 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java @@ -0,0 +1,212 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import androidx.annotation.Discouraged; +import androidx.annotation.VisibleForTesting; +import com.google.android.gms.tasks.Continuation; +import com.google.android.gms.tasks.SuccessContinuation; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.Tasks; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * Worker for executing tasks sequentially on the given executor service. + * + *

Work on the queue may suspend, or it may return a Task, such that the underlying thread may be + * re-used while the worker queue is suspended. + * + *

Work enqueued on this worker will be run serially, regardless of the underlying executor. + * Therefore, workers on the queue should not add new work to the queue and then block on it, as + * that would create a deadlock. In such a case, the worker can return a Task that depends on the + * future work, and run the future work on the executor's thread, but not put it in the queue as its + * own worker. + * + * @hide + */ +public class CrashlyticsWorker implements Executor { + private final ExecutorService executor; + + private final Object tailLock = new Object(); + private Task tail = Tasks.forResult(null); + + CrashlyticsWorker(ExecutorService executor) { + this.executor = executor; + } + + /** Returns the executor used by this worker. */ + public ExecutorService getExecutor() { + return executor; + } + + /** + * Submits a Callable task for asynchronous execution on the executor. + * + *

A blocking callable will block an underlying thread. + * + *

Returns a Task which will be resolved upon successful completion of the + * callable, or throws an ExecutionException if the callable throws an exception. + */ + @CanIgnoreReturnValue + public Task submit(Callable callable) { + synchronized (tailLock) { + // Chain the new callable onto the queue's tail. + Task result = tail.continueWithTask(executor, task -> Tasks.forResult(callable.call())); + tail = result; + return result; + } + } + + /** + * Submits a Runnable task for asynchronous execution on the executor. + * + *

A blocking runnable will block an underlying thread. + * + *

Returns a Task which will be resolved with null upon successful completion of + * the runnable, or throws an ExecutionException if the runnable throws an exception. + */ + @CanIgnoreReturnValue + public Task submit(Runnable runnable) { + synchronized (tailLock) { + // Chain the new runnable onto the queue's tail. + Task result = + tail.continueWithTask( + executor, + task -> { + runnable.run(); + return Tasks.forResult(null); + }); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task for asynchronous execution on the executor. + * + *

This is useful for making the worker suspend on an asynchronous operation, while letting the + * underlying threads be re-used. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable, throws an ExecutionException if the callable throws an + * exception, or throws a CancellationException if the task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTask(Callable> callable) { + synchronized (tailLock) { + // Chain the new callable task onto the queue's tail. + Task result = tail.continueWithTask(executor, task -> callable.call()); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task followed by a Continuation for asynchronous + * execution on the executor. + * + *

This is useful for submitting a task that must be immediately followed by another task, + * regardless of more tasks being submitted in parallel. For example, settings. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable and continued by the continuation, throws an ExecutionException + * if either task throws an exception, or throws a CancellationException if + * either task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTask( + Callable> callable, Continuation> continuation) { + synchronized (tailLock) { + // Chain the new callable task and continuation onto the queue's tail. + Task result = + tail.continueWithTask(executor, task -> callable.call()) + .continueWithTask(executor, continuation); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task followed by a SuccessContinuation for + * asynchronous execution on the executor. + * + *

This is useful for submitting a task that must be immediately followed by another task, only + * if it was successful, but regardless of more tasks being submitted in parallel. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable and continued by the continuation, throws an ExecutionException + * if either task throws an exception, or throws a CancellationException if + * the task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTaskOnSuccess( + Callable> callable, SuccessContinuation successContinuation) { + synchronized (tailLock) { + // Chain the new callable task and success continuation onto the queue's tail. + Task result = + tail.continueWithTask(executor, task -> callable.call()) + .continueWithTask( + executor, + task -> { + if (task.isSuccessful()) { + return successContinuation.then(task.getResult()); + } else if (task.getException() != null) { + return Tasks.forException(task.getException()); + } else { + return Tasks.forCanceled(); + } + }); + tail = result; + return result; + } + } + + /** + * Forwards a Runnable to the underlying executor. + * + *

This is useful for passing the worker as the executor to task continuations. + * + *

This is different than {@link #submit(Runnable)}. This will not submit the runnable to the + * worker to execute in order, this will forward the runnable to the underlying executor. If you + * are calling this directly from your code, you probably want {@link #submit(Runnable)}. + */ + @Override + @Discouraged(message = "This is probably not that you want. Use {@link #submit(Runnable)}.") + public void execute(Runnable runnable) { + executor.execute(runnable); + } + + /** + * Blocks until all current pending tasks have completed, up to 30 seconds. Only for testing. + * + *

This is not a shutdown, this does not stop new tasks from being submitted to the queue. + */ + @VisibleForTesting + public void await() throws ExecutionException, InterruptedException, TimeoutException { + // Submit an empty runnable, and await on it for 30 sec so deadlocked tests fail faster. + Tasks.await(submit(() -> {}), 30, TimeUnit.SECONDS); + + // Sleep for a bit here, instead of de-flaking individual test cases. + Thread.sleep(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 new file mode 100644 index 00000000000..dcb4cb6e7b1 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt @@ -0,0 +1,85 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency + +import com.google.firebase.crashlytics.internal.Logger +import java.util.concurrent.ExecutorService + +/** + * Container to hold the different Crashlytics workers. + * + * @suppress @hide + */ +class CrashlyticsWorkers( + backgroundExecutorService: ExecutorService, + blockingExecutorService: ExecutorService, +) { + + /** + * The common worker is for common background tasks, like background init, user actions, and + * processing uncaught exceptions. This is the main worker of the sdk. This worker will never + * block on a disk write or network call. + */ + @JvmField val common = CrashlyticsWorker(backgroundExecutorService) + + /** + * The disk write worker is for background tasks that persisting data to local disk. No user + * action should wait on this. Use for fire and forget, safe to ignore exceptions. + */ + @JvmField val diskWrite = CrashlyticsWorker(backgroundExecutorService) + + /** + * The data collect worker is for any background tasks that send data remotely, like fetching fid, + * settings, or uploading crash reports. This worker is suspended until permission is granted. + */ + @JvmField val dataCollect = CrashlyticsWorker(backgroundExecutorService) + + /** The network worker is for making blocking network calls. */ + @JvmField val network = CrashlyticsWorker(blockingExecutorService) + + /** Convenient preconditions specific for Crashlytics worker threads. */ + companion object { + private val threadName + get() = Thread.currentThread().name + + /** When enabled, failed preconditions will cause assertion errors for debugging. */ + @JvmStatic var enforcement: Boolean = false + + @JvmStatic + fun checkBlockingThread() = + checkThread(::isBlockingThread) { + "Must be called on a blocking thread, was called on $threadName." + } + + @JvmStatic + fun checkBackgroundThread() = + checkThread(::isBackgroundThread) { + "Must be called on a background thread, was called on $threadName." + } + + private fun isBlockingThread() = threadName.contains("Firebase Blocking Thread #") + + private fun isBackgroundThread() = threadName.contains("Firebase Background Thread #") + + private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { + if (!isCorrectThread()) { + Logger.getLogger().d(failureMessage()) + assert(enforcement, failureMessage) + } + } + } +} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java new file mode 100644 index 00000000000..bde486ae521 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java @@ -0,0 +1,18 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** @hide */ +package com.google.firebase.crashlytics.internal.concurrency; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java index e6af8a9bea3..36ca7fda8a5 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java @@ -18,12 +18,11 @@ import androidx.annotation.VisibleForTesting; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.firebase.crashlytics.internal.common.CommonUtils; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.List; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicMarkableReference; import java.util.concurrent.atomic.AtomicReference; @@ -47,7 +46,8 @@ public class UserMetadata { @VisibleForTesting public static final int MAX_ROLLOUT_ASSIGNMENTS = 128; private final MetaDataStore metaDataStore; - private final CrashlyticsBackgroundWorker backgroundWorker; + + private final CrashlyticsWorkers crashlyticsWorkers; private String sessionIdentifier; // The following references contain a marker bit, which is true if the data maintained in the @@ -69,9 +69,9 @@ public static String readUserId(String sessionId, FileStore fileStore) { } public static UserMetadata loadFromExistingSession( - String sessionId, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionId, FileStore fileStore, CrashlyticsWorkers crashlyticsWorkers) { MetaDataStore store = new MetaDataStore(fileStore); - UserMetadata metadata = new UserMetadata(sessionId, fileStore, backgroundWorker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, crashlyticsWorkers); // We don't use the set methods in this class, because they will attempt to re-serialize the // data, which is unnecessary because we just read them from disk. metadata.customKeys.map.getReference().setKeys(store.readKeyData(sessionId, false)); @@ -82,10 +82,10 @@ public static UserMetadata loadFromExistingSession( } public UserMetadata( - String sessionIdentifier, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionIdentifier, FileStore fileStore, CrashlyticsWorkers crashlyticsWorkers) { this.sessionIdentifier = sessionIdentifier; this.metaDataStore = new MetaDataStore(fileStore); - this.backgroundWorker = backgroundWorker; + this.crashlyticsWorkers = crashlyticsWorkers; } /** @@ -99,15 +99,18 @@ public void setNewSession(String sessionId) { sessionIdentifier = sessionId; Map keyData = customKeys.getKeys(); List rolloutAssignments = rolloutsState.getRolloutAssignmentList(); - if (getUserId() != null) { - metaDataStore.writeUserData(sessionId, getUserId()); - } - if (!keyData.isEmpty()) { - metaDataStore.writeKeyData(sessionId, keyData); - } - if (!rolloutAssignments.isEmpty()) { - metaDataStore.writeRolloutState(sessionId, rolloutAssignments); - } + crashlyticsWorkers.diskWrite.submit( + () -> { + if (getUserId() != null) { + metaDataStore.writeUserData(sessionId, getUserId()); + } + if (!keyData.isEmpty()) { + metaDataStore.writeKeyData(sessionId, keyData); + } + if (!rolloutAssignments.isEmpty()) { + metaDataStore.writeRolloutState(sessionId, rolloutAssignments); + } + }); } } @@ -129,14 +132,12 @@ public void setUserId(String identifier) { } userId.set(sanitizedNewId, true); } - backgroundWorker.submit( - () -> { - serializeUserDataIfNeeded(); - return null; - }); + crashlyticsWorkers.diskWrite.submit(this::serializeUserDataIfNeeded); } - /** @return defensive copy of the custom keys. */ + /** + * @return defensive copy of the custom keys. + */ public Map getCustomKeys() { return customKeys.getKeys(); } @@ -159,7 +160,9 @@ public void setCustomKeys(Map keysAndValues) { customKeys.setKeys(keysAndValues); } - /** @return defensive copy of the internal keys. */ + /** + * @return defensive copy of the internal keys. + */ public Map getInternalKeys() { return internalKeys.getKeys(); } @@ -188,11 +191,9 @@ public boolean updateRolloutsState(List rolloutAssignments) { return false; } List updatedRolloutAssignments = rolloutsState.getRolloutAssignmentList(); - backgroundWorker.submit( - () -> { - metaDataStore.writeRolloutState(sessionIdentifier, updatedRolloutAssignments); - return null; - }); + + crashlyticsWorkers.diskWrite.submit( + () -> metaDataStore.writeRolloutState(sessionIdentifier, updatedRolloutAssignments)); return true; } } @@ -224,7 +225,7 @@ private void serializeUserDataIfNeeded() { */ private class SerializeableKeysMap { final AtomicMarkableReference map; - private final AtomicReference> queuedSerializer = new AtomicReference<>(null); + private final AtomicReference queuedSerializer = new AtomicReference<>(null); private final boolean isInternal; public SerializeableKeysMap(boolean isInternal) { @@ -262,17 +263,16 @@ public void setKeys(Map keysAndValues) { } private void scheduleSerializationTaskIfNeeded() { - Callable newCallable = + Runnable newRunnable = () -> { queuedSerializer.set(null); serializeIfMarked(); - return null; }; // Don't schedule the task if there's another queued task waiting, because the already-queued // task will write the latest data. - if (queuedSerializer.compareAndSet(null, newCallable)) { - backgroundWorker.submit(newCallable); + if (queuedSerializer.compareAndSet(null, newRunnable)) { + crashlyticsWorkers.diskWrite.submit(newRunnable); } } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java index 205e10fb4a5..f7fcfffc060 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java @@ -15,6 +15,7 @@ package com.google.firebase.crashlytics.internal.network; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; @@ -56,6 +57,7 @@ public HttpGetRequest header(Map.Entry entry) { } public HttpResponse execute() throws IOException { + CrashlyticsWorkers.checkBlockingThread(); // Network calls must be on a blocking thread. InputStream stream = null; HttpsURLConnection connection = null; String body = null; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java index dcfbdf8d0fd..b22c01c7be3 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java @@ -17,6 +17,7 @@ import android.text.TextUtils; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpGetRequest; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.network.HttpResponse; @@ -96,6 +97,7 @@ protected HttpGetRequest createHttpGetRequest(Map queryParams) { @Override public JSONObject invoke(SettingsRequest requestData, boolean dataCollectionToken) { + CrashlyticsWorkers.checkBlockingThread(); if (!dataCollectionToken) { throw new RuntimeException("An invalid data collection token was used."); } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java index d44c37257e3..3d87b78eafe 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 @@ -30,12 +30,12 @@ import com.google.firebase.crashlytics.internal.common.DeliveryMechanism; import com.google.firebase.crashlytics.internal.common.IdManager; import com.google.firebase.crashlytics.internal.common.SystemCurrentTimeProvider; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.Locale; -import java.util.concurrent.Executor; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; -import org.json.JSONException; import org.json.JSONObject; /** Implements the logic of when to use cached settings, and when to load them from the network. */ @@ -149,8 +149,8 @@ public Settings getSettingsSync() { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(Executor executor) { - return loadSettingsData(SettingsCacheBehavior.USE_CACHE, executor); + public Task loadSettingsData(CrashlyticsWorkers crashlyticsWorkers) { + return loadSettingsData(SettingsCacheBehavior.USE_CACHE, crashlyticsWorkers); } /** @@ -158,7 +158,8 @@ public Task loadSettingsData(Executor executor) { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor executor) { + public Task loadSettingsData( + SettingsCacheBehavior cacheBehavior, CrashlyticsWorkers crashlyticsWorkers) { // TODO: Refactor this so that it doesn't do the cache lookup twice when settings are // expired. @@ -186,18 +187,24 @@ public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor } // Kick off fetching fresh settings. + // TODO(mrober): Refactor to call worker directly, not expose executor. return dataCollectionArbiter - .waitForDataCollectionPermission(executor) + .waitForDataCollectionPermission() .onSuccessTask( - executor, + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override public Task then(@Nullable Void aVoid) throws Exception { // Waited for data collection permission, so this is safe. final boolean dataCollectionToken = true; - final JSONObject settingsJson = - settingsSpiCall.invoke(settingsRequest, dataCollectionToken); + Future settingsFuture = + crashlyticsWorkers + .network + .getExecutor() + .submit(() -> settingsSpiCall.invoke(settingsRequest, dataCollectionToken)); + // TODO(mrober): Should we add a timeout here, or let the entire init timeout? + JSONObject settingsJson = settingsFuture.get(); if (settingsJson != null) { final Settings fetchedSettings = @@ -256,7 +263,7 @@ private Settings getCachedSettingsData(SettingsCacheBehavior cacheBehavior) { return toReturn; } - private void logSettings(JSONObject json, String message) throws JSONException { + private void logSettings(JSONObject json, String message) { Logger.getLogger().d(message + json.toString()); } diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java index 24ce37dd4b7..ed21115c075 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java +++ b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java @@ -28,10 +28,12 @@ import android.content.Context; import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.persistence.FileStore; @@ -41,6 +43,7 @@ import com.google.firebase.crashlytics.internal.settings.SettingsProvider; import com.google.firebase.inject.Deferred; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.TreeSet; @@ -63,6 +66,8 @@ public class CrashlyticsControllerRobolectricTest { @Mock private SessionReportingCoordinator mockSessionReportingCoordinator; @Mock private DataCollectionArbiter mockDataCollectionArbiter; + private CrashlyticsWorkers crashlyticsWorkers; + private static final CrashlyticsNativeComponent MISSING_NATIVE_COMPONENT = new CrashlyticsNativeComponentDeferredProxy( new Deferred() { @@ -78,17 +83,24 @@ public void setUp() { MockitoAnnotations.openMocks(this); testContext = getApplicationContext(); testFileStore = new FileStore(testContext); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); } @Test - public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist() { + public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist() + throws Exception { final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); mockSettingsProvider(true, false); - controller.doCloseSessions(mockSettingsProvider); + + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(10); + // Since we haven't added any app exit info to the shadow activity manager, there won't exist a // single app exit info, and so this method won't be called. verify(mockSessionReportingCoordinator, never()) @@ -97,7 +109,8 @@ public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntE } @Test - public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { + public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() throws Exception { + final String sessionIdPrevious = "sessionIdPrevious"; final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); // Adds multiple AppExitInfos to confirm that Crashlytics loops through @@ -107,26 +120,30 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { List testApplicationExitInfo = getApplicationExitInfoList(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) - .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); + .thenReturn(new TreeSet<>(Arrays.asList(sessionId, sessionIdPrevious))); mockSettingsProvider(true, false); - controller.doCloseSessions(mockSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(100); verify(mockSessionReportingCoordinator) .persistRelevantAppExitInfoEvent( - eq(sessionId), + eq(sessionIdPrevious), eq(testApplicationExitInfo), any(LogFileManager.class), any(UserMetadata.class)); } @Test - public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() { + public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() throws Exception { final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); mockSettingsProvider(false, false); - controller.doCloseSessions(mockSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(10); verify(mockSessionReportingCoordinator, never()) .persistRelevantAppExitInfoEvent( eq(sessionId), any(), any(LogFileManager.class), any(UserMetadata.class)); @@ -164,7 +181,6 @@ private CrashlyticsController createController() { final CrashlyticsController controller = new CrashlyticsController( testContext, - new CrashlyticsBackgroundWorker(Runnable::run), idManager, mockDataCollectionArbiter, testFileStore, @@ -175,7 +191,8 @@ private CrashlyticsController createController() { mockSessionReportingCoordinator, MISSING_NATIVE_COMPONENT, mock(AnalyticsEventLogger.class), - mock(CrashlyticsAppQualitySessionsSubscriber.class)); + mock(CrashlyticsAppQualitySessionsSubscriber.class), + crashlyticsWorkers); controller.openSession(SESSION_ID); return controller; } diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java index c29041668a6..67be4920dad 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java +++ b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java @@ -25,9 +25,11 @@ import android.app.ActivityManager; import android.app.ApplicationExitInfo; import android.content.Context; -import android.os.Build; -import androidx.annotation.RequiresApi; +import android.os.Build.VERSION_CODES; import androidx.test.core.app.ApplicationProvider; +import androidx.test.filters.SdkSuppress; +import com.google.firebase.concurrent.TestOnlyExecutors; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; @@ -61,6 +63,9 @@ public class SessionReportingCoordinatorRobolectricTest { private SessionReportingCoordinator reportingCoordinator; + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -72,7 +77,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + crashlyticsWorkers); mockEventInteractions(); } @@ -197,7 +203,7 @@ private List getAppExitInfoList() { return activityManager.getHistoricalProcessExitReasons(null, 0, 0); } - @RequiresApi(api = Build.VERSION_CODES.R) + @SdkSuppress(minSdkVersion = VERSION_CODES.R) private static CrashlyticsReport.ApplicationExitInfo convertApplicationExitInfo( ApplicationExitInfo applicationExitInfo) { // The ApplicationExitInfo inserted by ShadowApplicationManager does not contain an input trace, From f239ed63d3c0f8e95aa9b247315c4b252744466b Mon Sep 17 00:00:00 2001 From: themiswang Date: Mon, 19 Aug 2024 14:20:31 -0400 Subject: [PATCH 02/11] 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 42aa3e2ab2ae6afa31e43a5a5e73a188a8fc2ac2 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Mon, 19 Aug 2024 16:53:31 -0400 Subject: [PATCH 03/11] 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 42e594ad1bfa5b12a33a729c53a671a72587c4a7 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Mon, 19 Aug 2024 17:09:06 -0400 Subject: [PATCH 04/11] 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 af1e53a7d05c9c38f32c0c5d9fdd462dd2627740 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Wed, 21 Aug 2024 14:16:58 -0400 Subject: [PATCH 05/11] 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 df7e69cfbf6a30a874dbc832369ca30244b49073 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Wed, 21 Aug 2024 16:48:43 -0400 Subject: [PATCH 06/11] 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 aafc9a5cb3d05a4d8abff4f5952df660e951a0a5 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 22 Aug 2024 11:27:50 -0400 Subject: [PATCH 07/11] 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 73ddb6786891b7ae2a7ca013fead2192dcc5e895 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 22 Aug 2024 11:44:24 -0400 Subject: [PATCH 08/11] 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 f9c01c0fa951427a98e2a21d119ff17afbdb49c2 Mon Sep 17 00:00:00 2001 From: themiswang Date: Tue, 27 Aug 2024 09:57:23 -0400 Subject: [PATCH 09/11] 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 971c9e334fe..4deda310a06 100644 --- a/firebase-crashlytics-ndk/gradle.properties +++ b/firebase-crashlytics-ndk/gradle.properties @@ -1,2 +1,2 @@ version=19.1.1 -latestReleasedVersion=19.1.0 +latestReleasedVersion=19.1.0 \ No newline at end of file diff --git a/firebase-crashlytics/gradle.properties b/firebase-crashlytics/gradle.properties index 971c9e334fe..4deda310a06 100644 --- a/firebase-crashlytics/gradle.properties +++ b/firebase-crashlytics/gradle.properties @@ -1,2 +1,2 @@ version=19.1.1 -latestReleasedVersion=19.1.0 +latestReleasedVersion=19.1.0 \ No newline at end of file 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 9630ff13f4551c2425913a2fdcda00e485789348 Mon Sep 17 00:00:00 2001 From: themiswang Date: Tue, 27 Aug 2024 13:38:03 -0400 Subject: [PATCH 10/11] 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 f35763202849f4c1a4d3e85db9f87b5a63cfbbb3 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Thu, 5 Sep 2024 11:53:09 -0400 Subject: [PATCH 11/11] Update CHANGELOG (#6231) --- firebase-crashlytics/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/firebase-crashlytics/CHANGELOG.md b/firebase-crashlytics/CHANGELOG.md index 30835e8fcc5..372e47310ef 100644 --- a/firebase-crashlytics/CHANGELOG.md +++ b/firebase-crashlytics/CHANGELOG.md @@ -1,5 +1,8 @@ # Unreleased - +* [fixed] Improved data consistency for rapid user actions. +* [changed] Internal changes to improve startup time. +* [changed] Internal changes to the way background tasks are scheduled. +* [changed] Migrated SDK to use standard Firebase executors. # 19.1.0 * [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is