1229153 - crash in java.lang.IllegalStateException: Callback has already been executed for type=Snackbar:Show

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-1ad7a9f6-c905-4c05-a773-a14b82151129.
=============================================================

STR:
* Display a snackbar (e.g. by closing a tab)
* Press the home button to background Fennec

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regresssion

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #0)
> STR:
> * Display a snackbar (e.g. by closing a tab)
> * Press the home button to background Fennec

I'm unable to reproduce this with the STR. Does this happen every time?

The crash reports point to 44.0a2. Aurora only shows a snackbar when closing a tab.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I wonder how this can happen. To avoid this I synchronized onClick() and onDismiss() to invoke the callback only once:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?from=GeckoApp.java#880-898

Comment 4

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

Not every time, but I can reproduce in today's Nightly on a Nexus 4 running 4.4.2. I think this is a more reliable STR:

1. Turn on "Don't Keep Activities" in Developer options
2. Start Fennec
3. Close the about:home tab
4. Press Home button to background Fennec immediately, *while the snackbar is displayed*
5. Go back to Fennec
6. Repeat steps 3 and 4
7. Crash after waiting a few second (< 10 seconds)

I was a little puzzled too on why it's happening after scanning through the code.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a reproducible crash.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> 1. Turn on "Don't Keep Activities" in Developer options

With this developer setting I can see this happening too sometimes. I'll debug what's happening here..

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

What I see happening here is that GeckoApp.handleMessage() is called twice for "Snackbar:Show" - with the same callback (Not always but quite often). So we are actually trying to show two Snackbars. This is happening so fast that the first snackbar is dismissed instantly. Later when the second snackbar is dismissed we are calling the same callback again: IllegalStateException: Callback has already been executed.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Going deeper:
* Snackbar.jsm sends the event only once
* In EventDispatcher BrowserApp is registered twice for the event
* handleMessage() on BrowserApp is called twice.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Woah, I just realized we register for Snackbar:Show events but we never unregister. Therefore EventDispatcher will hold references to all BrowserApp instances that ever existed and routes Snackbar events to them.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r?jchen

Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r?jchen

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

In addition to the patch here I wonder:

* Should EventDispatcher use SoftReferences[1] so that the reference can be cleared if only EventDispatcher holds one?

* Does it make sense to pass the same EventCallback to all listeners? A listener can not know if another listener has already invoked this callback. But maybe we currently just do not have multiple listeners that might invoke (the same) callbacks.

[1] http://developer.android.com/reference/java/lang/ref/SoftReference.html

Comment 12

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

Comment on attachment 8697284 [details]
MozReview Request: Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r?jchen

https://reviewboard.mozilla.org/r/27541/#review24811

Comment 13

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

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> In addition to the patch here I wonder:
> 
> * Should EventDispatcher use SoftReferences[1] so that the reference can be
> cleared if only EventDispatcher holds one?

I see an advantage in using SoftReferences if we have a consumer that has complex lifecycle (i.e. it cannot call unregisterListener reliably). For BrowserApp, its lifecycle is well-defined, so we should just make sure we call unregisterListener. What do you think?

> * Does it make sense to pass the same EventCallback to all listeners? A
> listener can not know if another listener has already invoked this callback.
> But maybe we currently just do not have multiple listeners that might invoke
> (the same) callbacks.

I think a lot of the JS code assumes the callback is only called once, so even if we allow multiple listeners to have separate callbacks, the JS side probably won't support it very well.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/c2de784e68f7348720cc336438b6c5ec9e3acc1c
Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r=jchen

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

NI to myself: This needs uplifting. But today is merge day so I need to figure out what branches/versions need the patch tomorrow.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2de784e68f7

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8697284 [details]
MozReview Request: Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r?jchen

Approval Request Comment

[Feature/regressing bug #]: Regression introduced with the introduction of the snackbar (bug 1215026; meta bug 1157526)

[User impact if declined]: App might crash after resuming the app multiple times and a snackbar is shown. This regression can have negative impact on memory consumption too because it prevents previous activities from being freed.

[Describe test coverage new/current, TreeHerder]: Local testing, very simple patch.

[Risks and why]: Very low risk. We registered for an event and never unregistered. This patch adds the call to unregister.

[String/UUID change made/needed]: -

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8697284 [details]
MozReview Request: Bug 1229153 - GeckoApp: Unregister from Snackbar events in onDestroy(). r?jchen

Crash fix, taking it on Aurora45 and Beta44.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is causing conflicts trying to uplift to aurora and beta due to a lack of bug 1107811. Can we either get a rebased patch for uplift or request 1107811 gets uplifted first to make this apply correctly?

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll prepare a patch that applies cleanly to aurora and beta.

However I'll also NI nalexander: Do you think we should consider uplifting bug 1107811? Maybe this will reduce the amount of work for future patch uplifts? But I'm not sure how isolated the changes in bug 1107811 are.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1229153-beta.patch

This is the patch for beta. For aurora it seems like we do not need a separate patch: The changes in bug 1107811 are in aurora. So the existing patch should apply cleanly.

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #20)
> I'll prepare a patch that applies cleanly to aurora and beta.
> 
> However I'll also NI nalexander: Do you think we should consider uplifting
> bug 1107811? Maybe this will reduce the amount of work for future patch
> uplifts? But I'm not sure how isolated the changes in bug 1107811 are.

The changes are perfectly isolated: there should be no changes to the generated .class files.

Since hg has no knowledge of directories, you might need to run some follow-up |hg mv| commands, or re-run the commands (in the commit messages).  I can take this task if we want it.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d507d2d787fc

Comment 24

[u421692]

Verified as fixed on Firefox 44 Beta 1 on Alcatel One Touch 8008D following the STR from Comment 4

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f66bf4c67f3

Comment 26

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using steps from comment 4:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 46.0a1 (2015-12-22)

Comment 27

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using steps from comment 4:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0a2 (2015-12-23)