User id fix (#5432)

Fix Unity and Flutter on-demand user meta data behaviour. Cherry-picked
from previous closed pr:
#5275

Author: themiswang

firebase-crashlytics/CHANGELOG.md

@@ -10,6 +10,8 @@
   now deprecated. As early as April 2024, we'll no longer release KTX modules. For details, see the
   [FAQ about this initiative](https://firebase.google.com/docs/android/kotlin-migration)
 
+* [fixed] Fixed Flutter and Unity on-demand fatal `setUserIdentifier` behaviour. Github
+  [#10759](https://github.com/firebase/flutterfire/issues/10759)
 
 # 18.4.3
 * [fixed] Disabled `GradleMetadataPublishing` to fix breakage of the Kotlin extensions library. [#5337]

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

@@ -14,6 +14,7 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import static org.mockito.AdditionalMatchers.not;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyLong;
 import static org.mockito.Mockito.anyString;
@@ -38,6 +39,7 @@
 import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
 import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
+import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
 import com.google.firebase.crashlytics.internal.persistence.FileStore;
 import com.google.firebase.crashlytics.internal.settings.Settings;
@@ -52,6 +54,7 @@
 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 {
@@ -101,22 +104,33 @@ private class ControllerBuilder {
     private CrashlyticsNativeComponent nativeComponent = null;
     private AnalyticsEventLogger analyticsEventLogger;
     private SessionReportingCoordinator sessionReportingCoordinator;
+
+    private CrashlyticsBackgroundWorker backgroundWorker;
     private LogFileManager logFileManager = null;
 
+    private UserMetadata userMetadata = null;
+
     ControllerBuilder() {
       dataCollectionArbiter = mockDataCollectionArbiter;
       nativeComponent = mockNativeComponent;
 
       analyticsEventLogger = mock(AnalyticsEventLogger.class);
 
       sessionReportingCoordinator = mockSessionReportingCoordinator;
+
+      backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService());
     }
 
     ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) {
       dataCollectionArbiter = arbiter;
       return this;
     }
 
+    ControllerBuilder setUserMetadata(UserMetadata userMetadata) {
+      this.userMetadata = userMetadata;
+      return this;
+    }
+
     public ControllerBuilder setNativeComponent(CrashlyticsNativeComponent nativeComponent) {
       this.nativeComponent = nativeComponent;
       return this;
@@ -153,13 +167,13 @@ public CrashlyticsController build() {
       final CrashlyticsController controller =
           new CrashlyticsController(
               testContext.getApplicationContext(),
-              new CrashlyticsBackgroundWorker(new SameThreadExecutorService()),
+              backgroundWorker,
               idManager,
               dataCollectionArbiter,
               testFileStore,
               crashMarker,
               appData,
-              null,
+              userMetadata,
               logFileManager,
               sessionReportingCoordinator,
               nativeComponent,
@@ -211,6 +225,26 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th
         .persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong());
   }
 
+  @Test
+  public void testOnDemandFatal_callLogFatalException() {
+    Thread thread = Thread.currentThread();
+    Exception fatal = new RuntimeException("Fatal");
+    Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class);
+    UserMetadata mockUserMetadata = mock(UserMetadata.class);
+    when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
+        .thenReturn(new TreeSet<>(Collections.singleton(SESSION_ID)).descendingSet());
+
+    final CrashlyticsController controller =
+        builder()
+            .setLogFileManager(new LogFileManager(testFileStore))
+            .setUserMetadata(mockUserMetadata)
+            .build();
+    controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider);
+    controller.logFatalException(thread, fatal);
+
+    verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID)));
+  }
+
   public void testNativeCrashDataCausesNativeReport() throws Exception {
     final String sessionId = "sessionId_1_new";
     final String previousSessionId = "sessionId_0_previous";

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java

@@ -14,6 +14,8 @@
 
 package com.google.firebase.crashlytics.internal.metadata;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.firebase.crashlytics.internal.CrashlyticsTestCase;
 import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker;
 import com.google.firebase.crashlytics.internal.persistence.FileStore;
@@ -23,6 +25,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import org.junit.Test;
 
 @SuppressWarnings("ResultOfMethodCallIgnored") // Convenient use of files.
 public class MetaDataStoreTest extends CrashlyticsTestCase {
@@ -139,6 +142,55 @@ public void testReadUserData_noStoredData() {
     assertNull(userData.getUserId());
   }
 
+  @Test
+  public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() {
+    UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
+    userMetadata.setNewSession(SESSION_ID_2);
+    assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists())
+        .isFalse();
+  }
+
+  @Test
+  public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() {
+    UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
+    userMetadata.setNewSession(SESSION_ID_2);
+    assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists())
+        .isFalse();
+  }
+
+  @Test
+  public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() {
+    UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
+    final Map<String, String> keys =
+        new HashMap<String, String>() {
+          {
+            put(KEY_1, VALUE_1);
+            put(KEY_2, VALUE_2);
+            put(KEY_3, VALUE_3);
+          }
+        };
+    userMetadata.setCustomKeys(keys);
+    userMetadata.setNewSession(SESSION_ID_2);
+    assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists())
+        .isTrue();
+
+    MetaDataStore metaDataStore = new MetaDataStore(fileStore);
+    assertThat(metaDataStore.readKeyData(SESSION_ID_2)).isEqualTo(keys);
+  }
+
+  @Test
+  public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() {
+    String userId = "ThemisWang";
+    UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
+    userMetadata.setUserId(userId);
+    userMetadata.setNewSession(SESSION_ID_2);
+    assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists())
+        .isTrue();
+
+    MetaDataStore metaDataStore = new MetaDataStore(fileStore);
+    assertThat(metaDataStore.readUserId(SESSION_ID_2)).isEqualTo(userId);
+  }
+
   // Keys
 
   public void testWriteKeys() {

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

@@ -216,7 +216,7 @@ public Task<Void> call() throws Exception {
 
                 doWriteAppExceptionMarker(timestampMillis);
                 doCloseSessions(settingsProvider);
-                doOpenSession(new CLSUUID(idManager).toString());
+                doOpenSession(new CLSUUID(idManager).toString(), isOnDemand);
 
                 // If automatic data collection is disabled, we'll need to wait until the next run
                 // of the app.
@@ -498,7 +498,7 @@ void openSession(String sessionIdentifier) {
         new Callable<Void>() {
           @Override
           public Void call() throws Exception {
-            doOpenSession(sessionIdentifier);
+            doOpenSession(sessionIdentifier, /*isOnDemand=*/ false);
             return null;
           }
         });
@@ -550,7 +550,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) {
    * Not synchronized/locked. Must be executed from the single thread executor service used by this
    * class.
    */
-  private void doOpenSession(String sessionIdentifier) {
+  private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) {
     final long startedAtSeconds = getCurrentTimestampSeconds();
 
     Logger.getLogger().d("Opening a new session with ID " + sessionIdentifier);
@@ -568,6 +568,14 @@ private void doOpenSession(String sessionIdentifier) {
         startedAtSeconds,
         StaticSessionData.create(appData, osData, deviceData));
 
+    // If is on-demand fatal, we need to update the session id for userMetadata
+    // as well(since we don't really change the object to a new one for a new session).
+    // all the information in the previous session is still in memory, but we do need to
+    // manually writing them into persistence for the new session.
+    if (isOnDemand && sessionIdentifier != null) {
+      userMetadata.setNewSession(sessionIdentifier);
+    }
+
     logFileManager.setCurrentSession(sessionIdentifier);
     sessionsSubscriber.setSessionId(sessionIdentifier);
     reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds);

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java

@@ -41,7 +41,7 @@ public class UserMetadata {
 
   private final MetaDataStore metaDataStore;
   private final CrashlyticsBackgroundWorker backgroundWorker;
-  private final String sessionIdentifier;
+  private String sessionIdentifier;
 
   // The following references contain a marker bit, which is true if the data maintained in the
   // associated reference has been serialized since the last time it was updated.
@@ -77,6 +77,26 @@ public UserMetadata(
     this.backgroundWorker = backgroundWorker;
   }
 
+  /**
+   * Refresh the userMetadata to reflect the status of the new session. This API is mainly for
+   * on-demand fatal feature since we need to close and update to a new session. UserMetadata also
+   * need to make this update instead of updating session id, we also need to manually writing the
+   * into persistence for the new session.
+   */
+  public void setNewSession(String sessionId) {
+    synchronized (sessionIdentifier) {
+      sessionIdentifier = sessionId;
+      Map<String, String> keyData = customKeys.getKeys();
+      if (getUserId() != null) {
+        metaDataStore.writeUserData(sessionId, getUserId());
+      }
+      if (!keyData.isEmpty()) {
+        metaDataStore.writeKeyData(sessionId, keyData);
+      }
+      // TODO(themis): adding feature rollouts later
+    }
+  }
+
   @Nullable
   public String getUserId() {
     return userId.getReference();