Merge branch 'main' into gsakakihara/deduplicate_by_message_id

Author: gsakakihara

.github/workflows/check_format.yml

@@ -0,0 +1,73 @@
+name: Check Format
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+
+jobs:
+  determine_changed:
+    name: "Determine changed modules"
+    runs-on: ubuntu-22.04
+    if: (github.repository == 'Firebase/firebase-android-sdk' && github.event_name == 'push') || github.event_name == 'pull_request'
+    outputs:
+      modules: ${{ steps.changed-modules.outputs.modules }}
+    steps:
+      - uses: actions/[email protected]
+        with:
+          fetch-depth: 2
+          submodules: true
+
+      - name: Set up JDK 17
+        uses: actions/[email protected]
+        with:
+          java-version: 17
+          distribution: temurin
+          cache: gradle
+
+      - id: changed-modules
+        run: |
+          git diff --name-only HEAD~1 | xargs printf -- '--changed-git-paths %s\n' | xargs ./gradlew writeChangedProjects --output-file-path=modules.json
+          echo modules=$(cat modules.json) >> $GITHUB_OUTPUT
+
+  check_format:
+    name: "Check Format"
+    runs-on: ubuntu-22.04
+    needs:
+      - determine_changed
+    strategy:
+      fail-fast: false
+      matrix:
+        module: ${{ fromJSON(needs.determine_changed.outputs.modules) }}
+
+    steps:
+      - uses: actions/[email protected]
+        with:
+          fetch-depth: 2
+          submodules: true
+
+      - name: Set up JDK 17
+        uses: actions/[email protected]
+        with:
+          java-version: 17
+          distribution: temurin
+          cache: gradle
+
+      - name: ${{ matrix.module }} Check Format
+        run: |
+          ./gradlew ${{matrix.module}}:spotlessCheck
+
+  # A job that fails if any job in the check_format matrix fails,
+  # to be used as a required check for merging.
+  check_all:
+    runs-on: ubuntu-22.04
+    if: always()
+    name: Check Format (matrix)
+    needs: check_format
+    steps:
+      - name: Check matrix
+        if: needs.check_format.result != 'success'
+        run: exit 1

firebase-crashlytics/CHANGELOG.md

@@ -1,11 +1,11 @@
 # Unreleased
+* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is enabled.
+  ([Github #5919](//github.com/firebase/firebase-android-sdk/issues/5919))
+* [fixed] Ensure that on-demand fatal events are never processed on the main thread.
 
 
 # 19.0.3
 * [changed] Update the internal file system to handle long file names.
-* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is enabled. 
-  ([Github #5919](//github.com/firebase/firebase-android-sdk/issues/5919))
-
 
 ## Kotlin
 The Kotlin extensions library transitively includes the updated

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)
@@ -111,4 +112,6 @@ dependencies {
     androidTestImplementation(libs.androidx.test.junit)
     androidTestImplementation(libs.androidx.test.runner)
     androidTestImplementation(libs.truth)
+    androidTestImplementation(libs.playservices.tasks)
+    androidTestImplementation(project(":integ-testing"))
 }

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/CrashlyticsPluginsTest.java

@@ -0,0 +1,78 @@
+/*
+ * 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;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import androidx.test.core.app.ApplicationProvider;
+import com.google.firebase.FirebaseApp;
+import com.google.firebase.FirebaseOptions;
+import com.google.firebase.crashlytics.internal.common.CrashlyticsCore;
+import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter;
+import java.lang.reflect.Field;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for the internal apis that development platform plugins call. */
+public class CrashlyticsPluginsTest {
+  private static final String APP_ID = "1:1:android:1a";
+  private static final String API_KEY = "API-KEY-API-KEY-API-KEY-API-KEY-API-KEY";
+  private static final String PROJECT_ID = "PROJECT-ID";
+
+  @Before
+  public void setUp() {
+    FirebaseApp.initializeApp(
+        ApplicationProvider.getApplicationContext(),
+        new FirebaseOptions.Builder()
+            .setApplicationId(APP_ID)
+            .setApiKey(API_KEY)
+            .setProjectId(PROJECT_ID)
+            .build());
+  }
+
+  @After
+  public void tearDown() {
+    FirebaseApp.clearInstancesForTest();
+  }
+
+  @Test
+  public void accessCrashlyticsCore() {
+    // Both Flutter and Unity plugins access CrashlyticsCore from FirebaseCrashlytics.core field.
+    CrashlyticsCore core = FirebaseCrashlytics.getInstance().core;
+    assertThat(core).isNotNull();
+
+    // Verify the internal method logFatalException exists without reflection.
+    Runnable logFatalException = () -> core.logFatalException(new Throwable());
+    assertThat(logFatalException).isNotNull();
+
+    // Verify the internal method setInternalKey exists without reflection.
+    Runnable setInternalKey = () -> core.setInternalKey("", "");
+    assertThat(setInternalKey).isNotNull();
+  }
+
+  @Test
+  public void accessDataCollection() throws Exception {
+    // The Unity plugin accesses CrashlyticsCore.dataCollectionArbiter this via reflection.
+    CrashlyticsCore core = FirebaseCrashlytics.getInstance().core;
+    Field field = core.getClass().getDeclaredField("dataCollectionArbiter");
+    field.setAccessible(true); // The dataCollectionArbiter field is private in CrashlyticsCore.
+    DataCollectionArbiter dataCollectionArbiter = (DataCollectionArbiter) field.get(core);
+
+    assertThat(dataCollectionArbiter).isNotNull();
+  }
+}

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.CrashlyticsWorker;
 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 CrashlyticsWorker commonWorker =
+      new CrashlyticsWorker(TestOnlyExecutors.background());
+
   private Context testContext;
   private IdManager idManager;
   private SettingsProvider testSettingsProvider;
@@ -70,6 +74,8 @@ public class CrashlyticsControllerTest extends CrashlyticsTestCase {
   private DataCollectionArbiter mockDataCollectionArbiter;
   private CrashlyticsNativeComponent mockNativeComponent = mock(CrashlyticsNativeComponent.class);
 
+  private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
+
   @Override
   protected void setUp() throws Exception {
     super.setUp();
@@ -99,14 +105,19 @@ protected void setUp() throws Exception {
     when(testSettingsProvider.getSettingsAsync()).thenReturn(Tasks.forResult(testSettings));
   }
 
+  @Override
+  protected void tearDown() throws Exception {
+    super.tearDown();
+    commonWorker.await();
+  }
+
   /** A convenience class for building CrashlyticsController instances for testing. */
   private class ControllerBuilder {
     private DataCollectionArbiter dataCollectionArbiter;
     private CrashlyticsNativeComponent nativeComponent = null;
     private AnalyticsEventLogger analyticsEventLogger;
     private SessionReportingCoordinator sessionReportingCoordinator;
 
-    private CrashlyticsBackgroundWorker backgroundWorker;
     private LogFileManager logFileManager = null;
 
     private UserMetadata userMetadata = null;
@@ -118,8 +129,6 @@ private class ControllerBuilder {
       analyticsEventLogger = mock(AnalyticsEventLogger.class);
 
       sessionReportingCoordinator = mockSessionReportingCoordinator;
-
-      backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService());
     }
 
     ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) {
@@ -168,7 +177,7 @@ public CrashlyticsController build() {
       final CrashlyticsController controller =
           new CrashlyticsController(
               testContext.getApplicationContext(),
-              backgroundWorker,
+              commonWorker,
               idManager,
               dataCollectionArbiter,
               testFileStore,
@@ -179,7 +188,8 @@ public CrashlyticsController build() {
               sessionReportingCoordinator,
               nativeComponent,
               analyticsEventLogger,
-              mock(CrashlyticsAppQualitySessionsSubscriber.class));
+              mock(CrashlyticsAppQualitySessionsSubscriber.class),
+              diskWriteWorker);
       return controller;
     }
   }
@@ -208,6 +218,8 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal()
     controller.writeNonFatalException(thread, nonFatal);
     controller.doCloseSessions(testSettingsProvider);
 
+    commonWorker.await();
+
     verify(mockSessionReportingCoordinator)
         .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong());
   }
@@ -228,9 +240,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 +257,8 @@ public void testOnDemandFatal_callLogFatalException() {
     controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider);
     controller.logFatalException(thread, fatal);
 
+    commonWorker.await();
+
     verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID)));
   }
 
@@ -323,17 +336,20 @@ public File getOsFile() {
     final CrashlyticsController controller =
         builder().setNativeComponent(mockNativeComponent).setLogFileManager(logFileManager).build();
 
-    controller.finalizeSessions(testSettingsProvider);
+    commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
+    commonWorker.await();
+
     verify(mockSessionReportingCoordinator)
         .finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any());
     verify(mockSessionReportingCoordinator, never())
         .finalizeSessionWithNativeEvent(eq(sessionId), any(), any());
   }
 
   @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
-  public void testMissingNativeComponentCausesNoReports() {
+  public void testMissingNativeComponentCausesNoReports() throws Exception {
     final CrashlyticsController controller = createController();
-    controller.finalizeSessions(testSettingsProvider);
+    commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
+    commonWorker.await();
 
     List<String> sessions = testFileStore.getAllOpenSessionIds();
     for (String sessionId : sessions) {
@@ -363,13 +379,14 @@ 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");
+    diskWriteWorker.submit(() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
+    diskWriteWorker.await();
   }
 
   /**
@@ -384,7 +401,8 @@ public void testFinalizeSessionAfterCrashOk() throws Exception {
         testSettingsProvider, Thread.currentThread(), new RuntimeException());
 
     // This should not throw.
-    controller.finalizeSessions(testSettingsProvider);
+    commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
+    commonWorker.await();
   }
 
   @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
@@ -535,7 +553,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 +566,14 @@ public void testFatalEvent_sendsAppExceptionEvent() {
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
         .thenReturn(new TreeSet<>(Collections.singleton(sessionId)));
 
-    controller.openSession(SESSION_ID);
-    controller.handleUncaughtException(
-        testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal"));
-    controller.finalizeSessions(testSettingsProvider);
+    commonWorker.submit(
+        () -> {
+          controller.openSession(SESSION_ID);
+          controller.handleUncaughtException(
+              testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal"));
+          controller.finalizeSessions(testSettingsProvider);
+        });
+    commonWorker.await();
 
     assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger);
   }