1525796 - If you press the X button to close the page it becomes "Ghostly"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Overlays, defect, P2)

Description

[Galo]

Attachment: Screenshot_20190206-224130.png

User Agent: Mozilla/5.0 (Android 9; Mobile; rv:65.0) Gecko/65.0 Firefox/65.0

Steps to reproduce:

Go to the "tabs" section. Then, keep pressed the "X" button to close a tab for ahout a second.
Additional info: This happened on a Xioami MI A1 with Android 9. However the bug still appeared in Android 8.1

Actual results:

The tab will not disappear unless the app is closed. New tabs will overlap the "Ghost" tab.

Expected results:

Either nothing, or the tab gets closed.

Comment 1

[Stefan Deiac]

Hi, thanks for your report.
I tried to reproduce your issue using Nexus 6P (Android 8.1.0) and Google Pixel (Android 9) on Nightly (67.0a1 2019-02-06) and Release 65.0 but without any success. (please note that I tried several times)
Do you have some specific options or add-ons? Also, if you can retry with a fresh install it will be very awesome. I'll wait for your updates, thanks.

Comment 2

[Galo]

(In reply to Stefan Deiac from comment #1)

Hi, thanks for your report.
I tried to reproduce your issue using Nexus 6P (Android 8.1.0) and Google Pixel (Android 9) on Nightly (67.0a1 2019-02-06) and Release 65.0 but without any success. (please note that I tried several times)
Do you have some specific options or add-ons? Also, if you can retry with a fresh install it will be very awesome. I'll wait for your updates, thanks.

HI. I uninstalled and installed Firefox again and, having no add ons or special configurations, the bug still happened. I recorded a video of it: https://youtu.be/70T9OFhSqVY

Comment 3

[Stefan Deiac]

Hi, thanks for your updates. I tried another one time with OnePlus 5T (Android 9) and Samsung Galaxy S8 (Android 8.0) and I can confirm the issue.
Builds:

• Nightly 67.0a1 (2019-02-06);
• Beta 66.0b5;
• Release 65.0.

Notes:
-It seems that is 4/5 attempts reproducible.
-The 'Ghost' tabs disappear after closing and reopening the app.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

From discussing with Stefan I understand this is not a recent regression - the issue exists also on FF60.
Thanks for reporting.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1525796 - Ignore close tab action if user is moving the item; r?JanH

Using an ItemTouchHelper for various motion actions means it will have a
strong reference to that item's layout and prevent it from being destroyed when
the adapter tries to remove it before the animations are finished - the item
will be removed from RecyclerView's Adapter but the item's layout will still
remain on screen.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

This was a rabbit hole but I found that our TabsTouchHelperCallback is at fault here, it leaking the itemView for even after the tab was removed from the adapter.

Timeline showing the leak (with my logs added in code, hyperlinks to said logs position):
13:09:34.007 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: true
13:09:34.029 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.051 Trying to remove item #5 from 8 total: Glowing reviews tout counterfeit cash on the dark web - Los Angeles Times
13:09:34.052 We are left with 7 items
13:09:34.085 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.191 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.208 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.230 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.259 DRAW: action: ACTION_STATE_DRAG, isCurrentlyActive: false
13:09:34.273 User interaction with "Glowing reviews tout counterfeit cash on the dark web - Los Angeles Times" holder is finished

It is clearly seen that the itemHolder is still in TabsTouchHelperCallback even with 200ms after it should have been removed.

Unfortunately nothing I've tried (including getItemAnimator().isRunning() or getItemAnimator().endAnimation(item)) worked well so to reconcile both events - closing the tab and interacting with it I've created a new method in our TabsTouchHelperCallback - isInteractionInProgress which will inform if there is any interaction with RecyclerView's items in progress, including the time needed to finish return animations, and if so we will just ignore the close action.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed60c55e6f00
Ignore close tab action if user is moving the item; r=JanH

Comment 8

[Cristina Coroiu [:ccoroiu]]

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

Comment 9

[Tom Klein]

Is bug 1382125 a dup?

Comment 10

[Petru-Mugurel Lingurar [:petru]]

(In reply to Tom Klein from comment #9)

Is bug 1382125 a dup?

Seems like the same issue but happening in other part of the code.
Won't be fixed by this patch but the solution should be similar.

Comment 11

[Stefan Deiac]

Verified as fixed on the latest version of Nightly (67.0a1 2019-02-18) using OnePlus 5T (Android 9).

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Given the low test coverage here I think it is likely best to let this ride with nightly. If you strongly feel it should be in 66 and is safe for uplift, let me know.