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)
Tracking
(firefox11 fixed, fennec11+)
VERIFIED
FIXED
People
(Reporter: xti, Assigned: sriram)
Details
(Keywords: crash, reproducible, Whiteboard: [native-crash:P1])
Attachments
(1 file, 2 obsolete files)
4.91 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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
Updated•13 years ago
|
Severity: major → critical
Updated•13 years ago
|
Keywords: reproducible
Comment 2•13 years ago
|
||
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)
Updated•13 years ago
|
Assignee: nobody → mbrubeck
Comment 3•13 years ago
|
||
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.
Updated•13 years ago
|
Priority: -- → P2
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #575558 -
Flags: review?(mark.finkle) → review-
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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+
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 10•13 years ago
|
||
This has the required changes.
Attachment #575558 -
Attachment is obsolete: true
Attachment #575576 -
Attachment is obsolete: true
Attachment #575941 -
Flags: review?(mark.finkle)
Updated•13 years ago
|
Attachment #575941 -
Flags: review?(mark.finkle) → review+
Comment 11•13 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/dac80a8d25d4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Whiteboard: [native-crash] → [native-crash:P1]
Reporter | ||
Comment 12•13 years ago
|
||
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
Updated•12 years ago
|
tracking-fennec: --- → 11+
Updated•12 years ago
|
status-firefox11:
--- → fixed
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•