Fix breadcrumb race condition: log() drops entry before logException(… (#8076)

…) reads it

CrashlyticsCore.log() used common.submit() which completed as soon as
the diskWrite task was enqueued, not when it finished. This allowed the
subsequent logException() common task to call
logFileManager.getLogString() before writeToLog() had run on the
diskWrite worker, silently dropping the breadcrumb from the non-fatal
report.

Fix: change to common.submitTask() so the common worker suspends until
the diskWrite task resolves before dispatching the next item (e.g.
logException).

Adds a regression test that calls log() immediately before
logException(), awaits only the common worker, and asserts the
breadcrumb is present on disk.

Fixes #8034

Author: jrodiz

firebase-crashlytics/CHANGELOG.md

@@ -1,5 +1,8 @@
 # Unreleased
 
+- [fixed] Fixed race condition that caused logs from background threads to not be attached to
+  reports in some cases [#8034]
+
 # 20.0.5
 
 - [fixed] Fixed a runtime crash that could occur in minified native apps when using the Crashlytics

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java

@@ -16,6 +16,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.any;
@@ -458,6 +459,31 @@ public void testBreadcrumbSourceIsRegistered() {
     Mockito.verify(mockBreadcrumbSource).registerBreadcrumbHandler(any(BreadcrumbHandler.class));
   }
 
+  /**
+   * Regression test for https://github.com/firebase/firebase-android-sdk/issues/8034.
+   *
+   * <p>Verifies that a breadcrumb logged immediately before {@code logException} is written to disk
+   * before the non-fatal event snapshots the log. Prior to the fix, {@code log()} used
+   * {@code common.submit()} which completed as soon as the disk-write was enqueued, allowing the
+   * subsequent {@code logException()} task to read an empty log. The fix uses {@code submitTask()}
+   * so the common worker suspends until the disk write finishes.
+   */
+  @Test
+  public void testLog_breadcrumbIsWrittenBeforeLogExceptionReadsIt() throws Exception {
+    final String breadcrumb = "test breadcrumb";
+
+    crashlyticsCore.log(breadcrumb);
+    crashlyticsCore.logException(new RuntimeException("non-fatal"), Map.of());
+
+    // Awaiting common is sufficient: submitTask makes common suspend until diskWrite completes,
+    // so when this returns the log entry is guaranteed to be on disk.
+    crashlyticsWorkers.common.await();
+
+    final String logString = crashlyticsCore.getController().getLogString();
+    assertNotNull("Log should have been written before logException read it", logString);
+    assertTrue("Log should contain the breadcrumb", logString.contains(breadcrumb));
+  }
+
   @Test
   public void testOnPreExecute_nativeDidCrashOnPreviousExecution() throws Exception {
     appRestart(); // Create a previous execution

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java

@@ -25,6 +25,7 @@
 import android.util.Base64;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.tasks.SuccessContinuation;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
@@ -793,6 +794,12 @@ UserMetadata getUserMetadata() {
     return userMetadata;
   }
 
+  @VisibleForTesting
+  @Nullable
+  String getLogString() {
+    return logFileManager.getLogString();
+  }
+
   boolean isHandlingException() {
     return crashHandler != null && crashHandler.isHandlingException();
   }

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java

@@ -330,11 +330,10 @@ public void logException(@NonNull Throwable throwable, @NonNull Map<String, Stri
    */
   public void log(final String msg) {
     final long timestamp = System.currentTimeMillis() - startTime;
-    // queuing up on common worker to maintain the order
-    crashlyticsWorkers.common.submit(
-        () -> {
-          crashlyticsWorkers.diskWrite.submit(() -> controller.writeToLog(timestamp, msg));
-        });
+    // submitTask ensures the common worker waits for the diskWrite task to complete, ensuring
+    // that subsequent tasks on the common worker (e.g. logException) see this log entry.
+    crashlyticsWorkers.common.submitTask(
+        () -> crashlyticsWorkers.diskWrite.submit(() -> controller.writeToLog(timestamp, msg)));
   }
 
   public void setUserId(String identifier) {