Firebase-android-sdk: CrashlyticsUncaughtExceptionHandler was giving control to com.android.internal.os.RuntimeInit.KillApplicationHandler, hence bypassing any custom Thread.UncaughtExceptionHandler being set by apps

[amirulzin]

[READ] Step 1: Are you in the right place?

Yup

[REQUIRED] Step 2: Describe your environment

• Android Studio version: 4.0.1 (Stable)
• Firebase Component:
  
  
  
  • com.google.firebase:firebase-core:17.5.0
  
  
  • com.google.firebase:firebase-crashlytics:17.2.1
  
  

[REQUIRED] Step 3: Describe the problem

1. Project was using Fabric.
2. Migrated to Firebase Crashlytics, following instruction on the official Firebase migration doc, _verbatim_
3. Test a crash.
4. CrashlyticsUncaughtExceptionHandler wasn't returning the control back to our own custom UncaughtExceptionHandler.

Steps to reproduce:

1. Add Firebase Crashlytics to a new Android project
2. Create a custom UncaughtExceptionHandler and init that handler in Application.onCreate() e.g.

YourApplication.class

    @Override
    public void onCreate() {
          ...
          YourCustomUncaughtExceptionHandler.init(this);
          ...
     }

```java

public class YourCustomUncaughtExceptionHandler implements UncaughtExceptionHandler {

public void init(Context appContext){ 
       ....
       defaultHandler = Thread.getDefaultUncaughtExceptionHandler(); //This now an instance CrashlyticsUncaughtExceptionHandler, as per debugger
       ....
}
@Override
public void uncaughtException(Thread thread, Throwable ex) {
     defaultHandler.uncaughtException(thread, ex); //Doesn't return control to you

     //any handling below the line above won't be called
     ....
    Log.e("TAG", "Hello, Ground Control");
}

3. Force a crash.


#### Relevant Code:

I saw few similar issues in here. After a whole cold day in the rain with the debugger, I think I see the culprit. 

So here's a quick mental config:

1. Firebase does its initialization via `ContentProvider` (see CrashlyticsRegistrar), and it all goes to [FirebaseCrashlytics.init()](https://github.com/firebase/firebase-android-sdk/blob/master/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java#L61).

2. This continue to init `CrashlyticsUncaughtExceptionHandler` in [CrashlyticsCore](https://github.com/firebase/firebase-android-sdk/blob/6e286da45f952142d91482219f0ad1e019c2b06d/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java#L167-L168) 

3. Due to that, in [CrashlyticsUncaughtExceptionHandler](https://github.com/firebase/firebase-android-sdk/blob/master/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsUncaughtExceptionHandler.java#L35-L43) constructor below, `defaultHandler` **was using the default UncaughtExceptionHandler which came from Android framework itself**, i.e. `com.android.internal.os.RuntimeInit.KillApplicationHandler`

```java
  public CrashlyticsUncaughtExceptionHandler(
      CrashListener crashListener,
      SettingsDataProvider settingsProvider,
      Thread.UncaughtExceptionHandler defaultHandler) {
    this.crashListener = crashListener;
    this.settingsDataProvider = settingsProvider;
    this.defaultHandler = defaultHandler; <-- The issue. Its getting from the thread of the ContentProvider initialization
    this.isHandlingException = new AtomicBoolean(false);
  }

  @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 {
        crashListener.onUncaughtException(settingsDataProvider, thread, ex);
      }
    } catch (Exception e) {
      Logger.getLogger().e("An error occurred in the uncaught exception handler", e);
    } finally {
      Logger.getLogger()
          .d(
              "Crashlytics completed exception processing."
                  + " Invoking default exception handler.");
      defaultHandler.uncaughtException(thread, ex); <-- Might be fixable by just using the handler retrieved on the actual Thread parameter I think? Quite a lot to ponder on here
      isHandlingException.set(false);
    }
  }

