1325021 - Crash in java.lang.IllegalArgumentException: Doorhanger:Add was not registered at org.mozilla.gecko.EventDispatcher.unregisterListener(EventDispatcher.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This bug was filed from the Socorro interface and is 
report bp-8c7e5664-d7b3-4ba2-8962-d5b362161221.
=============================================================

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This is happened will switching between browser and custom tabs afaik.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Just happend again.

https://crash-stats.mozilla.com/report/index/9890e681-40e1-4051-b75c-a15872170202

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

This is weird. We register in the constructor of DoorHangerPopup and unregister when calling destroy(). There's no way to call destroy() without constructing the object first - so why are we not registered anymore?

Comment 4

[Jing-wei Wu [:jwu]]

DoorHangerPopup is initialized in onGlobalLayout(). Not sure is there any document or code guarantees that onGlobalLayout() will always be triggered before onDestroy() is called?

If onDestroy() is called without calling onGlobalLayout() before, this exception would be happened.

Comment 5

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

Attachment: Set mDoorHangerPopup to null after destroying it.

Here's a simple fix to make sure we do not call destroy() on the DoorHangerPopup multiple times, which will at least prevent this from being a crash.

The bigger issue here seems to be that we are hitting BrowserApp/GeckoApp.onDestroy() multiple times in a row without hitting GeckoApp.initialize() in between -- I haven't been able to reproduce this or figure out how it is occurring.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8448c330bab
Set mDoorHangerPopup to null after destroying it. r=sebastian

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

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

Still seems to happen in latest Nightly (e.g. bp-49acd1c2-3c4f-4269-8b49-b81d12170322)

Comment 9

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #8)
> Still seems to happen in latest Nightly (e.g.
> bp-49acd1c2-3c4f-4269-8b49-b81d12170322)

Yeah. This particular bug is essentially the same as bug 1338055*; unfortunately, fixing it just leads to another crash when taking the same steps. I'll likely land a single patch to handle this and the other crashes in bug 1347584 when I have them figured out.

* - Except instead of swiping to kill a custom tab while Fennec is open, we trigger this by swiping to kill Fennec while a custom tab is open.

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Hi Julian 
I think you are working on CustomTab now. Do you have any idea ?

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Hi Sebastian
Please help look at this code..[0]
Do you think the collection / remove logic could be the culprit?


[0] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java#160

Comment 12

[Julian Chu [:walkingice]]

In CustomTabs the code just extends GeckoApp and haven't touch anything relate to DoorHanger.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nevin Chen [:nechen] from comment #11)
> Hi Sebastian
> Please help look at this code..[0]
> Do you think the collection / remove logic could be the culprit?

Yeah, that is very likely. Most of those components are written with the constraint that there will always only be one BrowserApp/GeckoApp instance. However now with custom tabs and web apps this is no longer true.

We register for the event in the constructor of DoorHangerPopup (via GeckoApp). However we only call destroy() from BrowserApp and never from CustomTabsActivity. This looks wrong. Apart from that we should audit that both activities (or multiple in general) can register/unregister and do not interfere with each other.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Hi Dylan 
Per comment 13, I think the foundamental fix is to use the safer way to remove items in the collection.
Maybe you can add the fix to your patch? Please feel free to let me know how do you think.

Comment 15

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

So the problem that's happening here is that we're trying to remove listeners from an EventDispatcher that belongs to a separate activity; I don't think a change in how we remove items from the collection would be able to fix this; at best it would be papering over the bug.

I'm trying to address all the crashes that happen when you swipe-to-kill Fennec while focused on a custom tab, and part of that will be making sure that the listener unregistering that happens in BrowserApp.onDestroy() will be done on the correct EventDispatcher; another part, which Sebastian brought up, is making sure there is creation/destruction parity -- nothing should be created in GeckoApp and destroyed in BrowserApp or vice versa, etc.

Comment 17

[Jan Henning [:JanH]]

Also seen during development of bug 1351739 when enabling "Don't keep activities".
There, I'm launching a CustomTabsActivity from inside BrowserApp, so we end up with a sequence of
- BrowserApp onPause
- CustomTabs onCreate/Start/Resume
- BrowserApp onStop/Destroy and crash,

which I guess is the same sequence that happens when you keep activities, but then manually swipe the background activity away.

Comment 18

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1325021 - Safely removing the listener in the collection to prevent concurrency issues.

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

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8853740 [details]
Bug 1325021 - Safely removing the listener in the collection to prevent concurrency issues.

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

Comment 20

[Jan Henning [:JanH]]

Comment on attachment 8853740 [details]
Bug 1325021 - Safely removing the listener in the collection to prevent concurrency issues.

In the situation described above in comment 17, this now crashes with

> 04-02 22:32:19.426 14616-14616 E/GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
> java.lang.UnsupportedOperationException
>    at java.util.concurrent.CopyOnWriteArrayList$CowIterator.remove(CopyOnWriteArrayList.java:744)
>   at org.mozilla.gecko.EventDispatcher.safelyRemove(EventDispatcher.java:181)
>    at org.mozilla.gecko.EventDispatcher.unregisterListener(EventDispatcher.java:168)
>    at org.mozilla.gecko.EventDispatcher.unregisterGeckoThreadListener(EventDispatcher.java:221)
>    at org.mozilla.gecko.GeckoApp.onDestroy(GeckoApp.java:2425)
>    at org.mozilla.gecko.customtabs.CustomTabsActivity.onDestroy(CustomTabsActivity.java:152)
>    at android.app.Activity.performDestroy(Activity.java:6430)
>    at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1165)
>    at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:3855)
>    at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:3886)
>    at android.app.ActivityThread.-wrap5(ActivityThread.java)
>    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1411)
>    at android.os.Handler.dispatchMessage(Handler.java:102)
>    at android.os.Looper.loop(Looper.java:148)
>    at android.app.ActivityThread.main(ActivityThread.java:5459)
>    at java.lang.reflect.Method.invoke(Native Method)
>    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:728)
>    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:618)
>    at de.robv.android.xposed.XposedBridge.main(XposedBridge.java:102)

Actually in fact it's even worse than before, because now it crashes already when switching from custom tab to BrowserApp, whereas previously only the other route was affected. Besides, even without the above crash, your patch doesn't look like it solves the fundamental problem: The EventDispatcher wants to enforce (by crashing if necessary) that registering and unregistering of event listeners is done properly.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8853740 [details]
Bug 1325021 - Safely removing the listener in the collection to prevent concurrency issues.

Looks like Jan has this covered.

Comment 22

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Sebastian Kaspari (:sebastian) from comment #21)
> Comment on attachment 8853740 [details]
> Bug 1325021 - Safely removing the listener in the collection to prevent
> concurrency issues.
> 
> Looks like Jan has this covered.

If this is no longer happening, we can close the bug.
Can you confirm?

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

I've been testing Focus as my primary browser lately - So I do not have much personal experience. Looking at the crash reports there's at least build 20170503100516 affected.

Comment 24

[Jan Henning [:JanH]]

The saving grace is that these are diagnostic crashes (https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java#129), so at least we won't crash on Beta/Release.
However as this means we're having some lifecycle issues with these event listeners, depending on how these occur exactly we might or might not experience some other doorhanger related issues on Beta/Release instead...

Comment 25

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

Attachment: Reduce reliance on GeckoAppShell.getGeckoInterface

Right, I thought we could sidestep this with GeckoView-based custom tabs, but we actually run into a similar issue there as well. Here's a patch that should solve both.

I was aiming to remove GeckoApp.getEventDispatcher entirely, but unfortunately it's quite difficult to remove it from FormAssistPopup; we rely on being able to get the (instance) EventDispatcher in onAttachedToWindow there, and I don't see any obvious way of passing a GeckoApp reference to it before that happens. There are also some calls to GeckoApp.getEventDispatcher in testing, but I'm not so worried about those.

Comment 26

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

Comment on attachment 8864581 [details] [diff] [review]
Reduce reliance on GeckoAppShell.getGeckoInterface

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

For FindInPageBar and FormAssistPopup, see if you can get the GeckoApp instance from the views themselves (see http://stackoverflow.com/a/32973351).

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85d25fa905b
Reduce reliance on GeckoAppShell.getGeckoInterface to solve some custom tabs crashes. r=jchen

Comment 28

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

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

Comment 29

[Oana Horvath [:ohorvath]]

The crash is not reproducing anymore, the changes made here were applied to a different implementation of PWAs, we can close the bug as fixed.