Skip to content

Commit b0f1969

Browse files
mrwillis21mrwillis21
committed
Clean up error handling for report persistence
Ensure data written to disk is cleaned up properly when read/write/serialization errors occur.
1 parent 3f5146f commit b0f1969

18 files changed

+349
-218
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import android.content.SharedPreferences;
3535
import android.os.BatteryManager;
3636
import android.os.Bundle;
37+
import androidx.annotation.NonNull;
3738
import com.google.android.gms.tasks.Task;
3839
import com.google.android.gms.tasks.TaskCompletionSource;
3940
import com.google.android.gms.tasks.Tasks;
@@ -186,7 +187,7 @@ ControllerBuilder setReportUploader(ReportUploader uploader) {
186187
return setReportUploaderProvider(
187188
new ReportUploader.Provider() {
188189
@Override
189-
public ReportUploader createReportUploader(AppSettingsData settingsData) {
190+
public ReportUploader createReportUploader(@NonNull AppSettingsData settingsData) {
190191
return uploader;
191192
}
192193
});

‎firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/model/serialization/CrashlyticsReportJsonTransformTest.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.Event.Application.Execution.Thread.Frame;
2626
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.User;
2727
import com.google.firebase.crashlytics.internal.model.ImmutableList;
28+
import java.io.IOException;
2829
import java.util.ArrayList;
2930
import java.util.List;
3031
import org.junit.Before;
@@ -40,7 +41,7 @@ public void setUp() {
4041
}
4142

4243
@Test
43-
public void testReportToJsonAndBack_equals() {
44+
public void testReportToJsonAndBack_equals() throws IOException {
4445
final CrashlyticsReport testReport = makeTestReport();
4546
final String testReportJson = transform.reportToJson(testReport);
4647
final CrashlyticsReport reifiedReport = transform.reportFromJson(testReportJson);
@@ -49,7 +50,7 @@ public void testReportToJsonAndBack_equals() {
4950
}
5051

5152
@Test
52-
public void testEventToJsonAndBack_equals() {
53+
public void testEventToJsonAndBack_equals() throws IOException {
5354
final CrashlyticsReport.Session.Event testEvent = makeTestEvent();
5455
final String testEventJson = transform.eventToJson(testEvent);
5556
final CrashlyticsReport.Session.Event reifiedEvent = transform.eventFromJson(testEventJson);

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@
2222

2323
/** A {@link NativeSessionFile} backed by a byte array. */
2424
class BytesBackedNativeSessionFile implements NativeSessionFile {
25-
private final byte[] bytes;
26-
private final String dataTransportFilename;
27-
private final String reportsEndpointFilename;
25+
@Nullable private final byte[] bytes;
26+
@NonNull private final String dataTransportFilename;
27+
@NonNull private final String reportsEndpointFilename;
2828

2929
BytesBackedNativeSessionFile(
3030
@NonNull String dataTransportFilename,

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

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,9 @@ void enableExceptionHandling(
359359
new CrashlyticsUncaughtExceptionHandler.CrashListener() {
360360
@Override
361361
public void onUncaughtException(
362-
SettingsDataProvider settingsDataProvider, Thread thread, Throwable ex) {
362+
@NonNull SettingsDataProvider settingsDataProvider,
363+
@NonNull Thread thread,
364+
@NonNull Throwable ex) {
363365
handleUncaughtException(settingsDataProvider, thread, ex);
364366
}
365367
};
@@ -369,7 +371,9 @@ public void onUncaughtException(
369371
}
370372

371373
synchronized void handleUncaughtException(
372-
SettingsDataProvider settingsDataProvider, final Thread thread, final Throwable ex) {
374+
@NonNull SettingsDataProvider settingsDataProvider,
375+
@NonNull final Thread thread,
376+
@NonNull final Throwable ex) {
373377

374378
Logger.getLogger()
375379
.d(
@@ -422,6 +426,12 @@ public Task<Void> call() throws Exception {
422426
@Override
423427
public Task<Void> then(@Nullable AppSettingsData appSettingsData)
424428
throws Exception {
429+
if (appSettingsData == null) {
430+
Logger.getLogger()
431+
.d(
432+
"Did not receive app settings, cannot send reports at crash time.");
433+
return Tasks.forResult(null);
434+
}
425435
// Data collection is enabled, so it's safe to send the report.
426436
boolean dataCollectionToken = true;
427437
sendSessionReports(appSettingsData, dataCollectionToken);
@@ -567,6 +577,12 @@ public Task<Void> call() throws Exception {
567577
@Override
568578
public Task<Void> then(@Nullable AppSettingsData appSettingsData)
569579
throws Exception {
580+
if (appSettingsData == null) {
581+
Logger.getLogger()
582+
.d(
583+
"Did not receive app settings, cannot send reports during app startup.");
584+
return Tasks.forResult(null);
585+
}
570586
// Append the most recent org ID to each report file, even if it
571587
// was already appended during the crash time upload. This way
572588
// we'll always have the most recent value available attached.
@@ -597,7 +613,7 @@ public Task<Void> then(@Nullable AppSettingsData appSettingsData)
597613
private ReportUploader.Provider defaultReportUploader() {
598614
return new ReportUploader.Provider() {
599615
@Override
600-
public ReportUploader createReportUploader(AppSettingsData appSettingsData) {
616+
public ReportUploader createReportUploader(@NonNull AppSettingsData appSettingsData) {
601617
final String reportsUrl = appSettingsData.reportsUrl;
602618
final String ndkReportsUrl = appSettingsData.ndkReportsUrl;
603619
final String organizationId = appSettingsData.organizationId;
@@ -1135,7 +1151,8 @@ private static long getTimestampSeconds(Date date) {
11351151
}
11361152

11371153
/** Removes dashes in the Crashlytics session identifier to conform to Firebase constraints. */
1138-
private static String makeFirebaseSessionIdentifier(String sessionIdentifier) {
1154+
@NonNull
1155+
private static String makeFirebaseSessionIdentifier(@NonNull String sessionIdentifier) {
11391156
return sessionIdentifier.replaceAll("-", "");
11401157
}
11411158

@@ -1230,8 +1247,8 @@ private void writeSessionPartFile(
12301247
}
12311248
}
12321249

1233-
private static void appendToProtoFile(File file, CodedOutputStreamWriteAction writeAction)
1234-
throws Exception {
1250+
private static void appendToProtoFile(
1251+
@NonNull File file, @NonNull CodedOutputStreamWriteAction writeAction) throws Exception {
12351252
FileOutputStream fos = null;
12361253
CodedOutputStream cos = null;
12371254
try {
@@ -1592,8 +1609,11 @@ private void writeInitialPartsTo(CodedOutputStream cos, String sessionId) throws
15921609
}
15931610
}
15941611

1595-
private static void appendOrganizationIdToSessionFile(String organizationId, File file)
1596-
throws Exception {
1612+
private static void appendOrganizationIdToSessionFile(
1613+
@Nullable String organizationId, @NonNull File file) throws Exception {
1614+
if (organizationId == null) {
1615+
return;
1616+
}
15971617
appendToProtoFile(
15981618
file,
15991619
new CodedOutputStreamWriteAction() {
@@ -1698,7 +1718,7 @@ private CreateReportSpiCall getCreateReportSpiCall(String reportsUrl, String ndk
16981718
return new CompositeCreateReportSpiCall(defaultCreateReportSpiCall, nativeCreateReportSpiCall);
16991719
}
17001720

1701-
private void sendSessionReports(AppSettingsData appSettings, boolean dataCollectionToken)
1721+
private void sendSessionReports(@NonNull AppSettingsData appSettings, boolean dataCollectionToken)
17021722
throws Exception {
17031723
final Context context = getContext();
17041724
final ReportUploader reportUploader = reportUploaderProvider.createReportUploader(appSettings);
@@ -1842,6 +1862,7 @@ public void run() {
18421862
}
18431863
}
18441864

1865+
@NonNull
18451866
static List<NativeSessionFile> getNativeSessionFiles(
18461867
NativeSessionFileProvider fileProvider,
18471868
String previousSessionId,
@@ -1857,7 +1878,7 @@ static List<NativeSessionFile> getNativeSessionFiles(
18571878
try {
18581879
binaryImageBytes =
18591880
NativeFileUtils.binaryImagesJsonFromMapsFile(fileProvider.getBinaryImagesFile(), context);
1860-
} catch (IOException e) {
1881+
} catch (Exception e) {
18611882
// Keep processing, we'll add an empty binaryImages object.
18621883
}
18631884

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.crashlytics.internal.common;
1616

17+
import androidx.annotation.NonNull;
18+
1719
/** This class defines Crashlytics lifecycle events */
1820
interface CrashlyticsLifecycleEvents {
1921

@@ -22,7 +24,7 @@ interface CrashlyticsLifecycleEvents {
2224
*
2325
* @param sessionId the identifier for the new session
2426
*/
25-
void onBeginSession(String sessionId, long timestamp);
27+
void onBeginSession(@NonNull String sessionId, long timestamp);
2628

2729
/**
2830
* Called when a message is logged by the user.

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,13 @@ public CrashlyticsUncaughtExceptionHandler(
4646
public void uncaughtException(Thread thread, Throwable ex) {
4747
isHandlingException.set(true);
4848
try {
49-
crashListener.onUncaughtException(settingsDataProvider, thread, ex);
49+
if (thread == null) {
50+
Logger.getLogger().d("Could not handle uncaught exception; null thread");
51+
} else if (ex == null) {
52+
Logger.getLogger().d("Could not handle uncaught exception; null throwable");
53+
} else {
54+
crashListener.onUncaughtException(settingsDataProvider, thread, ex);
55+
}
5056
} catch (Exception e) {
5157
Logger.getLogger().e("An error occurred in the uncaught exception handler", e);
5258
} finally {

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
package com.google.firebase.crashlytics.internal.common;
1616

17+
import androidx.annotation.NonNull;
1718
import com.google.firebase.crashlytics.internal.settings.model.AppSettingsData;
1819

1920
public enum DataTransportState {
@@ -26,6 +27,7 @@ public enum DataTransportState {
2627
// Used to determine whether to upload reports through the new DataTransport API.
2728
static final int REPORT_UPLOAD_VARIANT_DATATRANSPORT = 2;
2829

30+
@NonNull
2931
static DataTransportState getState(boolean dataTransportState, boolean dataTransportNativeState) {
3032
if (!dataTransportState) {
3133
return NONE;
@@ -36,7 +38,8 @@ static DataTransportState getState(boolean dataTransportState, boolean dataTrans
3638
return ALL;
3739
}
3840

39-
static DataTransportState getState(AppSettingsData appSettingsData) {
41+
@NonNull
42+
static DataTransportState getState(@NonNull AppSettingsData appSettingsData) {
4043
final boolean dataTransportState =
4144
appSettingsData.reportUploadVariant == REPORT_UPLOAD_VARIANT_DATATRANSPORT;
4245
final boolean dataTransportNativeState =

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@
2727
/** A {@link NativeSessionFile} backed by a {@link File} currently on disk. */
2828
class FileBackedNativeSessionFile implements NativeSessionFile {
2929

30-
private final File file;
31-
private final String dataTransportFilename;
32-
private final String reportsEndpointFilename;
30+
@NonNull private final File file;
31+
@NonNull private final String dataTransportFilename;
32+
@NonNull private final String reportsEndpointFilename;
3333

3434
FileBackedNativeSessionFile(
3535
@NonNull String dataTransportFilename,

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
package com.google.firebase.crashlytics.internal.common;
1616

17+
import androidx.annotation.NonNull;
1718
import com.google.firebase.crashlytics.internal.Logger;
1819
import java.io.BufferedWriter;
1920
import java.io.File;
@@ -116,10 +117,12 @@ public Map<String, String> readKeyData(String sessionId) {
116117
return Collections.emptyMap();
117118
}
118119

120+
@NonNull
119121
public File getUserDataFileForSession(String sessionId) {
120122
return new File(filesDir, sessionId + USERDATA_SUFFIX + METADATA_EXT);
121123
}
122124

125+
@NonNull
123126
public File getKeysFileForSession(String sessionId) {
124127
return new File(filesDir, sessionId + KEYDATA_SUFFIX + METADATA_EXT);
125128
}

0 commit comments

Comments
 (0)