1538291 - Add partial history aggregation and possibly limit history size in GV session storage code

Status: NEW

(GeckoView :: General, enhancement, P3)

Description

[Dylan Roeh (:droeh) (he/him)]

We want to re-add partial history aggregation in SessionStateAggregator.js and handle the logic for that in updateSessionState; additionally, to prevent the session history size from becoming an issue (in the case of eg history API abuse), we may want to limit the number of entries.

Comment 1

[Jan Henning [:JanH]]

(In reply to Dylan Roeh (:droeh) from comment #0)

we may want to limit the number of entries.

As long as we only restore the current state of the Desktop code, which only uses partial session history updates for a relatively limited number of cases [1], shouldn't that already work through browser.sessionhistory.max_entries, which limits the amount of session history the docshell keeps around in the first place? Therefore the next full history update will truncate our cached data, too.

[1] And using partial session history updates more often than that would require solving bug 1350567 first.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Ah, I think my initial comment was unclear -- we want to limit the number of history entries present in mStateCache in GeckoSession, which (unless I'm misunderstanding something) could currently grow without bounds.

Comment 3

[Jan Henning [:JanH]]

If I'm not mistaken, a full update (which is all we're currently doing, and even in the future it would happen frequently enough because as I said doing it less often, e.g. bug 1350569, requires some more prerequisite work), will replace all the existing contents of the state cache (compare what the TabStateCache does with a fromIdx of -1) with the current session history held by the docshell, which is capped by browser.sessionhistory.max_entries. So the size of the history in the Java state cache couldn't really grow beyond the size of the history held by Gecko itself.

You're right that more partial updates might require further work to properly handle it in the cache (and it might need to mirror the history truncation that occurs in Gecko when you hit browser.sessionhistory.max_entries, too), but at the moment I don't see any problem there.

Comment 4

[Chris Peterson [:cpeterson]]

67=wontfix. Fenix MVP will use GeckoView 68, so we don't need to uplift this fix to 67 Beta.

Comment 5

[Chris Peterson [:cpeterson]]

Moving from Fenix M4 to M7 because Dylan says this bug doesn't need to block Fenix MVP.

Comment 6

[Chris Peterson [:cpeterson]]

Deferring this bug from Fenix's M7 (July) milestone to the M8 backlog for later in Q3.

Comment 7

[Chris Peterson [:cpeterson]]

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Comment 8

[Emily Toop (:fluffyemily)]

We are moving to SessionStoreListener rather than SessionStateAggregator. We should check that whatever we want from here is present in the SessionStoreListener.

Comment 9

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

The bug assignee didn't login in Bugzilla in the last 7 months.
:amoya, could you have a look please?
For more information, please visit auto_nag documentation.