1338055 - Crash in java.lang.IllegalArgumentException: Prompt:Show was not registered at org.mozilla.gecko.EventDispatcher.unregisterListener(EventDispatcher.java)

Status: RESOLVED 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-5456300e-ebdb-45c1-839e-17cb82170208.
=============================================================

Stack shows this happens when custom tabs activity is destroyed.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Right, I'm looking into this now.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch

This just sets mPromptService to null after destroying it so that it will not get destroyed a second time. Should stop the crash from occurring, but it is odd that it's happening to begin with; I don't think we should be able to hit GeckoApp.onDestroy() twice without calling GeckoApp.initialize() in between, which seems to be what is happening here.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I think there should be a duplicate somewhere. I remember finding it from crash-stats.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> I think there should be a duplicate somewhere. I remember finding it from
> crash-stats.

The other one was actually about "Doorhanger:Add" (bug 1325021).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3692a0e5a43
Prevent crash from destroying PromptService multiple times. r=sebastian

Comment 6

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

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

Comment 7

[Jan Henning [:JanH]]

Still crashing with today's Nightly (2017-03-11), e.g. https://crash-stats.mozilla.com/report/index/7c59d86f-a658-45e1-b6cb-fe7b02170311.

STR:
1. Open Firefox.
2. Open a custom tab.
3. Switch back to the main Firefox activity.
4. Swipe away the custom tab in the task switcher.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Thanks for the STR, that helped me figure out what is actually going on here:

When we are in Fennec and we swipe to kill a custom tab in the app switcher, we call onDestroy() on the GeckoApp belonging to the custom tab and destroy() on the PromptService belonging to the custom tab, but when we go to unregister the events we use GeckoApp.getEventDispatcher(); this relies on GeckoAppShell.getGeckoInterface() to get the GeckoApp, which unfortunately returns the GeckoApp belonging to Fennec.

(This is because we call setGeckoInterface() in GeckoApp.onResume(), and the custom tab does not get resumed before being destroyed.)

I'm looking right now to see if there is a nice way around this, but in either case it will be fixed for free when we switch to a GeckoView-based custom tab client.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Attachment: Update active GeckoInterface in GeckoApp.onDestroy()

OK, simple but slightly odd fix: update the 'active' GeckoInterface in GeckoApp.onDestroy(). This way we get the correct GeckoApp from GeckoAppShell.getGeckoInterface() during destruction, and when we switch back to any other GeckoApp-activity we set the GeckoInterface again in onResume() anyways. I also moved super.onDestroy() in BrowserApp.onDestroy() so that a similar issue with mDoorHangerPopup and similar can be avoided.

As far as I can tell, I don't think this should break anything, but it's possible there's something subtle I'm overlooking -- setting the active GeckoInterface in onDestroy() definitely feels odd.

Comment 10

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

Comment on attachment 8847653 [details] [diff] [review]
Update active GeckoInterface in GeckoApp.onDestroy()

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

Sounds like you should just keep an EventDispatcher reference inside PromptService.

I don't think we can guarantee that `CustomTabsActivity.onDestroy` is always called while BrowserApp is not already the active activity. Otherwise, if `onDestroy` is called while `BrowserApp` is active, `BrowserApp.onResume` won't be called because it's already active. Also, this leaks CustomTabsActivity beyond its lifetime, which is not ideal. If nothing else, you should save the previous GI and restore it afterwards.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: Use stored context to get instance EventDispatcher in PromptService

Right, we already store Context in PromptService, so I'm just using that to get the instance EventDispatcher in this patch. I think there are probably other problems with the same root cause (getting the wrong GeckoInterface from GeckoAppShell) -- in particular there definitely will be similar cases in bug 1347584. I kind of hate to have to spread Context/EventDispatcher references all over the place, so I'll see if any nicer approach presents itself while I'm working on that bug.

Comment 12

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

Comment on attachment 8849131 [details] [diff] [review]
Use stored context to get instance EventDispatcher in PromptService

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

::: mobile/android/base/java/org/mozilla/gecko/prompts/PromptService.java
@@ +20,5 @@
>      private static final String LOGTAG = "GeckoPromptService";
>  
>      private final Context mContext;
>  
>      public PromptService(Context context) {

Just make the parameter GeckoApp, and store it as mGeckoApp, instead of mContext.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b4a8e63215
Store a reference to the instance EventDispatcher in PromptService. r=jchen

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/63b4a8e63215