Skip to content

Commit 96e8ad8

Browse files
themiswangthemiswang
committed
Fix set user id for on-demand fatal
1 parent dff55b6 commit 96e8ad8

3 files changed

Lines changed: 68 additions & 6 deletions

File tree

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

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
3939
import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
4040
import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
41+
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
4142
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
4243
import com.google.firebase.crashlytics.internal.persistence.FileStore;
4344
import com.google.firebase.crashlytics.internal.settings.Settings;
@@ -52,6 +53,8 @@
5253
import java.util.TreeSet;
5354
import java.util.concurrent.Executor;
5455
import java.util.concurrent.TimeUnit;
56+
import org.junit.Test;
57+
import org.mockito.AdditionalMatchers;
5558
import org.mockito.ArgumentCaptor;
5659

5760
public class CrashlyticsControllerTest extends CrashlyticsTestCase {
@@ -101,22 +104,33 @@ private class ControllerBuilder {
101104
private CrashlyticsNativeComponent nativeComponent = null;
102105
private AnalyticsEventLogger analyticsEventLogger;
103106
private SessionReportingCoordinator sessionReportingCoordinator;
107+
108+
private CrashlyticsBackgroundWorker backgroundWorker;
104109
private LogFileManager logFileManager = null;
105110

111+
private UserMetadata userMetadata = null;
112+
106113
ControllerBuilder() {
107114
dataCollectionArbiter = mockDataCollectionArbiter;
108115
nativeComponent = mockNativeComponent;
109116

110117
analyticsEventLogger = mock(AnalyticsEventLogger.class);
111118

112119
sessionReportingCoordinator = mockSessionReportingCoordinator;
120+
121+
backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService());
113122
}
114123

115124
ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) {
116125
dataCollectionArbiter = arbiter;
117126
return this;
118127
}
119128

129+
ControllerBuilder setUserMetadata(UserMetadata userMetadata) {
130+
this.userMetadata = userMetadata;
131+
return this;
132+
}
133+
120134
public ControllerBuilder setNativeComponent(CrashlyticsNativeComponent nativeComponent) {
121135
this.nativeComponent = nativeComponent;
122136
return this;
@@ -153,13 +167,13 @@ public CrashlyticsController build() {
153167
final CrashlyticsController controller =
154168
new CrashlyticsController(
155169
testContext.getApplicationContext(),
156-
new CrashlyticsBackgroundWorker(new SameThreadExecutorService()),
170+
backgroundWorker,
157171
idManager,
158172
dataCollectionArbiter,
159173
testFileStore,
160174
crashMarker,
161175
appData,
162-
null,
176+
userMetadata,
163177
logFileManager,
164178
sessionReportingCoordinator,
165179
nativeComponent,
@@ -210,6 +224,26 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th
210224
.persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong());
211225
}
212226

227+
@Test
228+
public void testOnDemandFatal_callLogFatalException() {
229+
Thread thread = Thread.currentThread();
230+
Exception fatal = new RuntimeException("Fatal");
231+
Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class);
232+
UserMetadata mockUserMetadata = mock(UserMetadata.class);
233+
when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
234+
.thenReturn(new TreeSet<>(Collections.singleton(SESSION_ID)).descendingSet());
235+
236+
final CrashlyticsController controller =
237+
builder()
238+
.setLogFileManager(new LogFileManager(testFileStore))
239+
.setUserMetadata(mockUserMetadata)
240+
.build();
241+
controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider);
242+
controller.logFatalException(thread, fatal);
243+
244+
verify(mockUserMetadata).setNewSession(AdditionalMatchers.not(eq(SESSION_ID)));
245+
}
246+
213247
public void testNativeCrashDataCausesNativeReport() throws Exception {
214248
final String sessionId = "sessionId_1_new";
215249
final String previousSessionId = "sessionId_0_previous";

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ public Task<Void> call() throws Exception {
214214

215215
doWriteAppExceptionMarker(timestampMillis);
216216
doCloseSessions(settingsProvider);
217-
doOpenSession(new CLSUUID(idManager).toString());
217+
doOpenSession(new CLSUUID(idManager).toString(), isOnDemand);
218218

219219
// If automatic data collection is disabled, we'll need to wait until the next run
220220
// of the app.
@@ -496,7 +496,7 @@ void openSession(String sessionIdentifier) {
496496
new Callable<Void>() {
497497
@Override
498498
public Void call() throws Exception {
499-
doOpenSession(sessionIdentifier);
499+
doOpenSession(sessionIdentifier, false);
500500
return null;
501501
}
502502
});
@@ -548,7 +548,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) {
548548
* Not synchronized/locked. Must be executed from the single thread executor service used by this
549549
* class.
550550
*/
551-
private void doOpenSession(String sessionIdentifier) {
551+
private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) {
552552
final long startedAtSeconds = getCurrentTimestampSeconds();
553553

554554
Logger.getLogger().d("Opening a new session with ID " + sessionIdentifier);
@@ -566,6 +566,14 @@ private void doOpenSession(String sessionIdentifier) {
566566
startedAtSeconds,
567567
StaticSessionData.create(appData, osData, deviceData));
568568

569+
// If is on-demand fatal, we need to update the session id for userMetadata
570+
// as well(since we don't really change the object to a new one for a new session).
571+
// all the information in the previous session is still in memory, but we do need to
572+
// manually writing them into persistence for the new session.
573+
if (isOnDemand && sessionIdentifier != null) {
574+
userMetadata.setNewSession(sessionIdentifier);
575+
}
576+
569577
logFileManager.setCurrentSession(sessionIdentifier);
570578
reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds);
571579
}

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public class UserMetadata {
4141

4242
private final MetaDataStore metaDataStore;
4343
private final CrashlyticsBackgroundWorker backgroundWorker;
44-
private final String sessionIdentifier;
44+
private String sessionIdentifier;
4545

4646
// The following references contain a marker bit, which is true if the data maintained in the
4747
// associated reference has been serialized since the last time it was updated.
@@ -77,6 +77,26 @@ public UserMetadata(
7777
this.backgroundWorker = backgroundWorker;
7878
}
7979

80+
/**
81+
* Refresh the userMetadata to reflect the status of the new session. This API is mainly for
82+
* on-demand fatal feature since we need to close and update to a new session. UserMetadata also
83+
* need to make this update instead of updating session id, we also need to manually writing the
84+
* into persistence for the new session.
85+
*/
86+
public void setNewSession(String sessionId) {
87+
synchronized (sessionIdentifier) {
88+
sessionIdentifier = sessionId;
89+
backgroundWorker.submit(
90+
() -> {
91+
if (getUserId() != null) {
92+
metaDataStore.writeUserData(sessionIdentifier, getUserId());
93+
}
94+
// TODO(themis): adding feature rollouts later
95+
return null;
96+
});
97+
}
98+
}
99+
80100
@Nullable
81101
public String getUserId() {
82102
return userId.getReference();

0 commit comments

Comments
 (0)