Closed Bug 1302936 Opened 8 years ago Closed 8 years ago

Crash in java.lang.IllegalArgumentException: onTabChanged:OPENED_FROM_TABS_TRAY must specify a tab. at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java)

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(firefox49 unaffected, fennec50+, firefox50 wontfix, firefox51 fixed, firefox52+ fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox49 --- unaffected
fennec 50+ ---
firefox50 --- wontfix
firefox51 --- fixed
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: jchen, Assigned: twointofive)

Details

(Keywords: crash, Whiteboard: [TPE-1])

Crash Data

Attachments

(2 files)

Bug 1302936 - Ignore tab clicks when the tab has already been removed.
58 bytes, text/x-review-board-request
sebastian
: review+
jcristau
: approval-mozilla-aurora+
Details
Patch for 51 Beta
5.54 KB, patch
sebastian
: review+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review 
This bug was filed from the Socorro interface and is 
report bp-c7f3ea1a-7f80-47f1-83aa-9a7e22160915.
=============================================================

Crashes for a given Nightly all come from a single installation, but it looks like it could be a real bug.

> java.lang.IllegalArgumentException: onTabChanged:OPENED_FROM_TABS_TRAY must specify a tab.
> 	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:650)
> 	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:644)
> 	at org.mozilla.gecko.tabs.TabsListLayout$TabSwipeGestureListener.onTouch(TabsListLayout.java:502)
> 	at android.view.View.dispatchTouchEvent(View.java:9935)
> 	at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2663)
> 	...
Flags: needinfo?(s.kaspari)
Let's look at this in triage. It doesn't seem to happen often actually.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
tracking-fennec: ? → 50+
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Whiteboard: [TPE-1]
I think the error happened here https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#662

I can't reproduce the situation when tab will pass in null and msg not equals TabEvents.RESTORED.

Or maybe we should just return and not throw an exception here since there's nothing we can do to recover this situation.
Flags: needinfo?(s.kaspari)
This area of the code recently changed: We moved from a ListView implementation to a RecyclerView implementation in bug 1116415. However this crash still seems to happen, but with a slightly different stack trace [1]:

> java.lang.IllegalArgumentException: onTabChanged:OPENED_FROM_TABS_TRAY must specify a tab.
>	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:662)
>	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:656)
>	at org.mozilla.gecko.tabs.TabsLayout.onItemClicked(TabsLayout.java:138)
>	at org.mozilla.gecko.widget.RecyclerViewClickSupport$1.onClick(RecyclerViewClickSupport.java:27)
>	at android.view.View.performClick(View.java:5273)

This is the related code:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java#132

We select the clicked tab (by id) and then we try to get the tab by id and notify the listeners.

Two things here:
 * We do not need to select the tab by ID. Select tab should return the selected tab.
 * If the tab could not selected (Maybe a click can happen while the tab is being removed?) then we should continue

@Tom: What do you think? Especially about a click happening when (or slightly before) the tab is gone.

[1] e.g. https://crash-stats.mozilla.com/report/index/ab64a683-3cbf-40e8-b850-a69242161109
Flags: needinfo?(s.kaspari) → needinfo?(twointofive)
(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> @Tom: What do you think? Especially about a click happening when (or
> slightly before) the tab is gone.

Yeah, I suppose it's possible this could happen if you manage to tap on a tab you just closed while the remove animation is running (I'd have to try it to be sure (I feel like I have already, but maybe not)).

> Two things here:
>  * We do not need to select the tab by ID. Select tab should return the
> selected tab.
>  * If the tab could not selected (Maybe a click can happen while the tab is
> being removed?) then we should continue

I agree those are the (easy) fixes.  I can likely get to it this afternoon, otherwise I can definitely do it this weekend.  If that's too late to get it in before the merge deadline and that's a concern, anybody feel free to do it for me. :)
Flags: needinfo?(twointofive)
There's a third trace in the crash reports, coming from basically the same bug in the case where the tabs panel is a grid view instead of a list view (so tablets or phones in landscape) - see https://crash-stats.mozilla.com/report/index/f29f6b66-db73-4d17-8f71-430602161112 for example:

java.lang.IllegalArgumentException: onTabChanged:OPENED_FROM_TABS_TRAY must specify a tab.
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:641)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:635)
	at org.mozilla.gecko.tabs.TabsGridLayout$TabSwipeGestureListener.onTouch(TabsGridLayout.java:584)
	at android.view.View.dispatchTouchEvent(View.java:9371)
