Closed
Bug 1043457
Opened 10 years ago
Closed 10 years ago
Implement crash reporting in Java to allow for detection of early-stage crashes
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 36
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: rnewman, Assigned: jchen)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
tracking-fennec: ? → +
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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?
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
This patch uses CrashHandler to replace GeckoAppShell's existing exception handling. Custom logic is implemented by overriding CrashHandler methods.
Attachment #8506468 -
Flags: review?(snorp)
Updated•10 years ago
|
Attachment #8506464 -
Flags: review?(snorp) → review+
Updated•10 years ago
|
Attachment #8506465 -
Flags: review?(snorp) → review+
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
(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+
Reporter | ||
Updated•10 years ago
|
Priority: P5 → --
Assignee | ||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•