1892751 - Make sure GeckoSession lifecycle's final state update is working in SHIP-enabled Fenix

Status: RESOLVED WONTFIX

(GeckoView :: General, task, P2)

Comment 1

[Kaya [:kaya]]

Attachment: WIP: Bug 1892751 - Use runtime dispatcher to send state updates in non-SHIP code

Depends on D193929

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:owlish, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Mira Lobontiu (Android QA)]

Attachment: az_recorder_20240802_175356.mp4

STR:

1. Open a website.
2. In the same tab, open a different page, or tap on a link inside the website from step 1.
3. Repeat step 2 several times, to create tab's history.
4. Close the tab.
5. Go to Recently closed tabs.
6. Tap on the closed tab from step 4.
7. Long-tap on the back arrow in order to trigger the tab's history, and observe.

Comment 4

[Mira Lobontiu (Android QA)]

Attachment: logcat tab history.txt

Comment 5

[Chris Peterson [:cpeterson]]

Kaya says Jeff is working on this bug.

Comment 6

[Jeff Boek [:boek]]

Attachment: Bug 1892751 - Monitor engineSession after closing tab for last state update.

Comment 7

[Chris Peterson [:cpeterson]]

Kaya says this work doesn't need to block shipping SHIP.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd1be171a0ca
Monitor engineSession after closing tab for last state update. r=android-reviewers,kaya

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/bd1be171a0ca

Comment 10

[Jonathan Almeida [:jonalmeida]]

With the backout of the current patch that happened in bug 1958738 and the regressions that are linked, I think we need another approach.

We cannot wait for one more state update to come through in it's own natural time after the user has expected the tab to be closed. From what we've seen in bug 1958738 is that a state update can take a long time to show up and by then, we will not have closed the tab to free up expected resources.

What we may need is an API that explicitly let's us get the last session update. In GeckoView (or AC), we can probably try to wrap our close API so that the close happens when we get the state.

Is that kind of API possible, Kaya?

Comment 11

[Kaya [:kaya]]

I've already implemented a flushing mechanism for SHIP: https://searchfox.org/mozilla-central/rev/584b344830aa2558985675b99aaadbb915402caa/mobile/shared/modules/geckoview/GeckoViewTab.sys.mjs#219-225.

This gets triggered when a session's activeness changes - navigation between tabs: https://searchfox.org/mozilla-central/rev/584b344830aa2558985675b99aaadbb915402caa/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#2678

When I first tried implementing final state update, I've tried wiring this flush to the session's closing. But there's a catch there.

1- The flush mechanism is async. That's why, even if it is getting triggered immediately, there's still some waiting for receiving the final state (this is almost immediate, still too long for what I will note in 2nd point).
2- When session gets closed, the native objects are cleared, which means, we're losing the event dispatcher that'd be used for forwarding the final state update from native to the GV java side.

So, even if we'd try to flush to receive the final state without the buffer time of 10s, it would still be too late to wire this up inside the session's close API. That's why we need to find a way to keep the window event dispatcher on the native side, and detach it after the final state update is completed. And that was not easy, even though I've tried to play around with it to keep it around a bit longer.

One other alternative that I tried was to use the runtime event dispatcher, which is attached to the GeckoRuntime lifecycle - not gecko session's. There, I was receiving the session state updates to GeckoRuntime and forwarding them to corresponding session's for updating their caches. This would mean a breach in the stateless approach we try to achieve, that's why this was not accepted back then. You can see the playground I had here: https://phabricator.services.mozilla.com/D208502 (the patch is quite old, the code might have changed already).

There's also a problem on the frontend side as well. Even if we somehow fix receiving the final state update, we still need to adjust how we register and unregister the observers on A-C for notification purposes. One problem I had (after fixing the final state update receiving), the observers in GeckoEnginSession were already unregistered when the tab was getting removed in AC (via onTabRemoved callback). So, even if I would receive the final update, I would not be able to notify the observers in GeckoEngineSession, and the session state would not be updated on the embedder's side. That's why, the embedder's observers lifecycle also needs to be rearranged for successfully completing the final state update process.

Overall, I agree that we should have a new API to flush the session state for such operations like session closing, OS killing the process etc. But according to my past experiences, this has complications that we need to design and address. Happy to pair and do that together.

Comment 12

[Jonathan Almeida [:jonalmeida]]

(In reply to Kaya [:kaya] from comment #11)

Overall, I agree that we should have a new API to flush the session state for such operations like session closing, OS killing the process etc. But according to my past experiences, this has complications that we need to design and address. Happy to pair and do that together.

I'd be happy to pair! The aim is to minimize data loss, so flushing often when we can will be a good way to reduce that. We use similar patterns today with AutoSave.

Comment 13

[[:owlish] 🦉 PST]

Cloning this into a new bug 2010432 to continue work