Bug 1235475 - Crash at the exception source when an exception is in native code; r=snorp

When we have a Java exception in native code, the Java stack in the
exception will not be very useful because the top frame is the native
entry point. In this case, the native stack is more useful. However,
currently we don't get a good native stack in this situation because we
go through Java when handling the exception, and the native stack we get
will have a lot of unknown frames inside libdvm or libart. This patch
makes us stay in native code when handling an uncaught exception from
native code, so that we get a good native stack.

mobile/android/base/java/org/mozilla/gecko/CrashHandler.java

@@ -145,7 +145,7 @@ public class CrashHandler implements Thread.UncaughtExceptionHandler {
      * @param thread The exception thread
      * @param exc An exception
      */
-    protected void logException(final Thread thread, final Throwable exc) {
+    public static void logException(final Thread thread, final Throwable exc) {
         try {
             Log.e(LOGTAG, ">>> REPORTING UNCAUGHT EXCEPTION FROM THREAD "
                           + thread.getId() + " (\"" + thread.getName() + "\")", exc);

mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java

@@ -396,8 +396,20 @@ public class GeckoAppShell
      */
 
     @WrapForJNI(allowMultithread = true, noThrow = true)
-    public static void handleUncaughtException(Thread thread, Throwable e) {
+    public static String handleUncaughtException(Throwable e) {
-        CRASH_HANDLER.uncaughtException(thread, e);
+        if (AppConstants.MOZ_CRASHREPORTER) {
+            final Throwable exc = CrashHandler.getRootException(e);
+            final StackTraceElement[] stack = exc.getStackTrace();
+            if (stack.length >= 1 && stack[0].isNativeMethod()) {
+                // The exception occurred when running native code. Return an exception
+                // string and trigger the crash reporter inside the caller so that we get
+                // a better native stack in Socorro.
+                CrashHandler.logException(Thread.currentThread(), exc);
+                return CrashHandler.getExceptionStackTrace(exc);
+            }
+        }
+        CRASH_HANDLER.uncaughtException(null, e);
+        return null;
     }
 
     private static final Object sEventAckLock = new Object();

widget/android/AndroidBridge.cpp

@@ -2148,20 +2148,20 @@ Object::LocalRef AndroidBridge::ChannelCreate(Object::Param stream) {
     JNIEnv* const env = GetEnvForThread();
     auto rv = Object::LocalRef::Adopt(env, env->CallStaticObjectMethod(
             sBridge->jChannels, sBridge->jChannelCreate, stream.Get()));
-    HandleUncaughtException(env);
+    MOZ_CATCH_JNI_EXCEPTION(env);
     return rv;
 }
 
 void AndroidBridge::InputStreamClose(Object::Param obj) {
     JNIEnv* const env = GetEnvForThread();
     env->CallVoidMethod(obj.Get(), sBridge->jClose);
-    HandleUncaughtException(env);
+    MOZ_CATCH_JNI_EXCEPTION(env);
 }
 
 uint32_t AndroidBridge::InputStreamAvailable(Object::Param obj) {
     JNIEnv* const env = GetEnvForThread();
     auto rv = env->CallIntMethod(obj.Get(), sBridge->jAvailable);
-    HandleUncaughtException(env);
+    MOZ_CATCH_JNI_EXCEPTION(env);
     return rv;
 }
 

widget/android/AndroidBridge.h

@@ -561,7 +561,7 @@ public:
 
     bool CheckForException() {
         if (mJNIEnv->ExceptionCheck()) {
-            jni::HandleUncaughtException(mJNIEnv);
+            MOZ_CATCH_JNI_EXCEPTION(mJNIEnv);
             return true;
         }
         return false;

widget/android/GeneratedJNIWrappers.cpp

@@ -449,9 +449,9 @@ auto GeckoAppShell::HandleGeckoMessageWrapper(mozilla::jni::Object::Param a0) ->
 constexpr char GeckoAppShell::HandleUncaughtException_t::name[];
 constexpr char GeckoAppShell::HandleUncaughtException_t::signature[];
 
-auto GeckoAppShell::HandleUncaughtException(mozilla::jni::Object::Param a0, mozilla::jni::Throwable::Param a1) -> void
+auto GeckoAppShell::HandleUncaughtException(mozilla::jni::Throwable::Param a0) -> mozilla::jni::String::LocalRef
 {
-    return mozilla::jni::Method<HandleUncaughtException_t>::Call(nullptr, nullptr, a0, a1);
+    return mozilla::jni::Method<HandleUncaughtException_t>::Call(nullptr, nullptr, a0);
 }
 
 constexpr char GeckoAppShell::HideProgressDialog_t::name[];

widget/android/GeneratedJNIWrappers.h

@@ -1084,21 +1084,20 @@ public:
 public:
     struct HandleUncaughtException_t {
         typedef GeckoAppShell Owner;
-        typedef void ReturnType;
+        typedef mozilla::jni::String::LocalRef ReturnType;
-        typedef void SetterType;
+        typedef mozilla::jni::String::Param SetterType;
         typedef mozilla::jni::Args<
-                mozilla::jni::Object::Param,
                 mozilla::jni::Throwable::Param> Args;
         static constexpr char name[] = "handleUncaughtException";
         static constexpr char signature[] =
-                "(Ljava/lang/Thread;Ljava/lang/Throwable;)V";
+                "(Ljava/lang/Throwable;)Ljava/lang/String;";
         static const bool isStatic = true;
         static const bool isMultithreaded = true;
         static const mozilla::jni::ExceptionMode exceptionMode =
                 mozilla::jni::ExceptionMode::IGNORE;
     };
 
-    static auto HandleUncaughtException(mozilla::jni::Object::Param, mozilla::jni::Throwable::Param) -> void;
+    static auto HandleUncaughtException(mozilla::jni::Throwable::Param) -> mozilla::jni::String::LocalRef;
 
 public:
     struct HideProgressDialog_t {

widget/android/jni/Accessors.h

@@ -72,10 +72,10 @@ protected:
     static void EndAccess(JNIEnv* env, nsresult* rv)
     {
         if (Traits::exceptionMode == ExceptionMode::ABORT) {
-            return HandleUncaughtException(env);
+            MOZ_CATCH_JNI_EXCEPTION(env);
 
         } else if (Traits::exceptionMode == ExceptionMode::NSRESULT) {
-            return GetNsresult(env, rv);
+            GetNsresult(env, rv);
         }
     }
 };

widget/android/jni/Natives.h

@@ -107,7 +107,7 @@ struct NativePtr
         Clear(instance);
         SetNativeHandle(instance.Env(), instance.Get(),
                           reinterpret_cast<uintptr_t>(ptr.release()));
-        HandleUncaughtException(instance.Env());
+        MOZ_CATCH_JNI_EXCEPTION(instance.Env());
     }
 
     template<class LocalRef>
@@ -115,11 +115,11 @@ struct NativePtr
     {
         UniquePtr<Impl> ptr(reinterpret_cast<Impl*>(
                 GetNativeHandle(instance.Env(), instance.Get())));
-        HandleUncaughtException(instance.Env());
+        MOZ_CATCH_JNI_EXCEPTION(instance.Env());
 
         if (ptr) {
             SetNativeHandle(instance.Env(), instance.Get(), 0);
-            HandleUncaughtException(instance.Env());
+            MOZ_CATCH_JNI_EXCEPTION(instance.Env());
         }
     }
 };
@@ -155,7 +155,7 @@ struct NativePtr<Impl, /* UseWeakPtr = */ true>
         Clear(instance);
         SetNativeHandle(instance.Env(), instance.Get(),
                           reinterpret_cast<uintptr_t>(new WeakPtr<Impl>(ptr)));
-        HandleUncaughtException(instance.Env());
+        MOZ_CATCH_JNI_EXCEPTION(instance.Env());
     }
 
     template<class LocalRef>
@@ -163,11 +163,11 @@ struct NativePtr<Impl, /* UseWeakPtr = */ true>
     {
         const auto ptr = reinterpret_cast<WeakPtr<Impl>*>(
                 GetNativeHandle(instance.Env(), instance.Get()));
-        HandleUncaughtException(instance.Env());
+        MOZ_CATCH_JNI_EXCEPTION(instance.Env());
 
         if (ptr) {
             SetNativeHandle(instance.Env(), instance.Get(), 0);
-            HandleUncaughtException(instance.Env());
+            MOZ_CATCH_JNI_EXCEPTION(instance.Env());
             delete ptr;
         }
     }
@@ -328,7 +328,7 @@ class ProxyNativeCall
          mozilla::IndexSequence<Indices...>) const
     {
         Impl* const impl = NativePtr<Impl>::Get(inst);
-        HandleUncaughtException(inst.Env());
+        MOZ_CATCH_JNI_EXCEPTION(inst.Env());
         (impl->*mNativeCall)(inst, mozilla::Get<Indices>(mArgs)...);
     }
 
@@ -338,7 +338,7 @@ class ProxyNativeCall
          mozilla::IndexSequence<Indices...>) const
     {
         Impl* const impl = NativePtr<Impl>::Get(inst);
-        HandleUncaughtException(inst.Env());
+        MOZ_CATCH_JNI_EXCEPTION(inst.Env());
         (impl->*mNativeCall)(mozilla::Get<Indices>(mArgs)...);
     }
 

widget/android/jni/Refs.h

@@ -571,7 +571,7 @@ public:
         MOZ_ASSERT(ObjectArray::mInstance);
         JNIEnv* const env = GetEnvForThread();
         const size_t ret = env->GetArrayLength(jarray(ObjectArray::mInstance));
-        HandleUncaughtException(env);
+        MOZ_CATCH_JNI_EXCEPTION(env);
         return ret;
     }
 
