Skip to content

Commit 4714311

Browse files
authored
Remove session ID tracking from session reporting coordinator. (firebase#1980)
Multi-process apps can have multiple instances, which can result in one instance having stale state. Relying on the passed-in session ID as the source of truth ensures the SRC is always operating on the most recent open session.
1 parent 0977571 commit 4714311

File tree

5 files changed

+104
-93
lines changed

5 files changed

+104
-93
lines changed

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

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ public void testFatalEvent_persistsHighPriorityEventForSessionId() {
9595
mockEventInteractions();
9696

9797
reportManager.onBeginSession(sessionId, timestamp);
98-
reportManager.persistFatalEvent(mockException, mockThread, timestamp);
98+
reportManager.persistFatalEvent(mockException, mockThread, sessionId, timestamp);
9999

100100
verify(dataCapture)
101101
.captureEventData(mockException, mockThread, eventType, timestamp, 4, 8, true);
@@ -111,7 +111,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventForSessionId() {
111111
mockEventInteractions();
112112

113113
reportManager.onBeginSession(sessionId, timestamp);
114-
reportManager.persistNonFatalEvent(mockException, mockThread, timestamp);
114+
reportManager.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
115115

116116
verify(dataCapture)
117117
.captureEventData(mockException, mockThread, eventType, timestamp, 4, 8, false);
@@ -124,12 +124,13 @@ public void testNonFatalEvent_addsLogsToEvent() {
124124

125125
mockEventInteractions();
126126

127+
final String sessionId = "testSessionId";
127128
final String testLog = "test\nlog";
128129

129130
when(logFileManager.getLogString()).thenReturn(testLog);
130131

131-
reportManager.onBeginSession("testSessionId", timestamp);
132-
reportManager.persistNonFatalEvent(mockException, mockThread, timestamp);
132+
reportManager.onBeginSession(sessionId, timestamp);
133+
reportManager.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
133134

134135
verify(mockEventBuilder)
135136
.setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build());
@@ -143,10 +144,12 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
143144

144145
mockEventInteractions();
145146

147+
final String sessionId = "testSessionId";
148+
146149
when(logFileManager.getLogString()).thenReturn(null);
147150

148-
reportManager.onBeginSession("testSessionId", timestamp);
149-
reportManager.persistNonFatalEvent(mockException, mockThread, timestamp);
151+
reportManager.onBeginSession(sessionId, timestamp);
152+
reportManager.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
150153

151154
verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class));
152155
verify(mockEventBuilder).build();
@@ -159,12 +162,13 @@ public void testFatalEvent_addsLogsToEvent() {
159162

160163
mockEventInteractions();
161164

165+
final String sessionId = "testSessionId";
162166
final String testLog = "test\nlog";
163167

164168
when(logFileManager.getLogString()).thenReturn(testLog);
165169

166-
reportManager.onBeginSession("testSessionId", timestamp);
167-
reportManager.persistFatalEvent(mockException, mockThread, timestamp);
170+
reportManager.onBeginSession(sessionId, timestamp);
171+
reportManager.persistFatalEvent(mockException, mockThread, sessionId, timestamp);
168172

169173
verify(mockEventBuilder)
170174
.setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build());
@@ -178,10 +182,12 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
178182

179183
mockEventInteractions();
180184

185+
final String sessionId = "testSessionId";
186+
181187
when(logFileManager.getLogString()).thenReturn(null);
182188

183-
reportManager.onBeginSession("testSessionId", timestamp);
184-
reportManager.persistFatalEvent(mockException, mockThread, timestamp);
189+
reportManager.onBeginSession(sessionId, timestamp);
190+
reportManager.persistFatalEvent(mockException, mockThread, sessionId, timestamp);
185191

186192
verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class));
187193
verify(mockEventBuilder).build();
@@ -194,6 +200,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
194200

195201
mockEventInteractions();
196202

203+
final String sessionId = "testSessionId";
204+
197205
final String testKey1 = "testKey1";
198206
final String testValue1 = "testValue1";
199207
final String testKey2 = "testKey2";
@@ -213,8 +221,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
213221

214222
when(reportMetadata.getCustomKeys()).thenReturn(attributes);
215223

216-
reportManager.onBeginSession("testSessionId", timestamp);
217-
reportManager.persistNonFatalEvent(mockException, mockThread, timestamp);
224+
reportManager.onBeginSession(sessionId, timestamp);
225+
reportManager.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
218226

219227
verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes);
220228
verify(mockEventAppBuilder).build();
@@ -229,12 +237,14 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
229237

