588506 - nsSessionStartup is keeping restored session in memory

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Errrr, this isn't so awesome.

If you crashed & can resume, or will be restoring (sessionstore.resume_session_once || startup.page == 3) then we keep your entire session in memory for the entirety of your session, even if you go into PB mode.

This might actually be useful for bug 588482, but in general it's a bad thing to do. For those people who always restore and/or have multi-megabyte sessions (I'm looking at you Wayne), this is automatically devouring that memory.

So we can do this a couple ways, which will depend on how I go about bug 588482. We'll either want to add a method that wipes, or autowipe after a certain notification ("sessionstore-windows-restored" would make sense right now, but that might have to change soon).

I wouldn't block on this until we figure out what's going to happen elsewhere. But if we can do it, we might as well. I'm raising the blocking idea just because it might require API changes. We've been like this for many releases (since session store landed?) so I guess it's not super pressing...

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This currently allows about:sessionrestore access to your old session, even after you restore. Sort of an accidental feature if I may.

Comment 2

[Wayne Mery (:wsmwk)]

zpao, thanks for looking at me :)

how has this changed since bug 588482 is fixed?  
and does this issue contribute to making Bug 581510 - user is not warned about "out of memory ... nsSessionStore.js" - happen sooner?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> zpao, thanks for looking at me :)
> 
> how has this changed since bug 588482 is fixed?  

It's changed in that we keep an pre-processed copy (sans app tabs) of the data in memory in sessionstore (as well as the raw copy in sessionstartup) - only when not restoring normally or resuming after crash.

> and does this issue contribute to making Bug 581510 - user is not warned about
> "out of memory ... nsSessionStore.js" - happen sooner?

Possibly. Any time we're using more memory than we need to could be contributing.

Dietrich mentioned to me briefly when looking at this after I filed it that clearing after "sessionstore-windows-restored" make sense. So that's what I'll do.

It would be relatively easy for somebody to write code to load sessionstore.bak (which would hold the previous session) and show about:sessionrestore for that. In fact I assume that session manager and it's ilk can already do that. I think it's time to stop "supporting" the hidden about:sessionrestore feature (bug?).

Comment 4

[Jez Ng [:int3]]

Attachment: Patch v1

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 537685 [details] [diff] [review]
Patch v1

Review of attachment 537685 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few comments and then it'll be good to go!

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ +154,3 @@
>        this._sessionType = Ci.nsISessionStartup.RESUME_SESSION;
>      else if (initialState)
>        this._sessionType = Ci.nsISessionStartup.DEFER_SESSION;

Let's keep the else here. You're right that _iniString will be reset always, but I'm not 100% sure we can wait until after sessionstore has read (or attempted to read) the data.

@@ +159,1 @@
>  

And then you can get rid of this empty line.

@@ +197,3 @@
>        break;
>      case "sessionstore-windows-restored":
>        Services.obs.removeObserver(this, "sessionstore-windows-restored");

Add a comment here about what's happening. Something along the lines of "nsSessionStartup should clear it's data after nsSessionStore has read it"

@@ +197,4 @@
>        break;
>      case "sessionstore-windows-restored":
>        Services.obs.removeObserver(this, "sessionstore-windows-restored");
> +      this._iniString = null;

We can just go ahead and set _sessionType to NO_SESSION here and stop observing "browser:purge-session-history". We're clearing the session anyway, so there's no need to keep the _sessionType in a state that lies.

Comment 6

[Jez Ng [:int3]]

Attachment: Patch v2 with the changes from the above review added in.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 537869 [details] [diff] [review]
Patch v2 with the changes from the above review added in.

Review of attachment 537869 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 8

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/557edc6f6310

But, Paul, I think you put the wrong author...

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> But, Paul, I think you put the wrong author...

My bad, I thought it was another patch, sorry :)

Comment 10

[Pritpaul]

Hi folks,

I had definitely noticed the higher memory consumption when Firefox was started with restore data and therefore am glad this bug is fixed for this reason.  However, I relied very heavily on the apparently accidental (I didn't realize) feature mentioned in comment #1 that let about:sessionrestore show me the session even after it was restored.  (The webmail I use has concurrency issues and sometimes messes up without recovery state if too many requests are sent to it at once.)  I realize that this is best fixed in the webmail, but I can't do that.  I also know Firefox devs aren't likely to do anything for rare use cases and am not asking you to do so.  I'm only posting to ask whether anyone knows if the way of making this still work mentioned in comment #3 (with sessionrestore.bak) has been implemented in an extension or something, or if there are any other known ways to get this functionality.  I realize this bug may not be the best place to ask, but I figured that if anyone else was relying on this "accidental feature" they may also end up here to look for a solution.

Thanks!