Last Comment Bug 702619 - Swiftly closing two tabs at once results in an NPE at org.mozilla.gecko.TabsTray$TabsAdapter.<init>(TabsTray.java:138)
: Swiftly closing two tabs at once results in an NPE at org.mozilla.gecko.TabsT...
Status: VERIFIED FIXED
[native-crash:P1]
: crash, reproducible
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
P2 critical (vote)
: ---
Assigned To: Sriram Ramasubramanian [:sriram]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 07:21 PST by Cristian Nicolae (:xti)
Modified: 2016-07-29 14:20 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch (2.34 KB, patch)
2011-11-18 14:09 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Splinter Review
Patch 2: Waits for gecko response before allowing further tab close. (4.48 KB, patch)
2011-11-18 14:58 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
Patch (4.91 KB, patch)
2011-11-21 12:35 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review

Description User image Cristian Nicolae (:xti) 2011-11-15 07:21:54 PST
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111115
Firefox/11.0a1 Fennec/11.0a1
Devices: Motorola Droid 2
OS: Android 2.3.3

Steps to reproduce:
1. Open Fennec App
2. Browse to a couple of webpages, each one in a different tab
3. Tap on the Tab Menu button
4. Quick tap on the " x " button for each tab

Expected result:
The tabs are closed. No crash occurs after step 4.

Actual result:
After step 4, Fennec crashes as you can see in the following video: http://youtu.be/sa_odU4tBDc.

Note:
- here is the alogcat log: http://pastebin.mozilla.org/1383240
- crash report: https://crash-stats.mozilla.com/report/index/bp-a0871477-9f2c-422b-9a1f-e73652111115
Comment 1 User image Aaron Train [:aaronmt] 2011-11-15 07:32:07 PST
So that it's not lost in a day, the crash:

E/GeckoApp(17076): java.lang.NullPointerException
E/GeckoApp(17076):      at org.mozilla.gecko.TabsTray$TabsAdapter.<init>(TabsTray.java:138)
E/GeckoApp(17076):      at org.mozilla.gecko.TabsTray.onTabsChanged(TabsTray.java:124)
E/GeckoApp(17076):      at org.mozilla.gecko.GeckoApp.onTabsChanged(GeckoApp.java:639)
E/GeckoApp(17076):      at org.mozilla.gecko.GeckoApp$11.run(GeckoApp.java:823)
E/GeckoApp(17076):      at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoApp(17076):      at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoApp(17076):      at android.os.Looper.loop(Looper.java:123)
E/GeckoApp(17076):      at org.mozilla.gecko.GeckoApp$21.run(GeckoApp.java:1080)
E/GeckoApp(17076):      at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoApp(17076):      at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoApp(17076):      at android.os.Looper.loop(Looper.java:123)
E/GeckoApp(17076):      at android.app.ActivityThread.main(ActivityThread.java:3806)
E/GeckoApp(17076):      at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoApp(17076):      at java.lang.reflect.Method.invoke(Method.java:507)
E/GeckoApp(17076):      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:839)
E/GeckoApp(17076):      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:597)
E/GeckoApp(17076):      at dalvik.system.NativeStart.main(Native Method)
D/GeckoAppJava(17076): IME: run()
D/GeckoAppJava(17076): IME: v=org.mozilla.gecko.gfx.LayerView@4066d540
Comment 2 User image Aaron Train [:aaronmt] 2011-11-15 12:03:13 PST
Reproduced this by tapping the 'x' buttons on two tabs swiftly while the tab pulldown menu was still open.

--
Samsung Nexus S (Android 2.3.6)
20111115111855
http://hg.mozilla.org/projects/birch/rev/252ec90b5eaf
Comment 3 User image Matt Brubeck (:mbrubeck) 2011-11-15 13:50:32 PST
I get slightly different behaviors when I vary the steps.  For example, if only two tabs are open and I close both of them quickly, then fennec crashes with a NullPointerException at org.mozilla.gecko.BrowserToolbar.setFavicon(BrowserToolbar.java:187).

Another time when I had three tabs open and closed two quickly, I didn't get a crash, but a got into a state where the tab button no longer changed from a "+" to a counter after opening new tabs, and it was impossible to switch or close tabs.
Comment 4 User image Sriram Ramasubramanian [:sriram] 2011-11-18 14:09:29 PST
Created attachment 575558 [details] [diff] [review]
Patch

