733434 - Breakpad not reporting uncaught Java exceptions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

We try to catch and report unexpected Java exceptions, but there are some code paths where our exception handler is not in the call stack. We should investigate whether we can integrate Java's UncaughtExceptionHandler and Breakpad:

https://developer.android.com/reference/java/lang/Thread.UncaughtExceptionHandler.html

http://www.javapractices.com/topic/TopicAction.do?Id=229

Bug 730890 is an example of an uncaught Java exception that causes a Dalvik 0xdeadd00d abort. Those stack traces are pretty much useless for us.

Comment 1

[Aaron Train [:aaronmt]]

I think Scott also filed a similar bug 733434.

Comment 2

[Aaron Train [:aaronmt]]

(bug 731903)

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: bug-733434-part-1-register-global-exception-handler.patch

Register a global UncaughtExceptionHandler that calls Breakpad for uncaught exceptions on *any* thread.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: bug-733434-part-2-remove-old-exception-handlers.patch

Remove old exception handlers made redundant by global UncaughtExceptionHandler.

blassey, this patch removes sTryCatchAttached. Do we even need the Looper.loop() runnable if the global UncaughtExceptionHandler will call Breakpad without needing a try/catch?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 603459 [details] [diff] [review]
bug-733434-part-1-register-global-exception-handler.patch

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

overall, good stuff. I want to see a patch that registers for other threads though.

::: mobile/android/base/GeckoApp.java
@@ +1601,5 @@
>      /** Called when the activity is first created. */
>      @Override
>      public void onCreate(Bundle savedInstanceState)
>      {
> +        GeckoAppShell.registerBreakpadExceptionHandler();

this only registers for the main ui thread. We should probably do this for the gecko thread and the background thread as well. As well as any other thread that accesses JNI (perhaps doing it in GetJNIForThread()

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 603462 [details] [diff] [review]
bug-733434-part-2-remove-old-exception-handlers.patch

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

r+ for the GeckoApp.java and GeckoThread.java changes. Obviously need the first patch before landing though.

::: mobile/android/base/GeckoApp.java
@@ +1744,5 @@
> +        mMainHandler.post(new Runnable() {
> +            public void run() {
> +                Looper.loop();
> +            }
> +        });

not really needed anymore, just delete entirely

::: mobile/android/base/GeckoAppShell.java
@@ +156,5 @@
>          StringWriter sw = new StringWriter();
>          PrintWriter pw = new PrintWriter(sw);
>          e.printStackTrace(pw);
>          pw.flush();
> +        return sw.toString();

roll all of these changes into the previous patch.

::: mobile/android/base/GeckoThread.java
@@ -106,5 @@
> -                                   mUri,
> -                                   mRestoreSession);
> -        } catch (Exception e) {
> -            GeckoAppShell.reportJavaCrash(e);
> -        }

as I understand it, your previous patch will not catch these exceptions. Gotta fix that before removing this try/catch

Comment 7

[Chris Peterson [:cpeterson]]

> this only registers for the main ui thread. We should probably do this for the gecko 
> thread and the background thread as well. As well as any other thread that accesses JNI 
> (perhaps doing it in GetJNIForThread()

UncaughtExceptionHandler is global. It will trap uncaught exceptions from any Java thread. That's why the uncaughtException() method takes a Thread parameter:

https://developer.android.com/reference/java/lang/Thread.UncaughtExceptionHandler.html

I confirmed this by spawning a new Thread and throwing an exception, which was successfully trapped by the global UncaughtExceptionHandler.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: bug-733434-part-1-register-global-exception-handler-v2.patch

patch part 1 v2:
* Renamed registerBreakpadExceptionHandler() to registerGlobalExceptionHandler() to clarify its scope.
* Migrated reportJavaCrash(getStackTraceString(e)) change from patch 2.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: bug-733434-part-2-remove-old-exception-handlers-v2.patch

patch part 2 v2:
* Removed extraneous Looper runnable.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbfd556de65
https://hg.mozilla.org/integration/mozilla-inbound/rev/14515a9d0ab7

BTW Brad, you are not currently listed as a Fennec peer on the Modules wiki. You should probably fix that :-)
https://wiki.mozilla.org/Modules/All

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9cbfd556de65
https://hg.mozilla.org/mozilla-central/rev/14515a9d0ab7