Clean up error handling for report persistence (#1387)

Ensure data written to disk is cleaned up properly when
read/write/serialization errors occur.

Author: mrwillis21

encoders/firebase-encoders-json/gradle.properties

@@ -12,5 +12,5 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-version=16.1.0
-latestReleasedVersion=16.0.0
+version=16.1.1
+latestReleasedVersion=16.1.0

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

@@ -34,6 +34,7 @@
 import android.content.SharedPreferences;
 import android.os.BatteryManager;
 import android.os.Bundle;
+import androidx.annotation.NonNull;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
@@ -186,7 +187,7 @@ ControllerBuilder setReportUploader(ReportUploader uploader) {
       return setReportUploaderProvider(
           new ReportUploader.Provider() {
             @Override
-            public ReportUploader createReportUploader(AppSettingsData settingsData) {
+            public ReportUploader createReportUploader(@NonNull AppSettingsData settingsData) {
               return uploader;
             }
           });

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

@@ -25,6 +25,7 @@
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.Event.Application.Execution.Thread.Frame;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.User;
 import com.google.firebase.crashlytics.internal.model.ImmutableList;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import org.junit.Before;
@@ -40,7 +41,7 @@ public void setUp() {
   }
 
   @Test
-  public void testReportToJsonAndBack_equals() {
+  public void testReportToJsonAndBack_equals() throws IOException {
     final CrashlyticsReport testReport = makeTestReport();
     final String testReportJson = transform.reportToJson(testReport);
     final CrashlyticsReport reifiedReport = transform.reportFromJson(testReportJson);
@@ -49,7 +50,7 @@ public void testReportToJsonAndBack_equals() {
   }
 
   @Test
-  public void testEventToJsonAndBack_equals() {
+  public void testEventToJsonAndBack_equals() throws IOException {
     final CrashlyticsReport.Session.Event testEvent = makeTestEvent();
     final String testEventJson = transform.eventToJson(testEvent);
     final CrashlyticsReport.Session.Event reifiedEvent = transform.eventFromJson(testEventJson);

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java

@@ -123,6 +123,10 @@ public static FirebaseCrashlytics getInstance() {
    * @param throwable a {@link Throwable} to be recorded as a non-fatal event.
    */
   public void recordException(@NonNull Throwable throwable) {
+    if (throwable == null) { // Users could call this with null despite the annotation.
+      Logger.getLogger().w("Crashlytics is ignoring a request to log a null exception.");
+      return;
+    }
     core.logException(throwable);
   }
 

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

@@ -22,9 +22,9 @@
 
 /** A {@link NativeSessionFile} backed by a byte array. */
 class BytesBackedNativeSessionFile implements NativeSessionFile {
-  private final byte[] bytes;
-  private final String dataTransportFilename;
-  private final String reportsEndpointFilename;
+  @Nullable private final byte[] bytes;
+  @NonNull private final String dataTransportFilename;
+  @NonNull private final String reportsEndpointFilename;
 
   BytesBackedNativeSessionFile(
       @NonNull String dataTransportFilename,

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

@@ -359,7 +359,9 @@ void enableExceptionHandling(
         new CrashlyticsUncaughtExceptionHandler.CrashListener() {
           @Override
           public void onUncaughtException(
-              SettingsDataProvider settingsDataProvider, Thread thread, Throwable ex) {
+              @NonNull SettingsDataProvider settingsDataProvider,
+              @NonNull Thread thread,
+              @NonNull Throwable ex) {
             handleUncaughtException(settingsDataProvider, thread, ex);
           }
         };
@@ -369,7 +371,9 @@ public void onUncaughtException(
   }
 
   synchronized void handleUncaughtException(
-      SettingsDataProvider settingsDataProvider, final Thread thread, final Throwable ex) {
+      @NonNull SettingsDataProvider settingsDataProvider,
+      @NonNull final Thread thread,
+      @NonNull final Throwable ex) {
 
     Logger.getLogger()
         .d(
@@ -422,6 +426,12 @@ public Task<Void> call() throws Exception {
                           @Override
                           public Task<Void> then(@Nullable AppSettingsData appSettingsData)
                               throws Exception {
+                            if (appSettingsData == null) {
+                              Logger.getLogger()
+                                  .w(
+                                      "Received null app settings, cannot send reports at crash time.");
+                              return Tasks.forResult(null);
+                            }
                             // Data collection is enabled, so it's safe to send the report.
                             boolean dataCollectionToken = true;
                             sendSessionReports(appSettingsData, dataCollectionToken);
@@ -567,6 +577,12 @@ public Task<Void> call() throws Exception {
                               @Override
                               public Task<Void> then(@Nullable AppSettingsData appSettingsData)
                                   throws Exception {
+                                if (appSettingsData == null) {
+                                  Logger.getLogger()
+                                      .w(
+                                          "Received null app settings, cannot send reports during app startup.");
+                                  return Tasks.forResult(null);
+                                }
                                 // Append the most recent org ID to each report file, even if it
                                 // was already appended during the crash time upload. This way
                                 // we'll always have the most recent value available attached.
@@ -597,7 +613,7 @@ public Task<Void> then(@Nullable AppSettingsData appSettingsData)
   private ReportUploader.Provider defaultReportUploader() {
     return new ReportUploader.Provider() {
       @Override
-      public ReportUploader createReportUploader(AppSettingsData appSettingsData) {
+      public ReportUploader createReportUploader(@NonNull AppSettingsData appSettingsData) {
         final String reportsUrl = appSettingsData.reportsUrl;
         final String ndkReportsUrl = appSettingsData.ndkReportsUrl;
         final String organizationId = appSettingsData.organizationId;
@@ -630,7 +646,7 @@ public Void call() throws Exception {
   }
 
   /** Log a caught exception - write out Throwable as event section of protobuf */
-  void writeNonFatalException(final Thread thread, final Throwable ex) {
+  void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwable ex) {
     // Capture and close over the current time, so that we get the exact call time,
     // rather than the time at which the task executes.
     final Date time = new Date();
@@ -660,7 +676,7 @@ void setCustomKey(String key, String value) {
       if (context != null && CommonUtils.isAppDebuggable(context)) {
         throw ex;
       } else {
-        Logger.getLogger().e("Attempting to set custom attribute with null key, ignoring.", null);
+        Logger.getLogger().e("Attempting to set custom attribute with null key, ignoring.");
         return;
       }
     }
@@ -1135,7 +1151,8 @@ private static long getTimestampSeconds(Date date) {
   }
 
   /** Removes dashes in the Crashlytics session identifier to conform to Firebase constraints. */
-  private static String makeFirebaseSessionIdentifier(String sessionIdentifier) {
+  @NonNull
+  private static String makeFirebaseSessionIdentifier(@NonNull String sessionIdentifier) {
     return sessionIdentifier.replaceAll("-", "");
   }
 
@@ -1152,7 +1169,7 @@ private void writeFatal(Thread thread, Throwable ex, long eventTime) {
       final String currentSessionId = getCurrentSessionId();
 
       if (currentSessionId == null) {
-        Logger.getLogger().e("Tried to write a fatal exception while no session was open.", null);
+        Logger.getLogger().e("Tried to write a fatal exception while no session was open.");
         return;
       }
 
@@ -1171,11 +1188,11 @@ private void writeFatal(Thread thread, Throwable ex, long eventTime) {
    * Not synchronized/locked. Must be executed from the single thread executor service used by this
    * class.
    */
-  private void doWriteNonFatal(Thread thread, Throwable ex, long eventTime) {
+  private void doWriteNonFatal(@NonNull Thread thread, @NonNull Throwable ex, long eventTime) {
     final String currentSessionId = getCurrentSessionId();
 
     if (currentSessionId == null) {
-      Logger.getLogger().e("Tried to write a non-fatal exception while no session was open.", null);
+      Logger.getLogger().d("Tried to write a non-fatal exception while no session was open.");
       return;
     }
 
@@ -1230,8 +1247,8 @@ private void writeSessionPartFile(
     }
   }
 
-  private static void appendToProtoFile(File file, CodedOutputStreamWriteAction writeAction)
-      throws Exception {
+  private static void appendToProtoFile(
+      @NonNull File file, @NonNull CodedOutputStreamWriteAction writeAction) throws Exception {
     FileOutputStream fos = null;
     CodedOutputStream cos = null;
     try {
@@ -1584,16 +1601,19 @@ private void writeInitialPartsTo(CodedOutputStream cos, String sessionId) throws
           listFilesMatching(new FileNameContainsFilter(sessionId + tag + SESSION_FILE_EXTENSION));
 
       if (sessionPartFiles.length == 0) {
-        Logger.getLogger().e("Can't find " + tag + " data for session ID " + sessionId, null);
+        Logger.getLogger().d("Can't find " + tag + " data for session ID " + sessionId);
       } else {
         Logger.getLogger().d("Collecting " + tag + " data for session ID " + sessionId);
         writeToCosFromFile(cos, sessionPartFiles[0]);
       }
     }
   }
 
-  private static void appendOrganizationIdToSessionFile(String organizationId, File file)
-      throws Exception {
+  private static void appendOrganizationIdToSessionFile(
+      @Nullable String organizationId, @NonNull File file) throws Exception {
+    if (organizationId == null) {
+      return;
+    }
     appendToProtoFile(
         file,
         new CodedOutputStreamWriteAction() {
@@ -1610,7 +1630,7 @@ public void writeTo(CodedOutputStream cos) throws Exception {
    */
   private static void writeToCosFromFile(CodedOutputStream cos, File file) throws IOException {
     if (!file.exists()) {
-      Logger.getLogger().e("Tried to include a file that doesn't exist: " + file.getName(), null);
+      Logger.getLogger().e("Tried to include a file that doesn't exist: " + file.getName());
       return;
     }
 
@@ -1698,7 +1718,7 @@ private CreateReportSpiCall getCreateReportSpiCall(String reportsUrl, String ndk
     return new CompositeCreateReportSpiCall(defaultCreateReportSpiCall, nativeCreateReportSpiCall);
   }
 
-  private void sendSessionReports(AppSettingsData appSettings, boolean dataCollectionToken)
+  private void sendSessionReports(@NonNull AppSettingsData appSettings, boolean dataCollectionToken)
       throws Exception {
     final Context context = getContext();
     final ReportUploader reportUploader = reportUploaderProvider.createReportUploader(appSettings);
@@ -1842,6 +1862,7 @@ public void run() {
     }
   }
 
+  @NonNull
   static List<NativeSessionFile> getNativeSessionFiles(
       NativeSessionFileProvider fileProvider,
       String previousSessionId,
@@ -1857,7 +1878,7 @@ static List<NativeSessionFile> getNativeSessionFiles(
     try {
       binaryImageBytes =
           NativeFileUtils.binaryImagesJsonFromMapsFile(fileProvider.getBinaryImagesFile(), context);
-    } catch (IOException e) {
+    } catch (Exception e) {
       // Keep processing, we'll add an empty binaryImages object.
     }
 

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

@@ -311,12 +311,7 @@ public static String getVersion() {
    * Throwable was thrown. The Throwable will always be processed on a background thread, so it is
    * safe to invoke this method from the main thread.
    */
-  public void logException(final Throwable throwable) {
-    if (throwable == null) {
-      Logger.getLogger().w("Crashlytics is ignoring a request to log a null exception.");
-      return;
-    }
-
+  public void logException(@NonNull Throwable throwable) {
     controller.writeNonFatalException(Thread.currentThread(), throwable);
   }
 

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

@@ -14,6 +14,8 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import androidx.annotation.NonNull;
+
 /** This class defines Crashlytics lifecycle events */
 interface CrashlyticsLifecycleEvents {
 
@@ -22,7 +24,7 @@ interface CrashlyticsLifecycleEvents {
    *
    * @param sessionId the identifier for the new session
    */
-  void onBeginSession(String sessionId, long timestamp);
+  void onBeginSession(@NonNull String sessionId, long timestamp);
 
   /**
    * Called when a message is logged by the user.

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

@@ -46,7 +46,13 @@ public CrashlyticsUncaughtExceptionHandler(
   public void uncaughtException(Thread thread, Throwable ex) {
     isHandlingException.set(true);
     try {
-      crashListener.onUncaughtException(settingsDataProvider, thread, ex);
+      if (thread == null) {
+        Logger.getLogger().e("Could not handle uncaught exception; null thread");
+      } else if (ex == null) {
+        Logger.getLogger().e("Could not handle uncaught exception; null throwable");
+      } else {
+        crashListener.onUncaughtException(settingsDataProvider, thread, ex);
+      }
     } catch (Exception e) {
       Logger.getLogger().e("An error occurred in the uncaught exception handler", e);
     } finally {

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

@@ -14,6 +14,7 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import androidx.annotation.NonNull;
 import com.google.firebase.crashlytics.internal.settings.model.AppSettingsData;
 
 public enum DataTransportState {
@@ -26,6 +27,7 @@ public enum DataTransportState {
   // Used to determine whether to upload reports through the new DataTransport API.
   static final int REPORT_UPLOAD_VARIANT_DATATRANSPORT = 2;
 
+  @NonNull
   static DataTransportState getState(boolean dataTransportState, boolean dataTransportNativeState) {
     if (!dataTransportState) {
       return NONE;
@@ -36,7 +38,8 @@ static DataTransportState getState(boolean dataTransportState, boolean dataTrans
     return ALL;
   }
 
-  static DataTransportState getState(AppSettingsData appSettingsData) {
+  @NonNull
+  static DataTransportState getState(@NonNull AppSettingsData appSettingsData) {
     final boolean dataTransportState =
         appSettingsData.reportUploadVariant == REPORT_UPLOAD_VARIANT_DATATRANSPORT;
     final boolean dataTransportNativeState =

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

@@ -27,9 +27,9 @@
 /** A {@link NativeSessionFile} backed by a {@link File} currently on disk. */
 class FileBackedNativeSessionFile implements NativeSessionFile {
 
-  private final File file;
-  private final String dataTransportFilename;
-  private final String reportsEndpointFilename;
+  @NonNull private final File file;
+  @NonNull private final String dataTransportFilename;
+  @NonNull private final String reportsEndpointFilename;
 
   FileBackedNativeSessionFile(
       @NonNull String dataTransportFilename,

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

@@ -14,6 +14,7 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import androidx.annotation.NonNull;
 import com.google.firebase.crashlytics.internal.Logger;
 import java.io.BufferedWriter;
 import java.io.File;
@@ -116,10 +117,12 @@ public Map<String, String> readKeyData(String sessionId) {
     return Collections.emptyMap();
   }
 
+  @NonNull
   public File getUserDataFileForSession(String sessionId) {
     return new File(filesDir, sessionId + USERDATA_SUFFIX + METADATA_EXT);
   }
 
+  @NonNull
   public File getKeysFileForSession(String sessionId) {
     return new File(filesDir, sessionId + KEYDATA_SUFFIX + METADATA_EXT);
   }