And this is a short snippet from com.android.internal.os.RuntimeInit.KillApplicationHandler (needless code removed for brevity) as per Android Q:

    private static class KillApplicationHandler implements Thread.UncaughtExceptionHandler {
         .... 
        @Override
        public void uncaughtException(Thread t, Throwable e) {
            try {
             ....
            } catch (Throwable t2) {
                if (t2 instanceof DeadObjectException) {
                    // System process is dead; ignore
                } else {
                    try {
                        Clog_e(TAG, "Error reporting crash", t2);
                    } catch (Throwable t3) {
                        // Even Clog_e() fails!  Oh well.
                    }
                }
            } finally {
                // Try everything to make sure this process goes away.
                Process.killProcess(Process.myPid()); 
                System.exit(10);
            }
        }

which will practically kill the application once it finishes.

Possible Workarounds

None that are library consumer friendly at the moment:

• Affected users can disable Firebase auto-initializing via Content Provider itself and init Firebase manually, and only do this right after they init their own custom UncaughtExceptionHandler first.
• OR Just plug their own UncaughtExceptionHandler in a ContentProvider, with initOrder above 100

Right now, even if you disable Firebase Crashlytics via Android Manifest meta data, then initialize your custom crash handler, and finally reenable Firebase Crashlytics via FirebaseCrashlytics.getInstance().setCrashlyticsCollectionEnabled(true);, the issue will persist.

This is since, CrashlyticsUncaughtExceptionHandler defaultHandler is still referencing to the original handler of the ContentProvider initialization thread (which is the KillApplicationHandler mentioned above).

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[amirulzin]

This issue rear its ugly head if you migrated from Fabrics since normally, you'll init your UncaughtExceptionHandler much earlier, and only then, init Fabrics via Fabric.with(..).

After pondering about it, my take is that, due to Firebase Content Provider initialization, the issue morphed into as to how library consumers need to let Firebase Crashlytics actually see their own custom UncaughtExceptionHandler right when Firebase/Crashlytic itself initialize.

I think letting library consumer to set the default UncaughtExceptionHandler being called in CrashlyticsUncaughtExceptionHandler may not be a bad idea (probably need few layers of encapsulation), though it does feels a bit wonky in a sense.

[mrichards]

@amirulzin Can you describe the behavior that you would expect in your YourCustomUncaughtExceptionHandler#uncaughtException(...) example implementation? If you are not using Crashlytics, the call to defaultHandler.uncaughtException will invoke the system KillApplicationHandler, which will kill the app. What is the scenario (with or without Crashlytics) where you would expect to see the "Hello, Ground Control" log message?

If you're intentionally preventing the app from closing due to an uncaught exception, you could still report the exception to Crashlytics using Crashlytics#recordException(Throwable) in your custom handler. That API is intended for non-fatal exceptions, so it will work well for custom exception handler that prevents your app from crashing.

[amirulzin]

@mrichards
You nailed it.

• My project have certain behavior which prevent the app from entirely crashing with certain logging behavior as well (so not just Crashlytics).

• The user have the option to kill the app or restart the app (with an attempt to recover the entry states correctly once the app restarted).

• If the user kill the app, we will still invoke defaultHandler.uncaughtException(thread, exception) retrieved via Thread.getUncaughtExceptionHandler to propagate the exception "upwards" properly. This practically follows just like what the current CrashlyticsUncaughtExceptionHandler is doing.

Since we migrated from Fabrics, with the current Crashlytics SDK, we can't exactly call the default handler due to this behavior without using ContentProvider workaround. Hence any propagation upwards practically terminates on our end. I'm not completely certain if this is an ideal behavior to follow and to be set forward for future implementations.

While I suppose we can indeed call Crashlytics#recordException(Throwable), some concerns below do came up for me and any library consumers to truly ponder on before fully commiting to this behavior:

• Async behavior on Crashlytics#recordException. You now have to worry to not kill the app before this block finishes.

• Any other side effects either from the Android framework or Crashlytics itself, by not propagating the default handler behavior (probably a bit harder to keep up)

I do understand that this is a bit of an edge case so I'm open to your inputs. The ContentProvider workaround do works wonders though and we already prepared it for production (i.e. Crashlytics do see my handler and then set it as the proper default handler. Propagation also works well now.) and thus we now had migrated to Firebase Crashlytics completely.

[mrichards]

@amirulzin Thanks for describing the use case, and I'm glad you found a workable solution! I understand the concerns around async behavior of recordException, though I'm sure you understand why that is a necessary tradeoff to keep the SDK performant.

As you said, your requirement is a bit of an edge case, and the Crashlytics SDK is behaving as intended. So I'm going to close the issue. I appreciate you taking the time to post the ContentProvider workaround for others that may want to implement a similar feature!

[amirulzin]

No problem. For those dropping in, this is an example of a ContentProvider workaround for this:

<application>
    ...
    <!-- Firebase SDK initOrder is 100. Higher order init first -->
    <provider
        android:name=".UncaughtExceptionHandlerContentProvider"
        android:authorities="${applicationId}"
        android:exported="false"
        android:initOrder="101"
        android:grantUriPermissions="false" />
    ... 
</application>

public class UncaughtExceptionHandlerContentProvider extends ContentProvider {
    @Override
    public boolean onCreate() {
        MyCustomCrashHandler myHandler = new MyCustomCrashHandler(Thread.getDefaultUncaughtExceptionHandler());
        Thread.setDefaultUncaughtExceptionHandler(myHandler);
        return true;
    }

    @Nullable
    @Override
    public Cursor query(@NonNull Uri uri, @Nullable String[] projection, @Nullable String selection, @Nullable String[] selectionArgs, @Nullable String sortOrder) { return null; }

    @Nullable
    @Override
    public String getType(@NonNull Uri uri) { return null; }

    @Nullable
    @Override
    public Uri insert(@NonNull Uri uri, @Nullable ContentValues values) { return null; }

    @Override
    public int delete(@NonNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) { return 0; }

    @Override
    public int update(@NonNull Uri uri, @Nullable ContentValues values, @Nullable String selection, @Nullable String[] selectionArgs) { return 0; }
}

public class MyCustomCrashHandler implements UncaughtExceptionHandler {
    @Nullable 
    private final UncaughtExceptionHandler defaultHandler;

    public MyCustomCrashHandler(@Nullable UncaughtExceptionHandler defaultHandler)(){
         this.defaultHandler = defaultHandler;
    }

    @Override
    public void uncaughtException(@NonNull Thread thread, @NonNull Throwable ex) {
        // We are now safely being called after Crashlytics does its own thing. 
        // Whoever is the last handler on Thread.getDefaultUncaughtExceptionHandler() will execute first on uncaught exceptions.
        // Firebase Crashlytics will handle its own behavior first before calling ours in its own 'finally' block.
        // You can choose to propagate upwards (it will kill the app by default) or do your own thing and propagate if needed.

        try { 
            //do your own thing.
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            if (defaultHandler != null) {
                defaultHandler.uncaughtException(thread, ex) 
                // propagate upwards. With this workaround (and also without any other similar UncaughtExceptionHandler based on ContentProvider), 
                // defaultHandler should now be an instance of com.android.internal.os.RuntimeInit.KillApplicationHandler
                // hence properly killing the app via framework calls.
            }
        }
    }

[paulpv]

Thanks a TON @amirulzin !

NOTE that you can also have [almost] the best of both worlds by having a pre and a post crash handler:

class UncaughtExceptionHandlerContentProvider : ContentProvider() {
    companion object {
        private val TAG = Utils.TAG(UncaughtExceptionHandlerContentProvider::class.java)
        const val PRE_DELAY_MILLIS = 3000L
        const val POST_DELAY_MILLIS = 3000L
        fun initializeAfterFirebaseContentProvider() {
            Thread.setDefaultUncaughtExceptionHandler(PreFirebaseCrashHandler(Thread.getDefaultUncaughtExceptionHandler()))
        }
    }

    override fun onCreate(): Boolean {
        try {
            Log.i(TAG, "+onCreate()")
            Thread.setDefaultUncaughtExceptionHandler(PostFirebaseCrashHandler(Thread.getDefaultUncaughtExceptionHandler()))
            return true
        } finally {
            Log.i(TAG, "-onCreate()")
        }
    }

    override fun query(uri: Uri, projection: Array<out String>?, selection: String?, selectionArgs: Array<out String>?, sortOrder: String?): Cursor? {
        return null
    }

    override fun getType(uri: Uri): String? {
        return null
    }

    override fun insert(uri: Uri, values: ContentValues?): Uri? {
        return null
    }

    override fun delete(uri: Uri, selection: String?, selectionArgs: Array<out String>?): Int {
        return 0
    }

    override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array<out String>?): Int {
        return 0
    }

    class PreFirebaseCrashHandler(private val previousUncaughtExceptionHandler: Thread.UncaughtExceptionHandler?) : Thread.UncaughtExceptionHandler {
        companion object {
            private val TAG = Utils.TAG(PreFirebaseCrashHandler::class.java)
        }

        override fun uncaughtException(thread: Thread, throwable: Throwable) {
            Log.e(TAG, "uncaughtException($thread, $throwable)")
            try {
                // do your own thing...
                val limitedLogLines = MyLogCat.read(...)
                val crashlytics = FirebaseCrashlytics.getInstance()
                for (logLine in limitedLogLines) {
                    crashlytics.log(logLine)
                }
                Thread.sleep(PRE_DELAY_MILLIS)
            } catch (e: Throwable) {
                Log.e(TAG, "uncaughtException: EXCEPTION", e)
            } finally {
                try {
                    Log.e(TAG, "uncaughtException: +previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)")
                    previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)
                } finally {
                    Log.e(TAG, "uncaughtException: -previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)")
                }
            }
        }
    }

    class PostFirebaseCrashHandler(private val previousUncaughtExceptionHandler: Thread.UncaughtExceptionHandler?) : Thread.UncaughtExceptionHandler {
        companion object {
            private val TAG = Utils.TAG(PostFirebaseCrashHandler::class.java)
        }

        override fun uncaughtException(thread: Thread, throwable: Throwable) {
            Log.e(TAG, "+uncaughtException($thread, $throwable)")
            try {
                Thread.sleep(POST_DELAY_MILLIS)
            } catch (e: Throwable) {
                Log.e(TAG, "uncaughtException: EXCEPTION", e)
            } finally {
                try {
                    Log.e(TAG, "uncaughtException: +previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)")
                    previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)
                } finally {
                    Log.e(TAG, "uncaughtException: -previousUncaughtExceptionHandler?.uncaughtException(thread, throwable)")
                }
            }
            Log.e(TAG, "-uncaughtException($thread, $throwable)")
        }
    }
}

class MainApp : Application() {
    override fun onCreate() {
        super.onCreate()
        UncaughtExceptionHandlerContentProvider.initializeAfterFirebaseContentProvider()
        val crashlytics = FirebaseCrashlytics.getInstance()
        val platformInfo = Utils.getPlatformInfo(this, null)
        for ((key, value) in platformInfo) {
            if (key == "DeviceId") {
                crashlytics.setUserId(value)
            } else {
                crashlytics.setCustomKey(key, value)
            }
        }
    }
}