Steps to reproduce: 1. Open Firefox (tablet UI) with a single non-private tab. 2. Add a private tab. 3. Close the private tab. 4. Open the tab sidebar. Expected: The original non-private tab is selected and is visible in the tab sidebar. Actual: The original non-private tab is selected, but the tab sidebar is still showing the "Private" section, which is now empty (see screenshot). I think the tab sidebar should automatically switch views if necessary to show any newly-selected tab. This is reproducible in Nightly 21 and Aurora 20 on Motorola Xoom (Android 4.1).

This patch enforces a simple rule: If the selected tab changes while the TabsPanel is visible, switch to the view that contains the newly-selected tab. This also fixes bug 834416 and some other related bugs.

This patch looks like it will introduce the same sort of leak that I'm trying to fix in "part 1" of bug 834414 :( If there's no good place to unregister the listener, maybe I should just make Tabs keep the listeners as weak refs instead.

Comment on attachment 706230 [details] [diff] [review] patch Review of attachment 706230 [details] [diff] [review]: ----------------------------------------------------------------- I agree with Kats on this. Also, I don't know the UX take on this. Say there are 4 normal and 4 private tabs. If I close a private tab, will the next tab chosen be a private or a normal one? If it's normal, we are losing the context of where we were. If it's private, I would like to have this as a bug for sure. On the other side, I don't like TabsPanel listening for tab related events. It's just a container and shouldn't know anything about tabs. It's so generic -- like a LinearLayout or FrameLayout. Instead, the GeckoApp/BrowserApp should take the charge. They know to show a TabsPanel.Panel. Also, onTabsChanged is called on the UI thread. (No GeckoApp.mAppContext please :( The view has access to the UI thread, if needed :( ). GeckoApp already listens to Tab changes. https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/GeckoApp.java#l211 This could make a call to show the panel. My approach here would be, if (mTabsPanel.isShown() && mTabsPanel.currentPanel() is-not-same-as selectedTab's Panel) show(selectedTabs's panel).

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2) > This patch looks like it will introduce the same sort of leak that I'm > trying to fix in "part 1" of bug 834414 :( Good catch, thanks. > If there's no good place to unregister the listener, maybe I should just > make Tabs keep the listeners as weak refs instead. That sounds like a good idea. (We could register/unregister in the show/hide methods like TabsTray does, but I think it could still leak if the activity is destroyed while the TabsPanel is visible.) (In reply to Sriram Ramasubramanian [:sriram] from comment #3) > I agree with Kats on this. Also, I don't know the UX take on this. > Say there are 4 normal and 4 private tabs. If I close a private tab, will > the next tab chosen be a private or a normal one? That depends on the tab's "parent" and its ordering with the internal array in Tabs. The main case where this shows up for me is when closing the last private tab. Should I limit the patch to only switch panels if there are no tabs left in the current panel? (I could also write a separate patch so that after closing a tab, a tab from the same panel will get selected next whenever possible.) > If it's normal, we are losing the context of where we were. If it's private, > I would like to have this as a bug for sure. Note that in the phone UI, we already switch automatically to the tab tray of the newly selected tab, so this patch makes the tablet UI more consistent. > On the other side, I don't like TabsPanel listening for tab related events. > It's just a container and shouldn't know anything about tabs. > It's so generic -- like a LinearLayout or FrameLayout. Instead, the > GeckoApp/BrowserApp should take the charge. They know to show a > TabsPanel.Panel. Okay, sounds good. > Also, onTabsChanged is called on the UI thread. (No GeckoApp.mAppContext > please :( The view has access to the UI thread, if needed :( ). Got it.

Tracking as part of the private per tab feature for 20.

Comment on attachment 706744 [details] [diff] [review] patch v2 Review of attachment 706744 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r+ after removing the Runnable. ::: mobile/android/base/BrowserApp.java @@ +110,5 @@ > + public void run() { > + showTabs(panel); > + } > + }); > + } OnTabsChangedListener runs on UI thread. Hence we don't need the post(). You could remove "final" and the Runnable.

