1498854 - Session history popup breaks after one usage

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

Attachment: shistory_popup_broken.png

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.

Comment 1

[Jan Henning [:JanH]]

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...

Comment 2

[Jan Henning [:JanH]]

So the TabHistoryFragment attempts to remove itself when dismiss() is called [1] 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 [2].
Alternatively, the system is calling onPause() because the FragmentManager's back stack handling is trying to close us [3], 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().

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java#138-154
[2] 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.
[3] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#946

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1498854 - Rework dismissing of TabHistoryFragment. r?jchen

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.

Comment 4

[Pascal Chevrel:pascalc]

Tracking + fix-optional as we are in RC week, potential ride-along for a dot release if we have one.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7413b2c7cdb4
Rework dismissing of TabHistoryFragment. r=jchen

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7413b2c7cdb4

Comment 7

[Jan Henning [:JanH]]

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 8

[Pascal Chevrel:pascalc]

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

Comment 9

[Oana Horvath [:ohorvath]]

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 10

[Pascal Chevrel:pascalc]

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!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/defc169a1ee3

Comment 12

[Sorina Florean [:sflorean]]

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).