@@ -581,7 +581,7 @@ public:
         JNIEnv* const env = GetEnvForThread();
         auto ret = Object::LocalRef::Adopt(env, env->GetObjectArrayElement(
                 jobjectArray(ObjectArray::mInstance), jsize(index)));
-        HandleUncaughtException(env);
+        MOZ_CATCH_JNI_EXCEPTION(env);
         return ret;
     }
 
@@ -597,7 +597,7 @@ public:
             array.AppendElement(Object::LocalRef::Adopt(
                     env, env->GetObjectArrayElement(
                             jobjectArray(ObjectArray::mInstance), i)));
-            HandleUncaughtException(env);
+            MOZ_CATCH_JNI_EXCEPTION(env);
         }
         return array;
     }
@@ -613,7 +613,7 @@ public:
         JNIEnv* const env = GetEnvForThread();
         env->SetObjectArrayElement(jobjectArray(ObjectArray::mInstance),
                                    jsize(index), element.Get());
-        HandleUncaughtException(env);
+        MOZ_CATCH_JNI_EXCEPTION(env);
     }
 };
 
@@ -677,7 +677,7 @@ private:
         const jstring result = env->NewString(
                 reinterpret_cast<const jchar*>(str.BeginReading()),
                 str.Length());
-        HandleUncaughtException(env);
+        MOZ_CATCH_JNI_EXCEPTION(env);
         return result;
     }
 

