1341810 - Implement partial session history updates for Fennec

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Session Restore, defect, P3)

Description

[Jan Henning [:JanH]]

Once we've switched to use SessionHistory.jsm, we can easily collect session history entries only as needed instead of always serialising the full complement.

For this, we probably have to replace the history listener notifications from browser.js with something else that can pass along the index we're receiving from the history change listener where necessary.

Then, we can implement something similar as desktop (compare https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/browser/components/sessionstore/content/content-sessionStore.js#324 and https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/browser/components/sessionstore/TabStateCache.jsm#123), except that we're not e10s and can omit the message passing bit inbetween for the time being.

In fact, we should go one step further and see if we can avoid doing full collections for DOMTitleChanged as well and see whether just refreshing the current history entry works as well.

We should also figure out which bits of history need updating once we start capturing the presState (bug 1265818), because that is only added to/updated in the SHEntry when we're navigate away to some other entry. If I'm understanding the desktop code right, their logic for OnHistoryNewEntry e.g. only captures the new history entry, whereas we also ought to refresh the current entry, because the PresState is only added to/updated in an SHEntry when we're navigating away from it.

Comment 1

[Jan Henning [:JanH]]

Hacked something preliminary together and it seems to work, although I haven't looked too closely for any unhandled edge cases, subtle failures, etc. yet.

It probably won't help to eliminate the test_parse_rule workaround from bug 1337940 (https://hg.mozilla.org/mozilla-central/rev/3e542f18ed62) though, because it doesn't look like I'll be able to avoid doing a full history collect for OnHistoryPurge: As that notification is cancellable, there's no guarantee that the history is actually going to be purged, so I can't just go ahead and speculatively delete cached session store history entries myself in order to avoid a full session history collect call (ha ha) later on.
OnLenghtChanged might help here, although not in its current state (ideally I think it should notify not only about the new length, but about the delta as well) and even then I'm not sure if doing things that way would be absolutely foolproof under all circumstances. Outside of that test I suppose that running into the "browser.sessionhistory.max_entries" limit might not be *that* common, though, and until a tab reaches that point my planned implementation will still help.

I've also noticed that for OnHistoryNewEntry doing a simple zero-time timeout (to give the SHistory code a chance to run to completion) is enough to catch the new history entry and *index* afterwards, but for OnHistoryGoBack/Forward/GotoIndex that's not the case - the history index update is only triggered by the Docshell at some point during page loading, so nothing really deterministic in relation to those history listener notificatiosn. Thankfully in this case listening to OnIndexChanged instead works fine to catch those kinds of navigation.

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1341810 - Part 1 - onTabLoad() should remove any still pending history collects.

Review commit: https://reviewboard.mozilla.org/r/132476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132476/

Comment 3

[Jan Henning [:JanH]]

Attachment: Preparatory steps

- call ss directly
- compute aFromIdx we want to use for collect(), but only log it, don't use it yet

Review commit: https://reviewboard.mozilla.org/r/132478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132478/

Comment 4

[Jan Henning [:JanH]]

Attachment: implement partial session store updates for fennec

Review commit: https://reviewboard.mozilla.org/r/132480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132480/

Comment 5

[Jan Henning [:JanH]]

Since I'm still busy with other things, the current state of my WIP patch, just so it doesn't get lost.

Comment 6

[Jan Henning [:JanH]]

I hereby formally dedicate the new Session Store component, etc., etc. :-)

Comment 7

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.