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 3a513268148..3afc587b2fc 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 @@ -70,6 +70,10 @@ public void whenAvailable( private CrashlyticsCore crashlyticsCore; private BreadcrumbSource mockBreadcrumbSource; + private static final CrashlyticsWorker commonWorker = + new CrashlyticsWorker(TestOnlyExecutors.background()); + private static final CrashlyticsWorker diskWriteWorker = + new CrashlyticsWorker(TestOnlyExecutors.background()); @Override protected void setUp() throws Exception { @@ -93,7 +97,7 @@ public void testCustomAttributes() throws Exception { final String id = "id012345"; crashlyticsCore.setUserId(id); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals(id, metadata.getUserId()); final StringBuffer idBuffer = new StringBuffer(id); @@ -104,13 +108,13 @@ public void testCustomAttributes() throws Exception { final String superLongId = longId + "more chars"; crashlyticsCore.setUserId(superLongId); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals(longId, metadata.getUserId()); final String key1 = "key1"; final String value1 = "value1"; crashlyticsCore.setCustomKey(key1, value1); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals(value1, metadata.getCustomKeys().get(key1)); // Adding an existing key with the same value should return false @@ -124,7 +128,7 @@ public void testCustomAttributes() throws Exception { // test truncation of custom keys and attributes crashlyticsCore.setCustomKey(superLongId, superLongValue); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertNull(metadata.getCustomKeys().get(superLongId)); assertEquals(longValue, metadata.getCustomKeys().get(longId)); @@ -133,28 +137,28 @@ public void testCustomAttributes() throws Exception { final String key = "key" + i; final String value = "value" + i; crashlyticsCore.setCustomKey(key, value); - crashlyticsCore.commonWorker.await(); + commonWorker.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"); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertFalse(metadata.getCustomKeys().containsKey(key)); // should be able to update existing keys crashlyticsCore.setCustomKey(key1, longValue); - crashlyticsCore.commonWorker.await(); + commonWorker.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); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals("", metadata.getCustomKeys().get(key1)); // keys and values are trimmed. crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " "); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertTrue(metadata.getCustomKeys().containsKey(key1)); assertEquals(longValue, metadata.getCustomKeys().get(key1)); } @@ -205,7 +209,7 @@ public void testBulkCustomKeys() throws Exception { keysAndValues.put(intKey, String.valueOf(intValue)); crashlyticsCore.setCustomKeys(keysAndValues); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals(stringValue, metadata.getCustomKeys().get(stringKey)); assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey)); @@ -226,7 +230,7 @@ public void testBulkCustomKeys() throws Exception { addlKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(addlKeysAndValues); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); // Ensure all keys have been set assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA); @@ -244,7 +248,7 @@ public void testBulkCustomKeys() throws Exception { extraKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(extraKeysAndValues); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); // Make sure the extra keys were not added for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) { @@ -270,7 +274,7 @@ public void testBulkCustomKeys() throws Exception { updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue)); crashlyticsCore.setCustomKeys(updatedKeysAndValues); - crashlyticsCore.commonWorker.await(); + commonWorker.await(); assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey)); assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey))); @@ -443,8 +447,8 @@ CrashlyticsCore build(Context context) { new FileStore(context), mock(CrashlyticsAppQualitySessionsSubscriber.class), mock(RemoteConfigDeferredProxy.class), - new CrashlyticsWorker(TestOnlyExecutors.background()), - new CrashlyticsWorker(TestOnlyExecutors.background())); + commonWorker, + diskWriteWorker); 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..8d74d6fe249 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.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; @@ -69,6 +71,8 @@ public class SessionReportingCoordinatorTest { private SessionReportingCoordinator reportingCoordinator; + private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -80,7 +84,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + diskWriteWorker); } @Test @@ -116,7 +121,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 +132,8 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + final boolean expectedAllThreads = false; final boolean expectedHighPriority = false; @@ -136,7 +144,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes } @Test - public void testNonFatalEvent_addsLogsToEvent() { + public void testNonFatalEvent_addsLogsToEvent() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -149,6 +157,8 @@ public void testNonFatalEvent_addsLogsToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + verify(mockEventBuilder) .setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build()); verify(mockEventBuilder).build(); @@ -156,7 +166,7 @@ public void testNonFatalEvent_addsLogsToEvent() { } @Test - public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -168,6 +178,8 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class)); verify(mockEventBuilder).build(); verify(logFileManager, never()).clearLog(); @@ -212,7 +224,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() { } @Test - public void testNonFatalEvent_addsSortedKeysToEvent() { + public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -243,6 +255,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes); verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes); verify(mockEventAppBuilder).build(); @@ -252,7 +266,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { } @Test - public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -266,6 +280,8 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -274,7 +290,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 +303,8 @@ public void testNonFatalEvent_addRolloutsEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + diskWriteWorker.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -417,35 +435,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/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 071a1aafa0c..e65714e8f8f 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 @@ -96,8 +96,8 @@ public class CrashlyticsCore { private final RemoteConfigDeferredProxy remoteConfigDeferredProxy; - @VisibleForTesting final CrashlyticsWorker commonWorker; - @VisibleForTesting final CrashlyticsWorker diskWriteWorker; + private final CrashlyticsWorker commonWorker; + private final CrashlyticsWorker diskWriteWorker; // region Constructors @@ -172,7 +172,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) stackTraceTrimmingStrategy, settingsProvider, onDemandCounter, - sessionsSubscriber); + sessionsSubscriber, + diskWriteWorker); controller = new CrashlyticsController( @@ -337,7 +338,11 @@ public void logException(@NonNull Throwable throwable) { */ public void log(final String msg) { final long timestamp = System.currentTimeMillis() - startTime; - commonWorker.submit(() -> controller.writeToLog(timestamp, msg)); + // queuing up on common worker to maintain the order + commonWorker.submit( + () -> { + diskWriteWorker.submit(() -> controller.writeToLog(timestamp, msg)); + }); } public void setUserId(String identifier) { 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/SessionReportingCoordinator.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java index 4018d3c0394..be56b950c16 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 @@ -23,6 +23,7 @@ import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.crashlytics.internal.CrashlyticsWorker; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.metadata.LogFileManager; import com.google.firebase.crashlytics.internal.metadata.UserMetadata; @@ -47,10 +48,10 @@ import java.util.concurrent.Executor; /** - * This class handles Crashlytics lifecycle events and coordinates session data capture and + * 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, + CrashlyticsWorker diskWriteWorker) { 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, + diskWriteWorker); } private final CrashlyticsReportDataCapture dataCapture; @@ -87,22 +95,25 @@ public static SessionReportingCoordinator create( private final UserMetadata reportMetadata; private final IdManager idManager; + private final CrashlyticsWorker diskWriteWorker; + SessionReportingCoordinator( CrashlyticsReportDataCapture dataCapture, CrashlyticsReportPersistence reportPersistence, DataTransportCrashlyticsReportSender reportsSender, LogFileManager logFileManager, UserMetadata reportMetadata, - IdManager idManager) { + IdManager idManager, + CrashlyticsWorker diskWriteWorker) { this.dataCapture = dataCapture; this.reportPersistence = reportPersistence; this.reportsSender = reportsSender; this.logFileManager = logFileManager; this.reportMetadata = reportMetadata; this.idManager = idManager; + this.diskWriteWorker = diskWriteWorker; } - @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) { + diskWriteWorker.submit( + () -> { + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); + }); + return; + } - reportPersistence.persistEvent(addMetaDataToEvent(capturedEvent), sessionId, isHighPriority); + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); } private boolean onReportSendComplete(@NonNull Task task) { 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..41284f0f7f6 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 @@ -28,6 +28,8 @@ import android.os.Build; import androidx.annotation.RequiresApi; import androidx.test.core.app.ApplicationProvider; +import com.google.firebase.concurrent.TestOnlyExecutors; +import com.google.firebase.crashlytics.internal.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; @@ -61,6 +63,8 @@ public class SessionReportingCoordinatorRobolectricTest { private SessionReportingCoordinator reportingCoordinator; + private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -72,7 +76,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + diskWriteWorker); mockEventInteractions(); }