I couldn't get the crash how fast I tried.
The problem here (I suspect) is that we are able to close to tab before we get an event to popup.

Close first tab > Send event to Gecko > Gecko responds back > onTabsChanged() > update the list > which closes the Tabs-list.
Now within this entire flow, somehow if the user manages to hit on the only close button, things go crazy.
I'm defensively closing the tabs-list, if there were just two in the list and a close button was hit -- without waiting for Gecko.
Comment 5 User image Sriram Ramasubramanian [:sriram] 2011-11-18 14:58:57 PST
Created attachment 575576 [details] [diff] [review]
Patch 2: Waits for gecko response before allowing further tab close.

This solution, as discussed, will wait for Gecko to confirm the tab close, before allowing other tabs to be closed. This prevents the last tab to be closed, as the activity closed in onTabsChanged(), thereby not ending up in NPE.
Comment 6 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-18 19:21:23 PST
Comment on attachment 575558 [details] [diff] [review]
Patch

This patvh is a bit too fragile. I don't like the specific check for 2 tabs and closing the tabs tray when closing a tab is not what UX would like either.
Comment 7 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-18 19:26:01 PST
Comment on attachment 575576 [details] [diff] [review]
Patch 2: Waits for gecko response before allowing further tab close.


>                 close.setOnClickListener(new Button.OnClickListener() {
>                     public void onClick(View v) {
>+                        if (mWaitingForClose)
>+                            return;

Could we put the | mWaitingForClose = true; | right here? Just to keep the gap of possible re-entry closed.

>                         int tabId = Integer.parseInt("" + v.getTag());

Old code, but could you change 

"" + v.getTag()

to

v.getTag().toString()  ?

r+ with nits fixed
Comment 8 User image Lucas Rocha (:lucasr) 2011-11-21 02:34:44 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> Created attachment 575576 [details] [diff] [review] [diff] [details] [review]
> Patch 2: Waits for gecko response before allowing further tab close.
> 
> This solution, as discussed, will wait for Gecko to confirm the tab close,
> before allowing other tabs to be closed. This prevents the last tab to be
> closed, as the activity closed in onTabsChanged(), thereby not ending up in
> NPE.

I haven't looked at the patch in detail but this feels like a wrong approach to fix the bug if we want to keep UI responsive. We should never block user interaction to solve internal app state issues.

We should change UI to react to user interaction (click on close button) immediately and do internal management of tabs internally (remove tab from list immediately and wait it to close separately, for example). Yes, this might make code a bit more complex but it's the right thing to do for our users.
Comment 9 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 05:31:02 PST
(In reply to Lucas Rocha (:lucasr) from comment #8)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> > Created attachment 575576 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch 2: Waits for gecko response before allowing further tab close.
> > 
> > This solution, as discussed, will wait for Gecko to confirm the tab close,
> > before allowing other tabs to be closed. This prevents the last tab to be
> > closed, as the activity closed in onTabsChanged(), thereby not ending up in
> > NPE.
> 
> I haven't looked at the patch in detail but this feels like a wrong approach
> to fix the bug if we want to keep UI responsive. We should never block user
> interaction to solve internal app state issues.
> 
> We should change UI to react to user interaction (click on close button)
> immediately and do internal management of tabs internally (remove tab from
> list immediately and wait it to close separately, for example). Yes, this
> might make code a bit more complex but it's the right thing to do for our
> users.

I am less concerned with the problem than you, but you can file a new bug to do some event queuing or something.
Comment 10 User image Sriram Ramasubramanian [:sriram] 2011-11-21 12:35:53 PST
Created attachment 575941 [details] [diff] [review]
Patch

This has the required changes.
Comment 11 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 15:01:14 PST
https://hg.mozilla.org/projects/birch/rev/dac80a8d25d4
Comment 12 User image Cristian Nicolae (:xti) 2011-11-22 07:20:11 PST
Verified fixed:
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111122
Firefox/11.0a1 Fennec/11.0a1
Devices: Samsung Galaxy Nexus S
OS: Android 2.3.4

Note You need to log in before you can comment on or make changes to this bug.