Don't send fatal uncaught exception if an NDK crash exists (#3020)

Our backend expects at most one fatal event per session. In some cases,
such as Unity, the native signal handler may cause an uncaught exception
to be thrown, after we have recorded the NDK crash. If this happens,
Crashlytics should ignore the uncaught exception and send the NDK
crash information as the fatal event.

Author: mrichards

firebase-crashlytics-ndk/src/main/java/com/google/firebase/crashlytics/ndk/FirebaseCrashlyticsNdk.java

@@ -49,6 +49,7 @@ private interface SignalHandlerInstaller {
   }
 
   private boolean installHandlerDuringPrepareSession;
+  private String currentSessionId;
   private SignalHandlerInstaller signalHandlerInstaller;
 
   FirebaseCrashlyticsNdk(
@@ -57,6 +58,11 @@ private interface SignalHandlerInstaller {
     this.installHandlerDuringPrepareSession = installHandlerDuringPrepareSession;
   }
 
+  @Override
+  public boolean hasCrashDataForCurrentSession() {
+    return currentSessionId != null && hasCrashDataForSession(currentSessionId);
+  }
+
   @Override
   public boolean hasCrashDataForSession(@NonNull String sessionId) {
     return controller.hasCrashDataForSession(sessionId);
@@ -74,6 +80,7 @@ public synchronized void prepareNativeSession(
       long startedAtSeconds,
       @NonNull StaticSessionData sessionData) {
 
+    currentSessionId = sessionId;
     signalHandlerInstaller =
         () -> {
           Logger.getLogger().d("Initializing native session: " + sessionId);

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsNativeComponent.java

@@ -19,6 +19,8 @@
 
 public interface CrashlyticsNativeComponent {
 
+  boolean hasCrashDataForCurrentSession();
+
   boolean hasCrashDataForSession(@NonNull String sessionId);
 
   /**

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsNativeComponentDeferredProxy.java

@@ -40,6 +40,12 @@ public CrashlyticsNativeComponentDeferredProxy(
         });
   }
 
+  @Override
+  public boolean hasCrashDataForCurrentSession() {
+    CrashlyticsNativeComponent component = availableNativeComponent.get();
+    return component != null && component.hasCrashDataForCurrentSession();
+  }
+
   @Override
   public boolean hasCrashDataForSession(@NonNull String sessionId) {
     CrashlyticsNativeComponent component = availableNativeComponent.get();

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

@@ -156,7 +156,8 @@ public void onUncaughtException(
           }
         };
     crashHandler =
-        new CrashlyticsUncaughtExceptionHandler(crashListener, settingsProvider, defaultHandler);
+        new CrashlyticsUncaughtExceptionHandler(
+            crashListener, settingsProvider, defaultHandler, nativeComponent);
     Thread.setDefaultUncaughtExceptionHandler(crashHandler);
   }
 

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

@@ -14,44 +14,47 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent;
 import com.google.firebase.crashlytics.internal.Logger;
 import com.google.firebase.crashlytics.internal.settings.SettingsDataProvider;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 class CrashlyticsUncaughtExceptionHandler implements Thread.UncaughtExceptionHandler {
 
   interface CrashListener {
+
     void onUncaughtException(
         SettingsDataProvider settingsDataProvider, Thread thread, Throwable ex);
   }
 
   private final CrashListener crashListener;
   private final SettingsDataProvider settingsDataProvider;
   private final Thread.UncaughtExceptionHandler defaultHandler;
+  private final CrashlyticsNativeComponent nativeComponent;
 
   // use AtomicBoolean because value is accessible from other threads.
   private final AtomicBoolean isHandlingException;
 
   public CrashlyticsUncaughtExceptionHandler(
       CrashListener crashListener,
       SettingsDataProvider settingsProvider,
-      Thread.UncaughtExceptionHandler defaultHandler) {
+      Thread.UncaughtExceptionHandler defaultHandler,
+      CrashlyticsNativeComponent nativeComponent) {
     this.crashListener = crashListener;
     this.settingsDataProvider = settingsProvider;
     this.defaultHandler = defaultHandler;
     this.isHandlingException = new AtomicBoolean(false);
+    this.nativeComponent = nativeComponent;
   }
 
   @Override
   public void uncaughtException(Thread thread, Throwable ex) {
     isHandlingException.set(true);
     try {
-      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 {
+      if (shouldRecordUncaughtException(thread, ex)) {
         crashListener.onUncaughtException(settingsDataProvider, thread, ex);
+      } else {
+        Logger.getLogger().d("Uncaught exception will not be recorded by Crashlytics.");
       }
     } catch (Exception e) {
       Logger.getLogger().e("An error occurred in the uncaught exception handler", e);
@@ -65,4 +68,31 @@ public void uncaughtException(Thread thread, Throwable ex) {
   boolean isHandlingException() {
     return isHandlingException.get();
   }
+
+  /**
+   * Returns true if Crashlytics should record this exception. The decision to record is different
+   * than the decision to report the exception to Crashlytics servers, which is handled by the
+   * {@link DataCollectionArbiter}
+   *
+   * @return false if the thread or exception is null, or if a native crash already exists for this
+   *     session.
+   */
+  private boolean shouldRecordUncaughtException(Thread thread, Throwable ex) {
+    if (thread == null) {
+      Logger.getLogger().e("Crashlytics will not record uncaught exception; null thread");
+      return false;
+    }
+    if (ex == null) {
+      Logger.getLogger().e("Crashlytics will not record uncaught exception; null throwable");
+      return false;
+    }
+    // We should only report at most one fatal event for a session. If a native fatal already exists
+    // for this session, ignore the uncaught exception
+    if (nativeComponent.hasCrashDataForCurrentSession()) {
+      Logger.getLogger()
+          .d("Crashlytics will not record uncaught exception; native crash exists for session.");
+      return false;
+    }
+    return true;
+  }
 }