Last Comment Bug 739418 - If a rethrown exception is uncaught, report the stack trace of the original exception, not the rethrowing exception
: If a rethrown exception is uncaught, report the stack trace of the original e...
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 14
Assigned To: Chris Peterson [:cpeterson]
: Sebastian Kaspari (:sebastian)
Depends on: 701002
  Show dependency treegraph
Reported: 2012-03-26 14:40 PDT by Chris Peterson [:cpeterson]
Modified: 2012-04-06 16:01 PDT (History)
3 users (show)
cpeterson: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

bug-739418-report-rethrown-exceptions.patch (2.73 KB, patch)
2012-03-26 14:47 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
bug-739418-report-rethrown-exceptions-v2.patch (1.70 KB, patch)
2012-03-27 11:35 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-03-26 14:40:21 PDT
A rethrown exception's stack trace lists the rethrowing exception before the original exception (the `cause`). When reviewing crash reports in Socorro, unrelated crashes may look like dupes because their stack traces point to the same rethrowing error handler. If we move the original exception to the top of the crash report, Socorro can correctly collate related crash reports.

Examples of unrelated bugs that may look like dupes: bug 734624, bug 738977, and bug 738976.
Comment 1 Chris Peterson [:cpeterson] 2012-03-26 14:47:17 PDT
Created attachment 609499 [details] [diff] [review]

When reporting a rethrown exception, report the original exception's stack trace.
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-03-26 18:23:47 PDT
Comment on attachment 609499 [details] [diff] [review]

Review of attachment 609499 [details] [diff] [review]:

::: mobile/android/base/
@@ +161,5 @@
>              public void uncaughtException(Thread thread, Throwable e) {
> +                String stackTrace = getStackTraceString(e);
> +
> +                Throwable cause = e.getCause();
> +                if (cause != null) {

should this be while(cause != null)?

I'd do it this way:

Throwable cause;
while (null != (cause = e.getCause()))
   e = cause;
Comment 3 Chris Peterson [:cpeterson] 2012-03-27 11:35:28 PDT
Created attachment 609816 [details] [diff] [review]

If the uncaught exception was rethrown, walk the exception `cause` chain to find the original exception.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-03-28 14:42:18 PDT
Comment on attachment 609816 [details] [diff] [review]

Review of attachment 609816 [details] [diff] [review]:

::: mobile/android/base/
@@ +169,2 @@
>                                + thread.getId() + " (\"" + thread.getName() + "\")", e);

its probably worth moving this logging above the walking since it will include all the stacks
Comment 5 Chris Peterson [:cpeterson] 2012-03-29 13:02:11 PDT
Comment 6 Chris Peterson [:cpeterson] 2012-03-29 13:04:14 PDT
Comment on attachment 609816 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Socorro crash reports will be harder to diagnose.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low. Java only code that is only executed when reporting an uncaught exception.
String changes made by this patch: None
Comment 7 Chris Peterson [:cpeterson] 2012-03-30 10:05:48 PDT
Comment 8 Alex Keybl [:akeybl] 2012-04-02 09:56:49 PDT
Comment on attachment 609816 [details] [diff] [review]

[Triage Comment]
Low risk, mobile-only fix. Approved for Aurora 13.
Comment 9 Chris Peterson [:cpeterson] 2012-04-02 14:33:41 PDT
Comment 10 Chris Peterson [:cpeterson] 2012-04-03 13:17:35 PDT
Comment on attachment 609816 [details] [diff] [review]

If we are going to land bug 701002 to improve Java crash reporting on ESR10, then I recommend also landing this patch to make Java crash reports easier to collate in Socorro.
Comment 11 Alex Keybl [:akeybl] 2012-04-05 16:54:25 PDT
Comment on attachment 609816 [details] [diff] [review]

[Triage Comment]
Approved for ESR10 in support of crash killing. This has already had time to bake on Nightly/Aurora.
Comment 12 Chris Peterson [:cpeterson] 2012-04-06 09:24:01 PDT

Note You need to log in before you can comment on or make changes to this bug.