Closed Bug 1498854 Opened 4 years ago Closed 4 years ago
Session history popup breaks after one usage
STR: 1) Open a tab and navigate through a few links to build up some session history. 2) Long press the device back button. 3) The session history popup shows up. 4) Select some other history entry. 5a) Long press the device back button again. --> Nothing happens. 5b) Long press the forward/back button from the toolbar/overflow menu. --> What looks like multiple copies of the session history popup appear stacked on top of each other (see screenshot). Once in that state, short pressing the device back button behaves somewhat strangely, too and it takes multiple presses until hitting back actually navigates. Just clicking a link while in that state doesn't unbreak the popup, but after pressing back until the tab finally navigates unbreaks the popup until the next time you use it.
Also while the crash that bug 1476710 attempted to fix is real, that fix looks somewhat dubious - as per the documentation, getChildFragmentManager() is supposed to "Return a private FragmentManager for placing and managing Fragments inside of this Fragment." So calling the ChildFragmentManager instead avoids the crash because the ChildFragmentManager isn't currently executing a transaction, but on the other hand it doesn't do any useful work, either, because we're not actually dealing with nested fragments here and the ChildFragmentManager isn't at all involved with actually dismissing the TabHistoryFragment itself. By my guess, we could have had the same effect by simply commenting out the original line...
So the TabHistoryFragment attempts to remove itself when dismiss() is called  by calling popBackStackImmediate, which sometimes is safe and at other times isn't. dismiss() is being called from: - onItemClick(): When a history item has been selected - should be safe. - onClick(): When we've clicked outside of the history ListView - ditto. - onDestroy(): Might be unsafe because the system is already trying to destroy us, plus actually unnecessary now that we already want to dismiss() ourselves during onPause. - onPause(): Unsafe and partially unnecessary. When the system is calling onPause() because our parent activity is going into the background, we do want to dismiss ourselves, however we aren't allowed to use popBackStack*Immediate* while the FragmentManager is busy pausing us. Using the async popBackStack on the other hand means that by the time this actually runs we might already be past onSaveInstanceState(), which causes its own set of problems . Alternatively, the system is calling onPause() because the FragmentManager's back stack handling is trying to close us , in which case calling popBackStack again from inside the Fragment is wholly unnecessary. So I think the best choice would be to dismiss the fragment from *BrowserApp's* onPause handler *before* calling super.onPause(), so we can synchronously remove the fragment before the automatic lifecycle handling kicks in. Additionally, we shouldn't call popBackStack from inside the Fragment in response to onPause().  https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java#138-154  We could remove the fragment and commit allowing state loss, but this means that the TabHistoryFragment would then probably unexpectedly pop up again if BrowserApp was subsequently destroyed while in background and then restored during the next resume.  https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#946
Assignee: nobody → jh+bugzilla
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.
Tracking + fix-optional as we are in RC week, potential ride-along for a dot release if we have one.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/7413b2c7cdb4 Rework dismissing of TabHistoryFragment. r=jchen
Comment on attachment 9017041 [details] Bug 1498854 - Rework dismissing of TabHistoryFragment. r?jchen [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1476710 User impact if declined: After using the tab history popup to navigate to a different session history entry, long pressing the back button to access the popup again doesn't work and the back button temporarily behaves strangely. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: Create a tab with some session history and test that the various ways of dismissing the popup work: 1. By selecting a history entry. 2. By clicking outside of the popup. 3. By pressing the back button again. 4. By backgrounding Firefox. Afterwards, Firefox shouldn't crash and both back button and the popup should continue working normally if used again. List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): The cause of bug 1476710 is better understood now, so we revert to the previous behaviour to properly dismiss the popup again, but with a small change to avoid the crash in bug 1476710. String changes made/needed: none
Comment on attachment 9017041 [details] Bug 1498854 - Rework dismissing of TabHistoryFragment. r?jchen Hi Jan, I am not taking the patch for our last RC as the patch has only been fixed on nightly for a day but I am leaving the approval-mozilla-release set so as to keep it on our radar for a potential 63 dot release since it fixes this regression and seems a better fix to bug 1476710. Thanks
Verified on latest Nightly 64 (2018-10-19). No crashes occur following the STR. Devices: Sony Xperia Z5 Premium (Android 7.1.1) Google Pixel (Android 9)
Comment on attachment 9017041 [details] Bug 1498854 - Rework dismissing of TabHistoryFragment. r?jchen Regression fix, on 64 for 3 weeks with no regression. Approved for our next Fennec 63 dot release, thanks!
Attachment #9017041 - Flags: approval-mozilla-release? → approval-mozilla-release+
Verified as fixed on RC build 63.0.2 following the steps from comment 7. Devices: Google Pixel(Android 9), Sony Xperia Z2(Android 6.0.1).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.