Crashlytics efficient keys (#3254)

Refactors UserMetadata and related classes to gain some significant efficiency improvements when calling `setCustomKey`:
* Checks if the map contents have actually changed before writing the key/value store to disk.
* UserMetadata now maintains a "dirty" marker bit for the keys, so calls to serialize the key will no-op when the bit is false.
* Allow at most one queued serialization task at a time, and the queued task will always grab the latest key data when it executes. This prevents a large amount of stale Task objects being created and queued when `setCustomKeys` is invoked repeatedly. (Which has been known to cause OOMs in some extreme cases.)

Also finishes cleanup of extra files from PR #3160: That PR was a quick fix for misreading the user id, which was stored in two different places on disk. This is a proper fix that uses a single source of truth for the serialized user id.

Author: mrichards

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

@@ -37,7 +37,7 @@
 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.log.LogFileManager;
+import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.persistence.FileStore;
 import com.google.firebase.crashlytics.internal.settings.SettingsDataProvider;
 import com.google.firebase.crashlytics.internal.settings.TestSettingsData;
@@ -54,6 +54,7 @@
 
 public class CrashlyticsControllerTest extends CrashlyticsTestCase {
   private static final String GOOGLE_APP_ID = "google:app:id";
+  private static final String SESSION_ID = "session_id";
 
   private Context testContext;
   private IdManager idManager;
@@ -170,7 +171,7 @@ private ControllerBuilder builder() {
   /** Creates a new CrashlyticsController with default options and opens a session. */
   private CrashlyticsController createController() {
     final CrashlyticsController controller = builder().build();
-    controller.openSession();
+    controller.openSession(SESSION_ID);
     return controller;
   }
 
@@ -488,7 +489,7 @@ public void testFatalEvent_sendsAppExceptionEvent() {
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
         .thenReturn(new TreeSet<>(Collections.singleton(sessionId)));
 
-    controller.openSession();
+    controller.openSession(SESSION_ID);
     controller.handleUncaughtException(
         testSettingsDataProvider, Thread.currentThread(), new RuntimeException("Fatal"));
     controller.finalizeSessions(testSettingsDataProvider);

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

@@ -37,6 +37,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.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.persistence.FileStore;
 import com.google.firebase.crashlytics.internal.settings.SettingsController;
 import com.google.firebase.crashlytics.internal.settings.TestSettingsData;
@@ -105,6 +106,12 @@ public void testCustomAttributes() throws Exception {
     crashlyticsCore.setCustomKey(key1, value1);
     assertEquals(value1, metadata.getCustomKeys().get(key1));
 
+    // Adding an existing key with the same value should return false
+    assertFalse(metadata.setCustomKey(key1, value1));
+    assertTrue(metadata.setCustomKey(key1, "someOtherValue"));
+    assertTrue(metadata.setCustomKey(key1, value1));
+    assertFalse(metadata.setCustomKey(key1, value1));
+
     final String longValue = longId.replaceAll("0", "x");
     final String superLongValue = longValue + "some more chars";
 

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

@@ -31,7 +31,8 @@
 
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
-import com.google.firebase.crashlytics.internal.log.LogFileManager;
+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.model.CrashlyticsReport.CustomAttribute;
 import com.google.firebase.crashlytics.internal.model.ImmutableList;
@@ -411,20 +412,6 @@ public void onReportSend_successfulReportsAreDeleted() {
     verify(reportSender).sendReport(mockReport2);
   }
 
-  @Test
-  public void testPersistUserIdForCurrentSession_persistsCurrentUserIdForCurrentSessionId() {
-    final String currentSessionId = "currentSessionId";
-    final String userId = "testUserId";
-    final long timestamp = System.currentTimeMillis();
-    when(dataCapture.captureReportData(anyString(), anyLong())).thenReturn(mockReport);
-    when(reportMetadata.getUserId()).thenReturn(userId);
-
-    reportingCoordinator.onBeginSession(currentSessionId, timestamp);
-    reportingCoordinator.persistUserId(currentSessionId);
-
-    verify(reportPersistence).persistUserIdForSession(userId, currentSessionId);
-  }
-
   @Test
   public void testHasReportsToSend() {
     when(reportPersistence.hasFinalizedReports()).thenReturn(true);

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/log/LogFileManagerTest.java renamed to firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/LogFileManagerTest.java

@@ -12,9 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.firebase.crashlytics.internal.log;
+package com.google.firebase.crashlytics.internal.metadata;
 
-import static com.google.firebase.crashlytics.internal.log.LogFileManager.MAX_LOG_SIZE;
+import static com.google.firebase.crashlytics.internal.metadata.LogFileManager.MAX_LOG_SIZE;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;

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

@@ -12,9 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.firebase.crashlytics.internal.common;
+package com.google.firebase.crashlytics.internal.metadata;
 
 import com.google.firebase.crashlytics.internal.CrashlyticsTestCase;
+import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker;
 import com.google.firebase.crashlytics.internal.persistence.FileStore;
 import java.util.Collections;
 import java.util.HashMap;
@@ -39,6 +40,7 @@ public class MetaDataStoreTest extends CrashlyticsTestCase {
   private static final String ESCAPED = "\ttest\nvalue";
 
   private FileStore fileStore;
+  private CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run);
 
   private MetaDataStore storeUnderTest;
 
@@ -49,66 +51,69 @@ public void setUp() throws Exception {
     storeUnderTest = new MetaDataStore(fileStore);
   }
 
-  private static UserMetadata metadataWithUserId() {
-    return metadataWithUserId(USER_ID);
+  private UserMetadata metadataWithUserId(String sessionId) {
+    return metadataWithUserId(sessionId, USER_ID);
   }
 
-  private static UserMetadata metadataWithUserId(String id) {
-    UserMetadata metadata = new UserMetadata();
-    metadata.setUserId(id);
+  private UserMetadata metadataWithUserId(String sessionId, String userId) {
+    UserMetadata metadata = new UserMetadata(sessionId, fileStore, worker);
+    metadata.setUserId(userId);
     return metadata;
   }
 
   public void testWriteUserData_allFields() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId());
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals(USER_ID, userData.getUserId());
   }
 
   public void testWriteUserData_noFields() {
-    storeUnderTest.writeUserData(SESSION_ID_1, new UserMetadata());
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(
+        SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertNull(userData.getUserId());
   }
 
   public void testWriteUserData_singleField() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId());
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals(USER_ID, userData.getUserId());
   }
 
   public void testWriteUserData_null() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(null));
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals(null, userData.getUserId());
   }
 
   public void testWriteUserData_emptyString() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(""));
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals("", userData.getUserId());
   }
 
   public void testWriteUserData_unicode() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(UNICODE));
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(
+        SESSION_ID_1, metadataWithUserId(SESSION_ID_1, UNICODE).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals(UNICODE, userData.getUserId());
   }
 
   public void testWriteUserData_escaped() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(ESCAPED));
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    storeUnderTest.writeUserData(
+        SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertEquals(ESCAPED.trim(), userData.getUserId());
   }
 
   public void testWriteUserData_readDifferentSession() {
-    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId());
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_2);
+    storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId());
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, worker);
     assertNull(userData.getUserId());
   }
 
   public void testReadUserData_noStoredData() {
-    final UserMetadata userData = storeUnderTest.readUserData(SESSION_ID_1);
+    UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
     assertNull(userData.getUserId());
   }
 

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/persistence/CrashlyticsReportPersistenceTest.java

