1066175 - Make pre-Gecko crashes easier to diagnose

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

If we crash before Gecko loads (due to Java exception, abort(), etc.), we want the Play Store to catch our crash with an informative message.

Comment 1

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

Attachment: Only crash when crash reporting annotation succeeds (v1)

If we fail to annotate the crash report, most likely the crash reporter hasn't been initialized yet. If we force Fennec to crash, the crash reporter won't catch the crash, and we will lose any information about the Java exception. Instead, we should return here and invoke the system crash handler instead.

Comment 3

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

Attachment: Use other means to handle uncaught exception when Gecko is unavailable (v1)

When the Gecko crash reporter is unavailable, uncaught Java exceptions can end up being ignored. We should try more ways to report the exception.

For Nightly/Aurora builds that don't report crashes to the Play Store, we can cause a hang and get an ANR report sent. For official builds, we can use the system handler to send a report to the Play Store.

Comment 4

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

Comment on attachment 8488149 [details] [diff] [review]
Use other means to handle uncaught exception when Gecko is unavailable (v1)

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

I have some questions, and not sure we need all of this.

::: mobile/android/base/GeckoAppShell.java
@@ +496,1 @@
>              reportJavaCrash(getStackTraceString(e));

What happens when we call this and libxul isn't even loaded yet? Could it cause a native crash, or just throw an exception? Should we check GeckoThread state before calling?

@@ +501,5 @@
> +        try {
> +            // reportJavaCrash should have caused us to hard crash. If we're still here,
> +            // it probably means Gecko is not loaded, and we should do something else.
> +            if (AppConstants.MOZ_UPDATER) {
> +                // If we are using our own updater, we were not installed from

I'm not a big fan of this. I think we should just skip the hang and let it fallback to the system crash handler (and maybe logcat it, but I assume system handler will do that too). Otherwise, I think it looks like the hang is a symptom of the crash.

@@ +525,5 @@
> +                hangRunnable.run();
> +            }
> +        } finally {
> +            // In any case, bring up the app crashed dialog so we don't crash silently.
> +            if (systemUncaughtHandler != null) {

Do we actually need to do this? I think the Android one gets these anyway, otherwise we wouldn't have crashes in Google Play.

Comment 5

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Comment on attachment 8488149 [details] [diff] [review]
> Use other means to handle uncaught exception when Gecko is unavailable (v1)
> 
> Review of attachment 8488149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions, and not sure we need all of this.
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +496,1 @@
> >              reportJavaCrash(getStackTraceString(e));
> 
> What happens when we call this and libxul isn't even loaded yet? Could it
> cause a native crash, or just throw an exception? Should we check
> GeckoThread state before calling?

It's an UnsatisfiedLinkError that'll be caught by the try block.

> @@ +501,5 @@
> > +        try {
> > +            // reportJavaCrash should have caused us to hard crash. If we're still here,
> > +            // it probably means Gecko is not loaded, and we should do something else.
> > +            if (AppConstants.MOZ_UPDATER) {
> > +                // If we are using our own updater, we were not installed from
> 
> I'm not a big fan of this. I think we should just skip the hang and let it
> fallback to the system crash handler (and maybe logcat it, but I assume
> system handler will do that too). Otherwise, I think it looks like the hang
> is a symptom of the crash.

For Nightly/Aurora, the system handler will just display a dialog. We won't know of the crash until it shows up in the Play Store for Beta and Release. That's what I'm worried about. I guess generating an ANR report won't help much. We should really send something to Socorro.

> @@ +525,5 @@
> > +                hangRunnable.run();
> > +            }
> > +        } finally {
> > +            // In any case, bring up the app crashed dialog so we don't crash silently.
> > +            if (systemUncaughtHandler != null) {
> 
> Do we actually need to do this? I think the Android one gets these anyway,
> otherwise we wouldn't have crashes in Google Play.

Only for the main thread. If we crash in a background thread, the thread will die but we'll just keep going, unless we call the system handler specifically.

Comment 6

[Richard Newman [:rnewman]]

Error is not a subclass of Exception. Be careful when you write your catch blocks!

Comment 7

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

(In reply to Jim Chen [:jchen :nchen] from comment #5)
> 
> > @@ +501,5 @@
> > > +        try {
> > > +            // reportJavaCrash should have caused us to hard crash. If we're still here,
> > > +            // it probably means Gecko is not loaded, and we should do something else.
> > > +            if (AppConstants.MOZ_UPDATER) {
> > > +                // If we are using our own updater, we were not installed from
> > 
> > I'm not a big fan of this. I think we should just skip the hang and let it
> > fallback to the system crash handler (and maybe logcat it, but I assume
> > system handler will do that too). Otherwise, I think it looks like the hang
> > is a symptom of the crash.
> 
> For Nightly/Aurora, the system handler will just display a dialog. We won't
> know of the crash until it shows up in the Play Store for Beta and Release.
> That's what I'm worried about. I guess generating an ANR report won't help
> much. We should really send something to Socorro.

Brad and I were talking about having some stuff to generate Java crash reports without native code. I think that's the next step. I think we should work on that instead of relying on ANR for this.

> 
> > @@ +525,5 @@
> > > +                hangRunnable.run();
> > > +            }
> > > +        } finally {
> > > +            // In any case, bring up the app crashed dialog so we don't crash silently.
> > > +            if (systemUncaughtHandler != null) {
> > 
> > Do we actually need to do this? I think the Android one gets these anyway,
> > otherwise we wouldn't have crashes in Google Play.
> 
> Only for the main thread. If we crash in a background thread, the thread
> will die but we'll just keep going, unless we call the system handler
> specifically.

Interesting! That's probably worth a comment.

From rnewman's comment it sounds like you need to specifically catch UnsatisfiedLinkError. If you fix that and remove the ANR generation it's a r+ from me. And then we should work on submitting Java crashes without native code. With this stuff in place, I believe we should never get a Google Play crash report. Oh, we'll need to hook all of this up to the other activities/services as well (sync, etc).

Comment 8

[Richard Newman [:rnewman]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> Oh, we'll need to hook all of this up to the other
> activities/services as well (sync, etc).

Note that Sync is currently so well guarded that it basically never reports a crash -- because the UX is so bad if a syncadapter falls over. We'd actually like to change that, reporting pseudo-crashes without actually crashing.

Comment 9

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

Attachment: Use other means to handle uncaught exception when Gecko is unavailable (v2)

Removed the ANR block, and changed the remaining catch to using Throwable.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0118051b162
https://hg.mozilla.org/integration/mozilla-inbound/rev/20704dcbd253

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0118051b162
https://hg.mozilla.org/mozilla-central/rev/20704dcbd253

Comment 12

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

Comment on attachment 8488760 [details] [diff] [review]
Use other means to handle uncaught exception when Gecko is unavailable (v2)

Approval Request Comment

[Feature/regressing bug #]: N/A

[User impact if declined]: Some crashes not being reported

[Describe test coverage new/current, TBPL]: Locally, m-c

[Risks and why]: Some risk of more crash reports coming in; this bug makes us report crashes that we would have (erroneously) ignored before.

[String/UUID change made/needed]: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84c8f07ac553
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e761d32b93f

https://hg.mozilla.org/releases/mozilla-beta/rev/e1d77019dda9
https://hg.mozilla.org/releases/mozilla-beta/rev/0cc0faf4524b