Bug 1120441 - Don't try to show tab-history panel if app has been shutdown r?sebastian draft
authorAndrzej Hunt <ahunt@mozilla.com>
Wed, 18 Jan 2017 22:09:20 +0100
changeset 467602 01426bd204db5656b9546677b906e0fa9d81107b
parent 467601 045d8fe30f546ab08466c9586ce298e6459c2069
child 543731 7ac8002e66753a58c7d4346a24621a885444694a
push id43226
push userahunt@mozilla.com
push dateSat, 28 Jan 2017 05:04:06 +0000
reviewerssebastian
bugs1120441
milestone54.0a1
Bug 1120441 - Don't try to show tab-history panel if app has been shutdown r?sebastian Showing the tab history panel involves a gecko call to retrieve tab history. This can be slow, meaning we have no idea what state the app will be in when the tab history data is returned. Thus we need to protect the code that shows the tab history fragment against a number of scenarios to avoid crashes in those cases where the app might be shutting down: 1. If onSaveInstanceState() has been called (which might happen before or after onPause(), and might be linked to app shutdown - but the docs don't appear to give any guarantees), fragment transactions cannot be performed. We protect against this by accepting loss of state in fragment transactions. 2. If the Activity has been completely destroyed, trying to perform a fragment transaction will likewise fail. We protect against this by not even trying to perform the transaction if we definitively know that the Activity is being shut down (ie isFinishing()). In both of these cases, we simply must accept that we're potentially losing UI state: i.e. a user could request the tab history panel via long-back-press, followed by exiting the app; we now end up never ever showing the panel. This scenario doesn't seem like a major loss - and fixing this issue properly would require significant investment (i.e. we would need to either cache tab history on frontend side, or cache the tab-history panel request - and it's not clear users will still care about seeing the panel the next time they open firefox). MozReview-Commit-ID: JsAK1By8yqn
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
@@ -651,19 +651,30 @@ public class BrowserApp extends GeckoApp
 
         // Initialize Tab History Controller.
         tabHistoryController = new TabHistoryController(new OnShowTabHistory() {
             @Override
             public void onShowHistory(final List<TabHistoryPage> historyPageList, final int toIndex) {
                 runOnUiThread(new Runnable() {
                     @Override
                     public void run() {
+                        if (BrowserApp.this.isFinishing()) {
+                            // TabHistoryController is rather slow - and involves calling into Gecko
+                            // to retrieve tab history. That means there can be a significant
+                            // delay between the back-button long-press, and onShowHistory()
+                            // being called. Hence we need to guard against the Activity being
+                            // shut down (in which case trying to perform UI changes, such as showing
+                            // fragments below, will crash).
+                            return;
+                        }
+
                         final TabHistoryFragment fragment = TabHistoryFragment.newInstance(historyPageList, toIndex);
                         final FragmentManager fragmentManager = getSupportFragmentManager();
                         GeckoAppShell.vibrateOnHapticFeedbackEnabled(getResources().getIntArray(R.array.long_press_vibrate_msec));
+                        if (BrowserApp.this.isForegrounded())
                         fragment.show(R.id.tab_history_panel, fragmentManager.beginTransaction(), TAB_HISTORY_FRAGMENT_TAG);
                     }
                 });
             }
         });
         mBrowserToolbar.setTabHistoryController(tabHistoryController);
 
         final String action = intent.getAction();
@@ -1064,16 +1075,17 @@ public class BrowserApp extends GeckoApp
         if (urls != null) {
             openUrls(urls);
         }
     }
 
     @Override
     public void onResume() {
         super.onResume();
+
         if (mIsAbortingAppLaunch) {
             return;
         }
 
         if (!mHasResumed) {
             getAppEventDispatcher().unregisterUiThreadListener(this, "Prompt:ShowTop");
             mHasResumed = true;
         }
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
@@ -119,17 +119,20 @@ public class TabHistoryFragment extends 
     }
 
     // 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.addToBackStack(tag);
-        backStackId = transaction.commit();
+        // 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 (dismissed) {
             return;
         }