Bug 1498854 - Rework dismissing of TabHistoryFragment. r=jchen, a=pascalc
authorJan Henning <jh+bugzilla@buttercookie.de>
Tue, 16 Oct 2018 16:17:18 +0000
changeset 492965 defc169a1ee3
parent 492964 f91ac8ecb4dc
child 492966 032ed494717a
push id1843
push userryanvm@gmail.com
push dateMon, 05 Nov 2018 14:45:45 +0000
treeherdermozilla-release@b71bed0c6a40 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjchen, pascalc
bugs1498854, 1476710, 1093209
milestone63.0.2
Bug 1498854 - Rework dismissing of TabHistoryFragment. r=jchen, a=pascalc 1. The patch from bug 1476710 was nonsense and had the same effect as simply deleting that line, because the ChildFragmentManager is only of interest if the TabHistoryFragment loaded further Fragments itself. 2. The issue at hand is that under some circumstances the TabHistoryFragment will be trying to dismiss itself while its responsible FragmentManager is already busy transacting some Fragment state changes. More precisely, the fact that the Fragment is calling popBackStack*Immediately*, which isn't allowed if the FragmentManager is already handling some other transaction. 3. The dismiss() calls in response to the onClick() handlers are unproblematic, because they won't trigger any FragmentManager activity through any route other than the dismiss() call itself. 4. The dismiss() calls in onPause() *are* problematic because the Fragment- Manager will already be busy pausing the TabHistoryFragment, so triggering a further synchronous transaction is not allowed (and originally caused bug 1476710). 5. If the onPause() call happened because some external entity was attempting to remove the fragment (either BrowserApp directly, or indirectly by forwarding a back button press to the FragmentManager), then the Fragment trying to additionally remove itself is unnecessary. 6. If the onPause() call happens because the containing activity itself is being paused, then the Fragment being dismissed is the desired outcome (see bug 1093209), however the Fragment won't be able to remove itself because a) A synchronous transaction is illegal at that point in time. b) An async transaction would be possible, but might not complete until after onSaveInstanceState() had subsequently already been called, which again would be illegal because of state loss. c) An async transaction allowing state loss would succeed in any case, but would mean that if BrowserApp was subsequently destroyed while in back- ground and then later recreated from the savedInstanceState, the Tab- HistoryFragment would be recreated as well, which is undesired. 7. Therefore, the only way to dismiss the TabHistoryFragment when the containing activity is pausing is to synchronously dismiss the Fragment from inside the activity, *before* the onPause() call is forwarded to the FragmentManager. 8. Calling dismiss() in response to onDestroy() is unnecessary, because the Fragment is already irrevocably being removed as soon as we hit onPause(). 9. Because it doesn't make sense to have multiple TabHistoryFragments active at the same time, we also change the logic such that any attempt to show a new TabHistoryFragment will now replace the previous fragment. This is also useful in view of the fact that in order to close the Fragment, BrowserApp retrieves it by calling findFragmentByTag(), which simply returns the first matching Fragment, i.e. wouldn't properly handle things if we ever accidentally ended up with multiple Fragment instances active at the same time. Differential Revision: https://phabricator.services.mozilla.com/D8680
mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -1033,16 +1033,18 @@ public class BrowserApp extends GeckoApp
 
         for (BrowserAppDelegate delegate : delegates) {
             delegate.onResume(this);
         }
     }
 
     @Override
     public void onPause() {
+        dismissTabHistoryFragment();
+
         super.onPause();
         if (mIsAbortingAppLaunch) {
             return;
         }
 
         if (mHasResumed) {
             // Register for Prompt:ShowTop so we can foreground this activity even if it's hidden.
             getAppEventDispatcher().registerUiThreadListener(this, "Prompt:ShowTop");
@@ -3113,20 +3115,17 @@ public class BrowserApp extends GeckoApp
     }
 
     @Override
     public boolean onPrepareOptionsMenu(Menu aMenu) {
         if (aMenu == null)
             return false;
 
         // Hide the tab history panel when hardware menu button is pressed.
-        TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
-        if (frag != null) {
-            frag.dismiss();
-        }
+        dismissTabHistoryFragment();
 
         if (!GeckoThread.isRunning()) {
             aMenu.findItem(R.id.settings).setEnabled(false);
             aMenu.findItem(R.id.help).setEnabled(false);
         }
 
         Tab tab = Tabs.getInstance().getSelectedTab();
         // Unlike other menu items, the bookmark star is not tinted. See {@link ThemedImageButton#setTintedDrawable}.
@@ -4137,9 +4136,16 @@ public class BrowserApp extends GeckoApp
     }
 
     @Override
     public void onFinishedOnboarding(final boolean showBrowserHint) {
         if (showBrowserHint && !Tabs.hasHomepage(this)) {
             enterEditingMode();
         }
     }
+
+    private void dismissTabHistoryFragment() {
+        TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
+        if (frag != null) {
+            frag.dismiss();
+        }
+    }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
@@ -100,59 +100,61 @@ public class TabHistoryFragment extends 
         // Since the fragment view fills the entire screen, any clicks outside of the history
         // ListView will end up here.
         dismiss();
     }
 
     @Override
     public void onPause() {
         super.onPause();
-        dismiss();
+        onDismiss();
     }
 
     @Override
     public void onDestroy() {
         super.onDestroy();
-        dismiss();
 
         GeckoApplication.watchReference(getActivity(), this);
     }
 
     @Override
     public void onSaveInstanceState(Bundle outState) {
         if (backStackId >= 0) {
             outState.putInt(BACK_STACK_ID, backStackId);
         }
     }
 
     // Function to add this fragment to activity state with containerViewId as parent.
     // This similar in functionality to DialogFragment.show() except that containerId is provided here.
     public void show(final int containerViewId, final FragmentTransaction transaction, final String tag) {
         dismissed = false;
-        transaction.add(containerViewId, this, tag);
+        transaction.replace(containerViewId, this, tag);
         transaction.addToBackStack(tag);
         // Populating the tab history requires a gecko call (which can be slow) - therefore the app
         // state by the time we try to show this fragment is unknown, and we could be in the
         // middle of shutting down:
         backStackId = transaction.commitAllowingStateLoss();
     }
 
     // Pop the fragment from backstack if it exists.
     public void dismiss() {
+        if (backStackId >= 0) {
+            getFragmentManager().popBackStackImmediate(backStackId, FragmentManager.POP_BACK_STACK_INCLUSIVE);
+            backStackId = -1;
+        }
+        onDismiss();
+    }
+
+    private void onDismiss() {
         if (dismissed) {
             return;
         }
 
         dismissed = true;
 
-        if (backStackId >= 0) {
-            getChildFragmentManager().popBackStackImmediate(backStackId, FragmentManager.POP_BACK_STACK_INCLUSIVE);
-            backStackId = -1;
-        }
-
         if (parent != null) {
             parent.setVisibility(View.GONE);
         }
     }
 
     private static class TabHistoryAdapter extends ArrayAdapter<TabHistoryPage> {
         private final List<TabHistoryPage> pages;
         private final Context context;