@@ -390,13 +390,11 @@ public void testFinalizeReports_removesOldestReportsFirst() throws IOException {
   public void testLoadFinalizedReports_reportWithUserId_returnsReportWithProperUserId() {
     final String sessionId = "testSession";
     final String userId = "testUser";
-    final CrashlyticsReport testReport = makeTestReport(sessionId);
+    final CrashlyticsReport testReport = makeTestReport(sessionId, userId);
     final CrashlyticsReport.Session.Event testEvent = makeTestEvent();
 
     reportPersistence.persistReport(testReport);
     reportPersistence.persistEvent(testEvent, sessionId);
-    reportPersistence.persistUserIdForSession(userId, sessionId);
-
     reportPersistence.finalizeReports("skippedSession", 0L);
 
     final List<CrashlyticsReportWithSessionId> finalizedReports =
@@ -409,21 +407,19 @@ public void testLoadFinalizedReports_reportWithUserId_returnsReportWithProperUse
 
   public void
       testLoadFinalizedReports_reportsWithUserIdInMultipleSessions_returnsReportsWithProperUserIds() {
+    final String userId1 = "testUser1";
+    final String userId2 = "testUser2";
     final String sessionId1 = "testSession1";
-    final CrashlyticsReport testReport1 = makeTestReport(sessionId1);
     final String sessionId2 = "testSession2";
-    final CrashlyticsReport testReport2 = makeTestReport(sessionId2);
+    final CrashlyticsReport testReport1 = makeTestReport(sessionId1, userId1);
+    final CrashlyticsReport testReport2 = makeTestReport(sessionId2, userId2);
     final CrashlyticsReport.Session.Event testEvent1 = makeTestEvent();
     final CrashlyticsReport.Session.Event testEvent2 = makeTestEvent();
-    final String userId1 = "testUser1";
-    final String userId2 = "testUser2";
 
     reportPersistence.persistReport(testReport1);
     reportPersistence.persistReport(testReport2);
     reportPersistence.persistEvent(testEvent1, sessionId1);
     reportPersistence.persistEvent(testEvent2, sessionId2);
-    reportPersistence.persistUserIdForSession(userId1, sessionId1);
-    reportPersistence.persistUserIdForSession(userId2, sessionId2);
 
     reportPersistence.finalizeReports("skippedSession", 0L);
 
@@ -675,6 +671,10 @@ private static CrashlyticsReport makeTestReport(String sessionId) {
     return makeIncompleteReport().setSession(makeTestSession(sessionId)).build();
   }
 
+  private static CrashlyticsReport makeTestReport(String sessionId, String userId) {
+    return makeTestReport(sessionId).withSessionEndFields(0L, false, userId);
+  }
+
   private static CrashlyticsReport.FilesPayload makeFilePayload() {
     byte[] testContents = {0, 2, 20, 10};
     return CrashlyticsReport.FilesPayload.builder()

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

@@ -32,6 +32,7 @@
 import android.os.StatFs;
 import android.provider.Settings.Secure;
 import android.text.TextUtils;
+import androidx.annotation.Nullable;
 import com.google.firebase.crashlytics.internal.Logger;
 import java.io.BufferedReader;
 import java.io.Closeable;
@@ -632,4 +633,13 @@ public static boolean canTryConnection(Context context) {
       return true;
     }
   }
+
+  /** @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) {
+      return s2 == null;
+    }
+    return s1.equals(s2);
+  }
 }

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

@@ -33,7 +33,7 @@
  * future work, and run the future work on the executor's thread, but not put it in the queue as its
  * own worker.
  */
-class CrashlyticsBackgroundWorker {
+public class CrashlyticsBackgroundWorker {
   private final Executor executor;
 
   private Task<Void> tail = Tasks.forResult(null);