1279488 - Make crashes of GeckoBackgroundThread visible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

What's scary about bug 1279332 is that we don't notice that: The background thread throws an exception and stops running.

Let's catch the exception and re-throw it on the main thread, causing a crash. This will make such problems visible and we are able to fix it.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

After trying to add reporting of uncaught exceptions to the background thread I realized that this is already happening by default.

However we are suppressing the exception/crash here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java#137

We are currently in state RUNNING(6). The ranking just recently changed here:
https://hg.mozilla.org/mozilla-central/rev/e4c63fb9f8b0

EXITING was 8 before, now it's 3. So with current state RUNNING isStateAtLeast(EXITING) is now true (6 >= 3) and was false before (6 >= 8).

@jchen: From bug 1268125 I can't see why we changed the rank of EXITING. Was this intentional? Do we need to change the logic in GeckoAppShell now?

Comment 2

[Jan Henning [:JanH]]

I think I've seen something similar for the main/UI thread as well:
While working on the Recently Closed folder, I'd encountered a bug where the whole UI would simply hang.

It turned out that when about:home is opened from the tabs tray, things are initialised in a different order, so I was calling a function on an object while it was still null. Instead of giving an appropriate exception, it turned out that - just as in your case - the UI thread simply silently died afterwards, which of course left the whole UI hanging.

Comment 3

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

Yep the EXITING change was by design, but GeckoAppShell needs to now check

> GeckoThread.isState(GeckoThread.State.EXITING) ||
>     GeckoThread.isState(GeckoThread.State.EXITED)

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1279488 - GeckoAppShell.uncaughtException(): Update state check.

Review commit: https://reviewboard.mozilla.org/r/59212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59212/

Comment 5

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

Comment on attachment 8762671 [details]
Bug 1279488 - GeckoAppShell.uncaughtException(): Update state check.

https://reviewboard.mozilla.org/r/59212/#review56372

r+ with change.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:138
(Diff revision 1)
>          }
>  
>          @Override
>          public void uncaughtException(final Thread thread, final Throwable exc) {
> -            if (GeckoThread.isStateAtLeast(GeckoThread.State.EXITING)) {
> +            if (GeckoThread.isState(GeckoThread.State.EXITING) ||
> +                    GeckoThread.isState(GeckoThread.State.EXITING)) {

Staet.EXITED for the second line :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/59212/#review56562

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:138
(Diff revision 1)
>          }
>  
>          @Override
>          public void uncaughtException(final Thread thread, final Throwable exc) {
> -            if (GeckoThread.isStateAtLeast(GeckoThread.State.EXITING)) {
> +            if (GeckoThread.isState(GeckoThread.State.EXITING) ||
> +                    GeckoThread.isState(GeckoThread.State.EXITING)) {

Oops.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8762671 [details]
Bug 1279488 - GeckoAppShell.uncaughtException(): Update state check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59212/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0df6c5c1260b
GeckoAppShell.uncaughtException(): Update state check. r=jchen

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0df6c5c1260b