widget/android/jni/Utils.cpp

@@ -8,6 +8,10 @@
 #include "AndroidBridge.h"
 #include "GeneratedJNIWrappers.h"
 
+#ifdef MOZ_CRASHREPORTER
+#include "nsExceptionHandler.h"
+#endif
+
 namespace mozilla {
 namespace jni {
 
@@ -129,23 +133,34 @@ bool ThrowException(JNIEnv *aEnv, const char *aClass,
     return !aEnv->ThrowNew(cls.Get(), aMessage);
 }
 
-void HandleUncaughtException(JNIEnv *aEnv)
+bool HandleUncaughtException(JNIEnv* aEnv)
 {
     MOZ_ASSERT(aEnv, "Invalid thread JNI env");
 
     if (!aEnv->ExceptionCheck()) {
-        return;
+        return false;
     }
 
+#ifdef DEBUG
+    aEnv->ExceptionDescribe();
+#endif
+
     Throwable::LocalRef e =
             Throwable::LocalRef::Adopt(aEnv->ExceptionOccurred());
     MOZ_ASSERT(e);
 
     aEnv->ExceptionClear();
-    widget::GeckoAppShell::HandleUncaughtException(nullptr, e);
+    String::LocalRef stack = widget::GeckoAppShell::HandleUncaughtException(e);
 
-    // Should be dead by now...
+#ifdef MOZ_CRASHREPORTER
-    MOZ_CRASH("Failed to handle uncaught exception");
+    if (stack) {
+        // GeckoAppShell wants us to annotate and trigger the crash reporter.
+        CrashReporter::AnnotateCrashReport(
+                NS_LITERAL_CSTRING("AuxiliaryJavaStack"), nsCString(stack));
+    }
+#endif // MOZ_CRASHREPORTER
+
+    return true;
 }
 
 namespace {

widget/android/jni/Utils.h

@@ -53,7 +53,14 @@ inline bool ThrowException(const char *aMessage)
     return ThrowException(GetEnvForThread(), aMessage);
 }
 
-void HandleUncaughtException(JNIEnv *aEnv);
+bool HandleUncaughtException(JNIEnv* aEnv);
+
+#define MOZ_CATCH_JNI_EXCEPTION(env) \
+    do { \
+        if (mozilla::jni::HandleUncaughtException((env))) { \
+            MOZ_CRASH("JNI exception"); \
+        } \
+    } while (0)
 
 uintptr_t GetNativeHandle(JNIEnv* env, jobject instance);
 

widget/android/nsWindow.cpp

@@ -183,7 +183,7 @@ public:
             const auto natives = reinterpret_cast<
                     mozilla::WeakPtr<typename T::TargetClass>*>(
                     jni::GetNativeHandle(env, thisArg.Get()));
-            jni::HandleUncaughtException(env);
+            MOZ_CATCH_JNI_EXCEPTION(env);
 
             // The call is stale if the nsWindow has been destroyed on the
             // Gecko side, but the Java object is still attached to it through