Closed Bug 702619 Opened 13 years ago Closed 13 years ago

Swiftly closing two tabs at once results in an NPE at org.mozilla.gecko.TabsTray$TabsAdapter.<init>(TabsTray.java:138)

Categories

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

ARM
Android
defect

Tracking

(firefox11 fixed, fennec11+)

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: xti, Assigned: sriram)

Details

(Keywords: crash, reproducible, Whiteboard: [native-crash:P1])

Attachments

(1 file, 2 obsolete files)

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
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
Keywords: crash
Whiteboard: [native-crash]
Severity: major → critical
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
Summary: Fennec crashes if at least 2 tabs are closed → Swiftly closing two tabs at once results in an NPE at org.mozilla.gecko.TabsTray$TabsAdapter.<init>(TabsTray.java:138)
Assignee: nobody → mbrubeck
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.
Priority: -- → P2
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: mbrubeck → sriram
Attachment #575558 - Flags: review?(mark.finkle)
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.
Attachment #575576 - Flags: review?(mark.finkle)
Attachment #575558 - Flags: review?(mark.finkle) → review-
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 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
Attachment #575576 - Flags: review?(mark.finkle) → review+
(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.
(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.
Attached patch PatchSplinter Review
This has the required changes.
Attachment #575558 - Attachment is obsolete: true
Attachment #575576 - Attachment is obsolete: true
Attachment #575941 - Flags: review?(mark.finkle)
Attachment #575941 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/projects/birch/rev/dac80a8d25d4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [native-crash] → [native-crash:P1]
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
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.