Closed Bug 958706 Opened 10 years ago Closed 10 years ago

Don't hide JNI exceptions

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(2 files, 1 obsolete file)

Right now the JNI code has a lot of this idiom,

> env->CallFooBar();
> if (env->ExceptionCheck()) {
>     ALOG_BRIDGE("Exceptional exit of: %s", __PRETTY_FUNCTION__);
>     env->ExceptionDescribe();
>     env->ExceptionClear();
>     env->PopLocalFrame();
>     return;
> }
> return;

But if an exception occurs, we'd rather have our crash reporter catch it than silently log the exception and ignore it.
(In reply to Jim Chen [:jchen :nchen] from comment #0)
> Right now the JNI code has a lot of this idiom,
> 
> > env->CallFooBar();
> > if (env->ExceptionCheck()) {
> >     ALOG_BRIDGE("Exceptional exit of: %s", __PRETTY_FUNCTION__);
> >     env->ExceptionDescribe();
> >     env->ExceptionClear();
> >     env->PopLocalFrame();
> >     return;
> > }
> > return;
> 
> But if an exception occurs, we'd rather have our crash reporter catch it
> than silently log the exception and ignore it.

I wouldn't go that far. If we can survive an exception I'd rather carry on running than crash the user. We can alternatively report the exception through FHR or Telemetry so that we get the data.
I think it's worth it, at least on nightly/aurora. The Java functions called by JNI don't normally throw exceptions, so in the rare case that we do get an exception, it's very likely a bug and the crash reporter is designed just for this purpose.

If an uncaught exception happened on a Java thread, we would be crashing, so I think it makes sense to have the same thing happen on a JNI thread. What we have right now is the equivalent of putting "try { ... } catch (Throwable e) {Log.i(e);}" everywhere in our Java code, which may look like we dealt with the exception but it can prevent diagnosing the real issue.
Depends on: 959237
I would be OK with crashing on Nightly, Aurora, Beta builds. If we want to get volume data beta would need to crash as well as Nightly and Aurora.
Fixing this bug can give us better reports too, and not just the generic libdvm.so reports like bug 825996.
ThrowException throws a new exception with the appropriate message, meant for native JNI methods that return to Java code (i.e. methods in AndroidJNI.cpp).

HandleUncaughtException will be called by the generated JNI stubs that C++ code uses. HandleUncaughtException calls the new GeckoAppShell.handleUncaughtException method, which behaves exactly like the normal uncaught exception handler (annotates the crash report and crashes). GeckoAppShell.handleUncaughtException has the noThrow annotation that will be seen by the generated code; as a result, its generated stub will not call HandleUncaughtException and result in a loop.
Attachment #8360532 - Flags: review?(blassey.bugs)
I want to give this a shot. It's an easy backout if it turns out to be crashing too much.
Attachment #8360533 - Flags: review?(blassey.bugs)
Comment on attachment 8360532 [details] [diff] [review]
Add ThrowException and HandleUncaughtException methods to AndroidBridge (v1)

Review of attachment 8360532 [details] [diff] [review]:
-----------------------------------------------------------------

I'm on board with the approach, but want to see another patch

::: mobile/android/base/GeckoAppShell.java
@@ +422,5 @@
> +    @WrapElementForJNI(generateStatic = true, noThrow = true)
> +    public static void handleUncaughtException(Throwable e) {
> +        // okay if getDefaultUncaughtExceptionHandler returns null here
> +        // because we are returning to JNI code that's crashing anyways
> +        Thread.getDefaultUncaughtExceptionHandler()

Let's make this https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#219 a singleton with a getter and call it directly. My concern is that many of our uncaught exceptions are happening during startup and perhaps before we register, so let's not drop those on the floor.

::: mobile/android/base/mozglue/generatorannotations/WrapElementForJNI.java
@@ +42,5 @@
> +    /**
> +     * If set, the generated stub will not handle uncaught exceptions.
> +     * Any exception must be handled or cleared by the code calling the stub.
> +     */
> +    boolean noThrow() default false;

why do we need this?
Attachment #8360532 - Flags: review?(blassey.bugs) → feedback+
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Comment on attachment 8360532 [details] [diff] [review]
> Add ThrowException and HandleUncaughtException methods to AndroidBridge (v1)
> 
> Review of attachment 8360532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm on board with the approach, but want to see another patch
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +422,5 @@
> > +    @WrapElementForJNI(generateStatic = true, noThrow = true)
> > +    public static void handleUncaughtException(Throwable e) {
> > +        // okay if getDefaultUncaughtExceptionHandler returns null here
> > +        // because we are returning to JNI code that's crashing anyways
> > +        Thread.getDefaultUncaughtExceptionHandler()
> 
> Let's make this
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoAppShell.java#219 a singleton with a getter and call it directly. My
> concern is that many of our uncaught exceptions are happening during startup
> and perhaps before we register, so let's not drop those on the floor.

Ok. I put the processing code inside GeckoAppShell.handleUncaughtException, and made the default handler call it.

> ::: mobile/android/base/mozglue/generatorannotations/WrapElementForJNI.java
> @@ +42,5 @@
> > +    /**
> > +     * If set, the generated stub will not handle uncaught exceptions.
> > +     * Any exception must be handled or cleared by the code calling the stub.
> > +     */
> > +    boolean noThrow() default false;
> 
> why do we need this?

GeckoAppShell.handleUncaughtException needs this flag because otherwise we could end up with a loop:

* AndroidBridge::HandleUncaughtException calls GeckoAppShell.handleUncaughtException
* GeckoAppShell.handleUncaughtException throws exception
* JNI stub calls AndroidBridge::HandleUncaughtException
* AndroidBridge::HandleUncaughtException calls GeckoAppShell.handleUncaughtException
* and so on...

The flag can also be used by other JNI callers that want to handle exceptions manually.
Attachment #8360532 - Attachment is obsolete: true
Attachment #8360648 - Flags: review?(blassey.bugs)
Attachment #8360533 - Flags: review?(blassey.bugs) → review+
Attachment #8360648 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/0ed7f1bad6e3
https://hg.mozilla.org/mozilla-central/rev/a7cf0019d949
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: