Closed Bug 733434 Opened 8 years ago Closed 8 years ago

Breakpad not reporting uncaught Java exceptions

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: cpeterson, Assigned: cpeterson)

Details

Attachments

(2 files, 2 obsolete files)

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.
I think Scott also filed a similar bug 733434.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Register a global UncaughtExceptionHandler that calls Breakpad for uncaught exceptions on *any* thread.
Attachment #603459 - Flags: review?(doug.turner)
Attachment #603459 - Flags: review?(blassey.bugs)
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?
Attachment #603462 - Flags: review?(doug.turner)
Attachment #603462 - Flags: review?(blassey.bugs)
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()
Attachment #603459 - Flags: review?(blassey.bugs) → review-
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
Attachment #603462 - Flags: review?(blassey.bugs) → review+
> 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.
patch part 1 v2:
* Renamed registerBreakpadExceptionHandler() to registerGlobalExceptionHandler() to clarify its scope.
* Migrated reportJavaCrash(getStackTraceString(e)) change from patch 2.
Attachment #603459 - Attachment is obsolete: true
Attachment #603798 - Flags: review?(blassey.bugs)
Attachment #603459 - Flags: review?(doug.turner)
patch part 2 v2:
* Removed extraneous Looper runnable.
Attachment #603462 - Attachment is obsolete: true
Attachment #603802 - Flags: review?(blassey.bugs)
Attachment #603462 - Flags: review?(doug.turner)
Attachment #603798 - Flags: review?(blassey.bugs) → review+
Attachment #603802 - Flags: review?(blassey.bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9cbfd556de65
https://hg.mozilla.org/mozilla-central/rev/14515a9d0ab7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.