(In reply to Sriram Ramasubramanian [:sriram] from comment #7) > OnTabsChangedListener runs on UI thread. Hence we don't need the post(). > You could remove "final" and the Runnable. I get a concurrent modification exception when I run this code without the Runnable. I admit I don't know why; do you have any idea?

That could be a case where we are showing tabs already and a selected even calls showTabs() again. ConcurrentModification exception will be raised if two different threads are touching on same set of lists (usually). That makes me doubt if OnTabsChanged in running in UI thread.

(In reply to Sriram Ramasubramanian [:sriram] from comment #9) > ConcurrentModification exception will be raised if > two different threads are touching on same set of lists (usually). That > makes me doubt if OnTabsChanged in running in UI thread. Actually I find that gets thrown more often in practice when you modify the list while also iterating over it with an iterator. e.g. for (Iterator i = list.iterator(); i.hasNext()) { Object o = i.next(); if (something) list.remove(o); } This will *not* happen if you do the remove in a post back to the same thread since it will get queued up and execute after the iteration loop is complete. (I don't know if that is what's happening here, though. A stack from the ConcurrentModificationException would probably be useful)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > (In reply to Sriram Ramasubramanian [:sriram] from comment #9) > > ConcurrentModification exception will be raised if > > two different threads are touching on same set of lists (usually). That > > makes me doubt if OnTabsChanged in running in UI thread. > > Actually I find that gets thrown more often in practice when you modify the > list while also iterating over it with an iterator. e.g. > > for (Iterator i = list.iterator(); i.hasNext()) { > Object o = i.next(); > if (something) list.remove(o); > } I agree to this. But, we don't do this anywhere. Here is a possible cause: * Closing a tab will select the next tab first. * That SELECTED message is now in another panel, and we try to show that. * This in turn remove all the tabs in current panel (in this case Private tabs). * Now a CLOSED indication is coming from Gecko for the closed tab, causing the same list to be touched. However, this cannot happen every single time. This case seems like a rare one. But if it's happening every single time, we are doing something wrong somewhere else. Also, OnTabsChanged() is called on UI thread. So, SELECTED's hidePanel() and CLOSED's remove() will be queued up not causing to modify the list from two different threads.

Here's the stack trace from a build without the post(). This happens when I close the last tab in the private tab list, which then causes the panel to switch to the "normal" list. E/GeckoAppShell( 854): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main") E/GeckoAppShell( 854): java.util.ConcurrentModificationException E/GeckoAppShell( 854): at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:569) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs$7.run(Tabs.java:424) E/GeckoAppShell( 854): at android.app.Activity.runOnUiThread(Activity.java:4591) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:415) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:411) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs$3.run(Tabs.java:158) E/GeckoAppShell( 854): at android.app.Activity.runOnUiThread(Activity.java:4591) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs.selectTab(Tabs.java:155) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs.closeTab(Tabs.java:224) E/GeckoAppShell( 854): at org.mozilla.gecko.Tabs.closeTab(Tabs.java:213) E/GeckoAppShell( 854): at org.mozilla.gecko.TabsTray$3.onPropertyAnimationEnd(TabsTray.java:328) E/GeckoAppShell( 854): at org.mozilla.gecko.PropertyAnimator.stop(PropertyAnimator.java:157) E/GeckoAppShell( 854): at org.mozilla.gecko.PropertyAnimator.run(PropertyAnimator.java:95) E/GeckoAppShell( 854): at org.mozilla.gecko.PropertyAnimator$FramePosterPostJB$1.doFrame(PropertyAnimator.java:253) E/GeckoAppShell( 854): at android.view.Choreographer$CallbackRecord.run(Choreographer.java:723) E/GeckoAppShell( 854): at android.view.Choreographer.doCallbacks(Choreographer.java:555) E/GeckoAppShell( 854): at android.view.Choreographer.doFrame(Choreographer.java:524) E/GeckoAppShell( 854): at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:711) E/GeckoAppShell( 854): at android.os.Handler.handleCallback(Handler.java:615) E/GeckoAppShell( 854): at android.os.Handler.dispatchMessage(Handler.java:92) E/GeckoAppShell( 854): at android.os.Looper.loop(Looper.java:137) E/GeckoAppShell( 854): at android.app.ActivityThread.main(ActivityThread.java:4745) E/GeckoAppShell( 854): at java.lang.reflect.Method.invokeNative(Native Method) E/GeckoAppShell( 854): at java.lang.reflect.Method.invoke(Method.java:511) E/GeckoAppShell( 854): at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786) E/GeckoAppShell( 854): at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553) E/GeckoAppShell( 854): at dalvik.system.NativeStart.main(Native Method)

Hmm. The problem is that closing "private tabs panel", unregisters itself from the list of "mTabsChangedListeners". And since this list changes while the "while loop" iteration happens, a ConcurrentModification exception is thrown. I would recommend: 1. not adding the "post()" call in the patch. 2. Add/remove/notify listeners inside a synchronized block on the mTabsChangedListeners.

That won't help, it will still all be running on the same thread and so synchronization will change nothing. If you don't want to do the post() I think the only reasonable other alternative is to stop using an Iterator in notifyListeners() and manually iterate through the list instead (or use a CopyOnWriteArrayList like we do in util/EventDispatcher.java).

Comment on attachment 706744 [details] [diff] [review] patch v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): The evergreen everchanging tabs UI. User impact if declined: If all private tab closes, the private tab will shown, even though its empty. Testing completed (on m-c, etc.): Landed in m-i on 01/29 Risk to taking this patch (and alternatives if risky): Very low. This just switches the panels. String or UUID changes made by this patch: None.

Verified fixed on: -build: Firefox for Android 20.0a2 (2013-02-15), Firefox for Android 21.0a1 (2013-02-15) -device: Asus EEE Transformer -OS: Android 4.0.3