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
--- 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;
}