Assignee: nobody → twointofive
STR in the comment 3 (RecyclerView  tabs list) case:
1) Open some tabs (two is simplest for repeated close and then undo-close attempts).
2) Go to the tabs tray and do a just-slightly-slow double click on the close button of the second tab (or any tab).  If you click too fast you just click the close button twice, which is tolerated; if you click too slow then your second click is just a regular old click on nothing, which just causes the "do you want to undo the tab close?" snackbar to disappear.  If you click just right then the second click is on the part of the tab view that isn't the close button, and then you get a crash.

In the case where you wind up clicking twice on the close button, the second click actually does produce a null tab here: 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java#167
since the tab was already removed from the Tabs list by the first click, but in that case the subsequent 'Tabs.getInstance().closeTab(tab, true);' call we make checks for a null tab, so no harm is done.  I've still added an early return in the null case there though just for future proofing, and also because we could wind up calling autoHidePanel twice in that case (I think, I haven't checked), which is probably safe, but now that's two things that need to be safe...
I haven't been able to produce a crash in the comment 0 (pre bug 1116415 TwoWayView tabs list) case, or in the current tabs grid panel case.  The changes here in TabsGridLayout are simply to check for a null tab before calling Tabs.notifyListeners, so they should certainly prevent this crash, and it seems likely that the crash is occurring in some scenario similar to the STR in comment 8, so not calling autoHidePanel() when there's no tab should be the right thing to do.

Note that I'm only seeing a crash signature for the notifyListeners call at [1] in the grid case, but for a tap on the tabs grid, both the registered swipe listener and the registered click listener process the tap, so I think we should close the potential hole in the click listener at [2] as well (which I do in the patch) since otherwise we might just wind up moving the crash from [1] to [2].

If we want to uplift this to soon-to-be-beta I'll need to produce another patch for the TwoWayView tabs list case there.

[1]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java#583
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java#110
Comment on attachment 8810117 [details]
Bug 1302936 - Ignore tab clicks when the tab has already been removed.

https://reviewboard.mozilla.org/r/92514/#review92934

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java:135
(Diff revision 2)
>          final Tabs tabs = Tabs.getInstance();
> -        tabs.selectTab(tabId);
> +        final Tab tab = tabs.selectTab(tabId);

nit: There's no need to have a "tabs" reference anymore, I guess.
Attachment #8810117 - Flags: review?(s.kaspari) → review+
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/71f75f75b4e7
Ignore tab clicks when the tab has already been removed. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/71f75f75b4e7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
[Tracking Requested - why for this release]: Is the 5th top-crasher on aurora.

If I'm understanding things correctly this crash signature spiked with the landing of Bug 1116415 (which is in 52), but this  fix landed in 53  - Tom: can we uplift this to aurora/53?