230238
mockEventInteractions();
231239

240+
final String sessionId = "testSessionId";
241+
232242
final Map<String, String> attributes = Collections.emptyMap();
233243

234244
when(reportMetadata.getCustomKeys()).thenReturn(attributes);
235245

236-
reportManager.onBeginSession("testSessionId", timestamp);
237-
reportManager.persistNonFatalEvent(mockException, mockThread, timestamp);
246+
reportManager.onBeginSession(sessionId, timestamp);
247+
reportManager.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
238248

239249
verify(mockEventAppBuilder, never()).setCustomAttributes(any());
240250
verify(mockEventAppBuilder, never()).build();
@@ -249,6 +259,8 @@ public void testFatalEvent_addsSortedKeysToEvent() {
249259

250260
mockEventInteractions();
251261

262+
final String sessionId = "testSessionId";
263+
252264
final String testKey1 = "testKey1";
253265
final String testValue1 = "testValue1";
254266
final String testKey2 = "testKey2";
@@ -268,8 +280,8 @@ public void testFatalEvent_addsSortedKeysToEvent() {
268280

269281
when(reportMetadata.getCustomKeys()).thenReturn(attributes);
270282

271-
reportManager.onBeginSession("testSessionId", timestamp);
272-
reportManager.persistFatalEvent(mockException, mockThread, timestamp);
283+
reportManager.onBeginSession(sessionId, timestamp);
284+
reportManager.persistFatalEvent(mockException, mockThread, sessionId, timestamp);
273285

274286
verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes);
275287
verify(mockEventAppBuilder).build();
@@ -284,12 +296,14 @@ public void testFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
284296

285297
mockEventInteractions();
286298

299+
final String sessionId = "testSessionId";
300+
287301
final Map<String, String> attributes = Collections.emptyMap();
288302

289303
when(reportMetadata.getCustomKeys()).thenReturn(attributes);
290304

291-
reportManager.onBeginSession("testSessionId", timestamp);
292-
reportManager.persistFatalEvent(mockException, mockThread, timestamp);
305+
reportManager.onBeginSession(sessionId, timestamp);
306+
reportManager.persistFatalEvent(mockException, mockThread, sessionId, timestamp);
293307

294308
verify(mockEventAppBuilder, never()).setCustomAttributes(any());
295309
verify(mockEventAppBuilder, never()).build();
@@ -352,7 +366,7 @@ public void onSessionsFinalize_finalizesReports() {
352366
final String sessionId = "testSessionId";
353367
reportManager.onBeginSession(sessionId, System.currentTimeMillis());
354368
final long endedAt = System.currentTimeMillis();
355-
reportManager.finalizeSessions(endedAt);
369+
reportManager.finalizeSessions(endedAt, sessionId);
356370

357371
verify(reportPersistence).finalizeReports(sessionId, endedAt);
358372
}
@@ -435,7 +449,7 @@ public void testPersistUserIdForCurrentSession_persistsCurrentUserIdForCurrentSe
435449
when(reportMetadata.getUserId()).thenReturn(userId);
436450

437451
reportManager.onBeginSession(currentSessionId, timestamp);
438-
reportManager.persistUserId();
452+
reportManager.persistUserId(currentSessionId);
439453

440454
verify(reportPersistence).persistUserIdForSession(userId, currentSessionId);
441455
}

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

