If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
General
P1
critical
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: sebastian, Assigned: droeh)

Tracking

(Blocks: 2 bugs, {crash})

unspecified
Firefox 55
All
Android
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 disabled, firefox54 disabled, firefox55 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

9 months ago
This bug was filed from the Socorro interface and is 
report bp-8c7e5664-d7b3-4ba2-8962-d5b362161221.
=============================================================
(Reporter)

Comment 1

9 months ago
This is happened will switching between browser and custom tabs afaik.
(Reporter)

Comment 2

8 months ago
Just happend again.

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

Comment 3

8 months ago
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

8 months ago
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.
(Assignee)

Comment 5

7 months ago
Created attachment 8845594 [details] [diff] [review]
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.
Assignee: nobody → droeh
Attachment #8845594 - Flags: review?(s.kaspari)
(Reporter)

Updated

7 months ago
Attachment #8845594 - Flags: review?(s.kaspari) → review+

Comment 6

7 months ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8448c330bab
Set mDoorHangerPopup to null after destroying it. r=sebastian

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a8448c330bab
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
status-firefox52: --- → unaffected
status-firefox53: --- → disabled
status-firefox54: --- → disabled
status-firefox-esr52: --- → unaffected
Still seems to happen in latest Nightly (e.g. bp-49acd1c2-3c4f-4269-8b49-b81d12170322)
Status: RESOLVED → REOPENED
Flags: needinfo?(droeh)
Resolution: FIXED → ---
(Assignee)

Comment 9

6 months ago
(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.
Flags: needinfo?(droeh)

Comment 10

6 months ago
Hi Julian 
I think you are working on CustomTab now. Do you have any idea ?
Flags: needinfo?(walkingice0204)

Comment 11

6 months ago
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
Flags: needinfo?(s.kaspari)
In CustomTabs the code just extends GeckoApp and haven't touch anything relate to DoorHanger.
Flags: needinfo?(walkingice0204)
(Reporter)

Comment 13

6 months ago
(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.
Flags: needinfo?(s.kaspari)

Comment 14

6 months ago
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.
Flags: needinfo?(droeh)
(Assignee)

Comment 15

6 months ago
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.
Flags: needinfo?(droeh)

Updated

6 months ago
Duplicate of this bug: 1352378

Comment 17

6 months ago
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.
Blocks: 1351739
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

6 months ago
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.
Attachment #8853740 - Flags: feedback-
(Reporter)

Comment 21

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

Looks like Jan has this covered.
Attachment #8853740 - Flags: review?(s.kaspari)
Blocks: 1285858
Priority: -- → P1
(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?
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 23

5 months ago
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.
Flags: needinfo?(s.kaspari)

Comment 24

5 months ago
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...
(Assignee)

Comment 25

5 months ago
Created attachment 8864581 [details] [diff] [review]
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.
Attachment #8864581 - Flags: review?(nchen)
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).
Attachment #8864581 - Flags: review?(nchen) → review+

Comment 27

5 months ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85d25fa905b
Reduce reliance on GeckoAppShell.getGeckoInterface to solve some custom tabs crashes. r=jchen

Comment 28

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d85d25fa905b
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago5 months ago
Resolution: --- → FIXED
Blocks: 1363167
No longer blocks: 1363167
Blocks: 1363457
You need to log in before you can comment on or make changes to this bug.