(In case you're not familiar with the uplift process yet: there's a detailed description at https://wiki.mozilla.org/Release_Management/Uplift_rules - the TL;DR: version is that you set the "approval‑mozilla‑aurora" flag on the patch which triggers the uplift process.)
tracking-firefox52: --- → ?
Flags: needinfo?(twointofive)
Comment on attachment 8810117 [details]
Bug 1302936 - Ignore tab clicks when the tab has already been removed.

Approval Request Comment
[Feature/Bug causing the regression]: This crash existed before bug 1116415, but that bug made it easier to produce the crash.
[User impact if declined]: Possible crash when slow double tapping the close button on a tab or tapping a tab while its close animation is running (those are the known ways to reproduce), or some variation of tapping on a tab after it's been closed but hasn't yet disappeared from the screen.
[Is this code covered by automated tests?]: No.  
[Has the fix been verified in Nightly?]: There are three crash signatures in the crash reports, two present on aurora: one when the tabs panel is a list, and one when it's a grid.  The list fix has been verified on Nightly.  The grid fix is speculative since we don't have steps to reproduce.  There's only one report from 53.0a1, and if I'm reading the crash buildid and patch push dates correctly, the report is from before this patch landed on 53.
[Needs manual test from QE? If yes, steps to reproduce]: Steps to reproduce the list case are in comment 8 - they should be followed on a phone in portrait mode.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Fairly low risk.
[Why is the change risky/not risky?]: In both the list and grid cases, we're fixing the crash by adding checks to make sure a tab is not null.  We can't reproduce the crash in the grid case, but the fix here should still be a safe and good thing to do in any case, and should prevent this crash signature.
[String changes made/needed]: None.
Flags: needinfo?(twointofive)
Attachment #8810117 - Flags: approval-mozilla-aurora?
fennec topcrash, tracking for 52
tracking-firefox52: ? → +
Comment on attachment 8810117 [details]
Bug 1302936 - Ignore tab clicks when the tab has already been removed.

stability fix, seems to be verified on nightly, take in aurora
Attachment #8810117 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Guys, I see that release is quite impacted too. Shouldn't we uplift that to 51 too? Thanks
status-firefox50: affected → wontfix
Flags: needinfo?(twointofive)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(ahunt)
Yeah, that would be nice. However we recently refactored that part of the code, so I'm not sure if it's easy to apply the fix to the Beta branch. Tom, what do you think?
Flags: needinfo?(s.kaspari)
Yeah, I'll need to dig into it again, but it's looking like for beta we'd just need to add what we did for the grid case in aurora here:

https://hg.mozilla.org/releases/mozilla-aurora/rev/76cf4e84939e#l2.43

to the list case in beta here:

https://dxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java#502

(the list case doesn't have a separate click listener like the grid case did, so the other aurora change in TabsGridLayout won't be needed for the list case).

That seems pretty safe, given that the similar grid change in aurora hasn't given us any problems - what do you think Sebastian?  I can submit a patch for more careful review.
Flags: needinfo?(twointofive)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(ahunt)
Yeah, sounds good. Let's create a patch.
Flags: needinfo?(s.kaspari)
(As I mentioned on IRC, this patch was tested using a full mach build on beta, but the gradle build was failing on beta trunk (errors here: https://pastebin.mozilla.org/8954136), so this hasn't been through a gradle build.  As far as I know that has no relevance as far as this patch and bug are concerned, but I'm mentioning it anyway just in case.)

For future reference, does reviewboard support separate patches for different releases?  I didn't want to try it and make a mess...
Attachment #8820890 - Flags: review?(s.kaspari)
The try run has some persistent eslint errors unrelated to this patch.  Also I should have noted yesterday that, as with the grid case in all channels, I'm unable to reproduce the crash in the list case on beta, so the patch is speculative in both the list and grid cases on beta (though the same patch in the grid case has proven effective on aurora).
(In reply to Tom Klein from comment #23)
> (As I mentioned on IRC, this patch was tested using a full mach build on
> beta, but the gradle build was failing on beta trunk (errors here:
> https://pastebin.mozilla.org/8954136), so this hasn't been through a gradle
> build.  As far as I know that has no relevance as far as this patch and bug
> are concerned, but I'm mentioning it anyway just in case.)

Yep, that's correct. Our actual builds do not use gradle (although some automation tasks do).

In your case you are hitting the dex limit what we have seen in beta builds every now and then. In your case using 

> app:assembleLocalDebug

instead of 

> app:assembleLocal***Old***Debug

should help.

> For future reference, does reviewboard support separate patches for
> different releases?  I didn't want to try it and make a mess...

That's a good question and I actually never tried - and always uploaded patch files for uplifts. If you want to know, you can find the folks working on it in #mozreview on IRC. They have been quite responsive in the past. :)
Comment on attachment 8820890 [details] [diff] [review]
Patch for 51 Beta

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

LGTM. Thanks!
Attachment #8820890 - Flags: review?(s.kaspari) → review+
Comment on attachment 8820890 [details] [diff] [review]
Patch for 51 Beta

Approval Request Comment
[Feature/Bug causing the regression]: Existed since the OPENED_FROM_TABS_TRAY event was introduced (bug 1278980), as far as I know (I haven't verified that). 
[User impact if declined]: Possible crash during some variation of tapping on a tab after it's been closed but hasn't yet disappeared from the screen.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: There is a list case and a grid case that are being patched; the grid case patch is the same in all channels, the list case patch is different on beta (though more similar to the grid case in beta than in the other channels). There have been no crashes with this signature on nightly since the original patch landed, and no crashes on aurora since the aurora patch landed.  
[Needs manual test from QE? If yes, steps to reproduce]: There are no steps to reproduce on beta; the patch is speculative.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Fairly low risk.
[Why is the change risky/not risky?]: In both the list and grid cases, we're fixing the crash by adding checks to make sure a tab is not null.  We can't reproduce the crash, but the fix here should still be a safe and good thing to do in any case, and should prevent this crash signature.
[String changes made/needed]: None.
Attachment #8820890 - Flags: approval-mozilla-beta?
Comment on attachment 8820890 [details] [diff] [review]
Patch for 51 Beta

This signature is a top crash on 50.1 release but only shows up as about 50 crashes/week on beta 51. Let's uplift this for beta 11.
Attachment #8820890 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No crashes were encountered in today's beta testing session (51.0b11 / January 4th, 2017). 
I will keep this issue opened until we have more crash stats information after the build gets to more users, post playstore launch.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: