We currently store and reload data in sessionstore.js that hasn't been used in several months, in particular in closedTabs and closedWindows. We should clean it up.

One possible strategy would be to add a date when we place something in closedTabs or closedWindows for the first time, then upon load, remove stuff that hasn't been used for e.g. one week.

I see how to do that. I'll take it. Nicholas, why did you remove the MemShrink label?

I removed the tag because there's no data on how it will affect memory consumption, and we already have about eight other bugs relating to session store tagged with MemShrink. The MemShrink tag probably has less meaning than you think. All it means is that (a) it'll get looked at once in a MemShrink meeting. And if it gets marked as a MemShrink:P1, then we'll periodically revisit it in a MemShrink meeting. If it gets marked as a MemShrink:P2 or P3, then it's unlikely to be looked at again, and the tag won't make it any more likely to be fixed.

Here's a draft that performs some cleanup during idle-daily. Untested, of course.

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #1) > One possible strategy would be to add a date when we place something in > closedTabs or closedWindows for the first time, then upon load, remove stuff > that hasn't been used for e.g. one week. This sounds like we want bug 873556 as a prerequisite.

Comment on attachment 8340109 [details] [diff] [review] Draft v2 Review of attachment 8340109 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems mostly fine to me but we will probably have a hard time coming up with good values for the TTL constants. Your RFC on the mailing lists made it pretty clear that there are people that value that sessionstore tends to forget nothing. Not sure what's the best way to proceed here at the moment. ::: browser/components/sessionstore/src/SessionStore.jsm @@ +3730,5 @@ > * where we don't have any non-popup windows on Windows and Linux. Then we must > * resize such that we have at least one non-popup window. > */ > _capClosedWindows : function ssi_capClosedWindows() { > + this._removeOldStuff(); Where is it? :)

(In reply to Tim Taubert [:ttaubert] from comment #7) > The patch seems mostly fine to me but we will probably have a hard time > coming up with good values for the TTL constants. Your RFC on the mailing > lists made it pretty clear that there are people that value that > sessionstore tends to forget nothing. Sure, this is sensitive. At first I felt uneasy with that idea. We could do this in session store : 1) Keep the last closed window (1). 2) Apply the filter "1 week of age" to the remaining last closed windows (2 to n). 3) For each window : 3.1) Keep the last closed tab (1). 3.2) Apply the filter "1 week of age" to the remaining last closed tabs (2 to m). This would already clean a lot, without being too dangerous.

I followed the m.d.platform thread, am in the camp that requires sessionstore to "not lose 'active' data", and am generally wary of this proposal. But, I am in favor of comment 8 especially if it helps mitigate against dataloss bug 581510 that I opened 3 years ago. Is this likely to help? But I suggest posting comment 8 (or whatever the proposal ends up being) in m.d.platform for further feedback before implementation. It might also help to explain what is meant by "a very long time." from your original post. I don't recall seeing a definition. n.b. afaict bug 581510 is not about the size of sessionstore.js file per se, but rather is about some artificial limitation or bug of memory data structure. But I assume that's not the case for dataloss bug 903621. (would be cool if someone could take another look at that - I'm happy to answer any questions)

As long as this covers only *closed* tabs and windows (older than 1 week), I think this is pretty uncontroversial. And that's all I can see in the patch.

So, I'm planning to resume work on this bug soonish. For clarity, my objective is to remove from sessionstore.js all the tabs and all the windows that have been *closed* at least 2 weeks ago (delay will be defined by a preference). This should contribute to improve memory usage, sessionstore-related jank, startup time and battery life. I do not see a good reason to keep a single old and unused closed window forever or to keep a single old and unused closed tab forever. We might consider cleaning up old (again, 2 weeks by default) history entries in some followup bug.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #11) > For clarity, my objective is to remove from sessionstore.js all the tabs and > all the windows that have been *closed* at least 2 weeks ago (delay will be > defined by a preference). This should contribute to improve memory usage, > sessionstore-related jank, startup time and battery life. I do not see a > good reason to keep a single old and unused closed window forever or to keep > a single old and unused closed tab forever. This would be welcome. Given that the tabs have been closed, I don't see a data loss issue here. > We might consider cleaning up old (again, 2 weeks by default) history > entries in some followup bug. Here I may disagree. Do you mean the tab's Back and Forward pages ? If so, these have to be kept. Otherwise, in a lot of session restores I would lose *all* the tab navigation histories. Safari restores these neatly. Furthermore, in several bugs of Firefox' session restore the tab appears to be lost and the only way to have it back is by using its navigation history. If there are really good reasons to trash some Back and Forward pages, it could be OK to do so *after some number*, but not with just some age criterion. I mean : keeping the last 10 pages and trashing the 11th and further ones could be OK. But with just some age criterion it would often mean trashing all the history of the tab, and this would be a catastrophe. Thank you.

Let's discuss the history part in the relevant bug, if we ever decide to open it.

Turned this into a meta bug. The bug for closed tabs and windows is now bug 989393.

What about history entries of frames within a tab? Some websites (arstechnica.com being my main offender) reload the ad iframe which then gets put into sessionstore. They don't affect the main history (the back button) so why are they stored? With 3 or 4 ads per page I quickly found a tab with 50+ children.

Apparently, on arstechnica.com, all iframes are dynamic, so their history should not be stored at all since bug 936271. If it is stored, this sounds like a bug.

I haven't actually loaded some of those tabs in a year or more, they've been preserved in sessionrestore amber. Would bug 936271 only help with sites visited since it landed?

Indeed.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11) > For clarity, my objective is to remove from sessionstore.js all the tabs and > all the windows that have been *closed* at least 2 weeks ago (delay will be > defined by a preference). Now, this change has been implemented in Firefox. Can you please clarify what this change does? 1) At session restore, tabs and windows that were closed at least 14 days ago do not appear anymore in "Last closed tabs" and "Last closed windows". OR 2) At session restore, and during nominal use of Firefox, tabs and windows that were closed at least 14 days ago do not appear anymore in "Last closed tabs" and "Last closed windows". ? Thank you.

Option 1.

Thank you David. So there seems to be a bug. It seems to be a regression due to this change. I have seen "last closed tabs" and "last closed windows" disappearing over time, while Firefox remained open. This is expected not to happen. I have seen that happening on two Macs. You may see the discussion here: https://support.mozilla.org/en-US/questions/1053356

Nicolas: That is weird, but this codepath doesn't affect the menus, nor does it know anything about menus. Could you file a new bug?