Closed
Bug 588506
Opened 14 years ago
Closed 13 years ago
nsSessionStartup is keeping restored session in memory
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 7
People
(Reporter: zpao, Assigned: int3)
References
Details
(Keywords: memory-leak)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
zpao
:
review+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•14 years ago
|
||
This currently allows about:sessionrestore access to your old session, even after you restore. Sort of an accidental feature if I may.
Comment 2•14 years ago
|
||
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?
Reporter | ||
Comment 3•14 years ago
|
||
(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?).
Whiteboard: [good first bug]
Reporter | ||
Comment 5•14 years ago
|
||
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.
Attachment #537685 -
Flags: review?(paul) → review-
Reporter | ||
Comment 7•14 years ago
|
||
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
Attachment #537869 -
Flags: review+
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug] → [incoming]
Comment 8•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/557edc6f6310 But, Paul, I think you put the wrong author...
Assignee: nobody → jezreel
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [incoming]
Target Milestone: --- → Firefox 7
Comment 9•13 years ago
|
||
(In reply to comment #8) > But, Paul, I think you put the wrong author... My bad, I thought it was another patch, sorry :)
Updated•13 years ago
|
Comment 10•13 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•