Closed Bug 1043457 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
fennec + ---

People

(Reporter: rnewman, Assigned: jchen)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
tracking-fennec: ? → +
Duplicate of this bug: 1066630
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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?
(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.
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.
Attachment #8506464 - Flags: review?(snorp)
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.
Attachment #8506465 - Flags: review?(snorp)
This patch implements the necessary code to take a Throwable and write out the appropriate files for the crash reporter to send to Socorro.
Attachment #8506467 - Flags: review?(snorp)
This patch uses CrashHandler to replace GeckoAppShell's existing exception handling. Custom logic is implemented by overriding CrashHandler methods.
Attachment #8506468 - Flags: review?(snorp)
Attachment #8506464 - Flags: review?(snorp) → review+
Attachment #8506465 - Flags: review?(snorp) → review+
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()?
Attachment #8506467 - Flags: review?(snorp) → review+
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?
Attachment #8506468 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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) (jwillcox@mozilla.com) 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.
Attachment #8506467 - Attachment is obsolete: true
Attachment #8507179 - Flags: review+
filter on [mass-p5]
Priority: -- → P5
Priority: P5 → --
You need to log in before you can comment on or make changes to this bug.