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]
:
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 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 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 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 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 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 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 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 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 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 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 Sriram Ramasubramanian [:sriram] 2011-11-21 12:35:53 PST
Created attachment 575941 [details] [diff] [review]
Patch

This has the required changes.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 15:01:14 PST
https://hg.mozilla.org/projects/birch/rev/dac80a8d25d4
Comment 12 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.