Lines changed: 54 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -388,13 +388,22 @@ synchronized void handleUncaughtException(
388388
new Callable<Task<Void>>() {
389389
@Override
390390
public Task<Void> call() throws Exception {
391+
final long timestampSeconds = getTimestampSeconds(time);
392+
393+
final String currentSessionId = getCurrentSessionId();
394+
if (currentSessionId == null) {
395+
Logger.getLogger()
396+
.e("Tried to write a fatal exception while no session was open.");
397+
return Tasks.forResult(null);
398+
}
399+
391400
// We've fatally crashed, so write the marker file that indicates a crash occurred.
392401
crashMarker.create();
393402

394-
long timestampSeconds = getTimestampSeconds(time);
395-
reportingCoordinator.persistFatalEvent(ex, thread, timestampSeconds);
396-
writeFatal(thread, ex, timestampSeconds);
397-
writeAppExceptionMarker(time.getTime());
403+
reportingCoordinator.persistFatalEvent(
404+
ex, thread, makeFirebaseSessionIdentifier(currentSessionId), timestampSeconds);
405+
doWriteFatal(thread, ex, currentSessionId, timestampSeconds);
406+
doWriteAppExceptionMarker(time.getTime());
398407

399408
Settings settings = settingsDataProvider.getSettings();
400409
int maxCustomExceptionEvents = settings.getSessionData().maxCustomExceptionEvents;
@@ -652,8 +661,15 @@ void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwab
652661
public void run() {
653662
if (!isHandlingException()) {
654663
long timestampSeconds = getTimestampSeconds(time);
655-
reportingCoordinator.persistNonFatalEvent(ex, thread, timestampSeconds);
656-
doWriteNonFatal(thread, ex, timestampSeconds);
664+
final String currentSessionId = getCurrentSessionId();
665+
if (currentSessionId == null) {
666+
Logger.getLogger()
667+
.d("Tried to write a non-fatal exception while no session was open.");
668+
return;
669+
}
670+
reportingCoordinator.persistNonFatalEvent(
671+
ex, thread, makeFirebaseSessionIdentifier(currentSessionId), timestampSeconds);
672+
doWriteNonFatal(thread, ex, currentSessionId, timestampSeconds);
657673
}
658674
}
659675
});
@@ -690,8 +706,12 @@ private void cacheUserData(final UserMetadata userMetaData) {
690706
new Callable<Void>() {
691707
@Override
692708
public Void call() throws Exception {
693-
reportingCoordinator.persistUserId();
694709
final String currentSessionId = getCurrentSessionId();
710+
if (currentSessionId == null) {
711+
Logger.getLogger().d("Tried to cache user data while no session was open.");
712+
return null;
713+
}
714+
reportingCoordinator.persistUserId(makeFirebaseSessionIdentifier(currentSessionId));
695715
new MetaDataStore(getFilesDir()).writeUserData(currentSessionId, userMetaData);
696716
return null;
697717
}
@@ -739,21 +759,14 @@ public Void call() throws Exception {
739759
*
740760
* <p>May return <code>null</code> if no session begin file is present.
741761
*/
762+
@Nullable
742763
private String getCurrentSessionId() {
743764
final File[] sessionBeginFiles = listSortedSessionBeginFiles();
744765
return (sessionBeginFiles.length > 0)
745766
? getSessionIdFromSessionFile(sessionBeginFiles[0])
746767
: null;
747768
}
748769

749-
/** @return */
750-
private String getPreviousSessionId() {
751-
final File[] sessionBeginFiles = listSortedSessionBeginFiles();
752-
return (sessionBeginFiles.length > 1)
753-
? getSessionIdFromSessionFile(sessionBeginFiles[1])
754-
: null;
755-
}
756-
757770
/**
758771
* Returns the session ID that forms the beginning part of the name of the provided file
759772
*
@@ -790,7 +803,7 @@ boolean finalizeSessions(int maxCustomExceptionEvents) {
790803

791804
Logger.getLogger().d("Finalizing previously open sessions.");
792805
try {
793-
doCloseSessions(maxCustomExceptionEvents, false);
806+
doCloseSessions(maxCustomExceptionEvents, true);
794807
} catch (Exception e) {
795808
Logger.getLogger().e("Unable to finalize previously open sessions.", e);
796809
return false;
@@ -823,16 +836,16 @@ private void doOpenSession() throws Exception {
823836
}
824837

825838
void doCloseSessions(int maxCustomExceptionEvents) throws Exception {
826-
doCloseSessions(maxCustomExceptionEvents, true);
839+
doCloseSessions(maxCustomExceptionEvents, false);
827840
}
828841

829842
/**
830843
* Not synchronized/locked. Must be executed from the single thread executor service used by this
831844
* class.
832845
*/
833-
private void doCloseSessions(int maxCustomExceptionEvents, boolean includeCurrent)
846+
private void doCloseSessions(int maxCustomExceptionEvents, boolean skipCurrentSession)
834847
throws Exception {
835-
final int offset = includeCurrent ? 0 : 1;
848+
final int offset = skipCurrentSession ? 1 : 0;
836849

837850
trimOpenSessions(MAX_OPEN_SESSIONS + offset);
838851

@@ -850,9 +863,7 @@ private void doCloseSessions(int maxCustomExceptionEvents, boolean includeCurren
850863
// maximum chance that the user code that sets this information has been run.
851864
writeSessionUser(mostRecentSessionIdToClose);
852865

853-
if (includeCurrent) {
854-
reportingCoordinator.onEndSession();
855-
} else if (nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) {
866+
if (nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) {
856867
// We only finalize the current session if it's a Java crash, so only finalize native crash
857868
// data when we aren't including current.
858869
finalizePreviousNativeSession(mostRecentSessionIdToClose);
@@ -863,7 +874,13 @@ private void doCloseSessions(int maxCustomExceptionEvents, boolean includeCurren
863874

864875
closeOpenSessions(sessionBeginFiles, offset, maxCustomExceptionEvents);
865876

866-
reportingCoordinator.finalizeSessions(getCurrentTimestampSeconds());
877+
String currentSessionId = null;
878+
if (skipCurrentSession) {
879+
currentSessionId =
880+
makeFirebaseSessionIdentifier(getSessionIdFromSessionFile(sessionBeginFiles[0]));
881+
}
882+
883+
reportingCoordinator.finalizeSessions(getCurrentTimestampSeconds(), currentSessionId);
867884
}
868885

869886
/**
@@ -942,11 +959,11 @@ private File[] listFilesMatching(FilenameFilter filter) {
942959
return listFilesMatching(getFilesDir(), filter);
943960
}
944961

945-
private File[] listFilesMatching(File directory, FilenameFilter filter) {
962+
private static File[] listFilesMatching(File directory, FilenameFilter filter) {
946963
return ensureFileArrayNotNull(directory.listFiles(filter));
947964
}
948965

949-
private File[] ensureFileArrayNotNull(File[] files) {
966+
private static File[] ensureFileArrayNotNull(File[] files) {
950967
return (files == null) ? new File[] {} : files;
951968
}
952969

@@ -1117,7 +1134,7 @@ private void finalizePreviousNativeSession(String previousSessionId) {
11171134
return;
11181135
}
11191136

1120-
writeAppExceptionMarker(eventTime);
1137+
doWriteAppExceptionMarker(eventTime);
11211138
List<NativeSessionFile> nativeSessionFiles =
11221139
getNativeSessionFiles(
11231140
nativeSessionFileProvider,
@@ -1151,17 +1168,14 @@ private static String makeFirebaseSessionIdentifier(@NonNull String sessionIdent
11511168
* Not synchronized/locked. Must be executed from the single thread executor service used by this
11521169
* class.
11531170
*/
1154-
private void writeFatal(Thread thread, Throwable ex, long eventTime) {
1171+
private void doWriteFatal(
1172+
@NonNull Thread thread,
1173+
@NonNull Throwable ex,
1174+
@NonNull String currentSessionId,
1175+
long eventTime) {
11551176
ClsFileOutputStream fos = null;
11561177
CodedOutputStream cos = null;
11571178
try {
1158-
final String currentSessionId = getCurrentSessionId();
1159-
1160-
if (currentSessionId == null) {
1161-
Logger.getLogger().e("Tried to write a fatal exception while no session was open.");
1162-
return;
1163-
}
1164-
11651179
fos = new ClsFileOutputStream(getFilesDir(), currentSessionId + SESSION_FATAL_TAG);
11661180
cos = CodedOutputStream.newInstance(fos);
11671181
writeSessionEvent(cos, thread, ex, eventTime, EVENT_TYPE_CRASH, true);
@@ -1173,7 +1187,7 @@ private void writeFatal(Thread thread, Throwable ex, long eventTime) {
11731187
}
11741188
}
11751189

1176-
private void writeAppExceptionMarker(long eventTime) {
1190+
private void doWriteAppExceptionMarker(long eventTime) {
11771191
try {
11781192
new File(getFilesDir(), APP_EXCEPTION_MARKER_PREFIX + eventTime).createNewFile();
11791193
} catch (IOException e) {
@@ -1185,14 +1199,11 @@ private void writeAppExceptionMarker(long eventTime) {
11851199
* Not synchronized/locked. Must be executed from the single thread executor service used by this
11861200
* class.
11871201
*/
1188-
private void doWriteNonFatal(@NonNull Thread thread, @NonNull Throwable ex, long eventTime) {
1189-
final String currentSessionId = getCurrentSessionId();
1190-
1191-
if (currentSessionId == null) {
1192-
Logger.getLogger().d("Tried to write a non-fatal exception while no session was open.");
1193-
return;
1194-
}
1195-
1202+
private void doWriteNonFatal(
1203+
@NonNull Thread thread,
1204+
@NonNull Throwable ex,
1205+
@NonNull String currentSessionId,
1206+
long eventTime) {
11961207
ClsFileOutputStream fos = null;
11971208
CodedOutputStream cos = null;
11981209
try {

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,4 @@ interface CrashlyticsLifecycleEvents {
4848
* @param userId
4949
*/
5050
void onUserId(String userId);
51-
52-
/** Called when the current session should be closed */
53-
void onEndSession();
5451
}

0 commit comments

Comments
 (0)