1043457 - Implement crash reporting in Java to allow for detection of early-stage crashes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

We rely on Google Play to report crashes around library loading and similar failures.

That's less than ideal: we don't see those crashes until Beta, and we don't get the benefit of integration with crash-stats.

The obvious solution for this is to catch and generate a crash report directly from Java.

tracking? so we think about whether to prioritize this.

Comment 2

[Nick Alexander :nalexander [he/him]]

In #mobile, jchen said:

09:22 jchen: vng: oh cool. i'm actually working on a java-based crash reporter that will report to socorro (our crash reporting backend). the idea is so our java components that run outside of gecko will be able to use it to report crashes

Is this Fennec only?  Do we mean Java components running in the Fennec package, or do we mean Java components more generally, like MozStumbler?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #2)
> In #mobile, jchen said:
> 
> 09:22 jchen: vng: oh cool. i'm actually working on a java-based crash
> reporter that will report to socorro (our crash reporting backend). the idea
> is so our java components that run outside of gecko will be able to use it
> to report crashes
> 
> Is this Fennec only?  Do we mean Java components running in the Fennec
> package, or do we mean Java components more generally, like MozStumbler?

The focus was on Fennec components like pre-Gecko loading, search activity, etc. But now that we know MozStumbler has a similar need, I guess we should look into a more generic library.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add a CrashHandler class to handle uncaught exceptions (v1)

This patch adds a CrashHandler class that contains necessary code to catch and handle an uncaught exception. The goal of CrashHandler is to be free of Gecko dependencies, so it can be used outside of Fennec code.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Context and unregister options to CrashHandler (v1)

This patch adds additional constructors that accept a Context to be used for retrieving application information. It also adds an unregister method so a CrashHandler can be tied to the lifecycle of an activity, for example.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1043457 - Implement reporting exceptions through the crash reporter;

This patch implements the necessary code to take a Throwable and write out the appropriate files for the crash reporter to send to Socorro.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Switch Fennec to using CrashHandler (v1)

This patch uses CrashHandler to replace GeckoAppShell's existing exception handling. Custom logic is implemented by overriding CrashHandler methods.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8506467 [details] [diff] [review]
Bug 1043457 - Implement reporting exceptions through the crash reporter;

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

::: mobile/android/base/CrashHandler.java
@@ +180,5 @@
> +            return appContext.getPackageName();
> +        }
> +
> +        try {
> +            // Package name is also the command line string.

Is this always true? I think you can specify process name in AndroidManifest. Won't we always require an app context anyway?

@@ +258,5 @@
> +     * @param extras The crash extras Bundle
> +     */
> +    protected String getServerUrl(final Bundle extras) {
> +        return String.format(
> +            "https://crash-reports.mozilla.com/submit?id=%s&version=%s&buildid=%s",

This should be a constant somewhere.

@@ +280,5 @@
> +
> +            // Avoid AppConstants dependency for SDK version constants,
> +            // because CrashHandler could be used outside of Fennec code.
> +            if (Build.VERSION.SDK_INT < 17) {
> +                pb = new ProcessBuilder(

Can't we just use appContext.startActivity()?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8506468 [details] [diff] [review]
Switch Fennec to using CrashHandler (v1)

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

Nice! This is pretty exciting. Is there any hope of testing this before putting it in m-c?

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Implement reporting exceptions through the crash reporter (v1.1)

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> Comment on attachment 8506467 [details] [diff] [review]
> Bug 1043457 - Implement reporting exceptions through the crash reporter;
> 
> ::: mobile/android/base/CrashHandler.java
> @@ +180,5 @@
> > +            return appContext.getPackageName();
> > +        }
> > +
> > +        try {
> > +            // Package name is also the command line string.
> 
> Is this always true? I think you can specify process name in
> AndroidManifest. Won't we always require an app context anyway?

Good point. I think it's true for most cases, and I'd like CrashHandler to be usable even without a Context (e.g. when you declare it statically like 'private static final CRASH_HANDLER = new CrashHandler()')

> @@ +258,5 @@
> > +     * @param extras The crash extras Bundle
> > +     */
> > +    protected String getServerUrl(final Bundle extras) {
> > +        return String.format(
> > +            "https://crash-reports.mozilla.com/submit?id=%s&version=%s&buildid=%s",
> 
> This should be a constant somewhere.

Moved to a string constant

> @@ +280,5 @@
> > +
> > +            // Avoid AppConstants dependency for SDK version constants,
> > +            // because CrashHandler could be used outside of Fennec code.
> > +            if (Build.VERSION.SDK_INT < 17) {
> > +                pb = new ProcessBuilder(
> 
> Can't we just use appContext.startActivity()?

Added startActivity() call if appContext is not null.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> Comment on attachment 8506468 [details] [diff] [review]
> Switch Fennec to using CrashHandler (v1)
> 
> Review of attachment 8506468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! This is pretty exciting. Is there any hope of testing this before
> putting it in m-c?

I tested that it successfully sent reports to crash-stats.

Comment 11

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

filter on [mass-p5]

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/fx-team/rev/bbfa604ad1c1
https://hg.mozilla.org/integration/fx-team/rev/2d875415dd44
https://hg.mozilla.org/integration/fx-team/rev/630d6ed0b55b
https://hg.mozilla.org/integration/fx-team/rev/11421d58d71b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbfa604ad1c1
https://hg.mozilla.org/mozilla-central/rev/2d875415dd44
https://hg.mozilla.org/mozilla-central/rev/630d6ed0b55b
https://hg.mozilla.org/mozilla-central/rev/11421d58d71b