User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170824053622 Steps to reproduce: browser.sessions.getRecentlyClosed({}).then gets also tabs that were closed in PREVIOUS browsing sessions (i.e. before I file->exit or alt + f4 browser). These tabs have sessionID undefined. Tabs closed in current session have normal sessionId. Actual results: browser.sessions.getRecentlyClosed({}).then gets also tabs that were closed in PREVIOUS browsing sessions (i.e. before I file->exit or alt + f4 browser). These tabs have sessionID undefined. Tabs closed in current session have normal sessionId and it is possible to restore them using browser.sessions.restore. Expected results: These tabs should not be loaded (into array) at all. This bug is perhaps somewhat connected to https://bugzilla.mozilla.org/show_bug.cgi?id=1375546.

Seems to be Tab Mix Plus issue...

TMP session manager needs to be disabled to prevent this issue. Author also promised to look into it...

If this is a bug in sessions.getRecentlyClosed() then it should be a P1 to fix. If it is an issue in the extension, then it should get closed as INVALID.

This is pretty busted (when comparing operation to mdn docs). I'm not even clear how to discern stuff closed in the current session.

This seems like the expected behavior to me. I don't understand why it's a problem.

(In reply to Kris Maglione [:kmag] from comment #5) > This seems like the expected behavior to me. I don't understand why it's a > problem. getRecentlyClosed is returning closed data from all prior sessions. Per docs[1] it is only supposed to return closed data since startup. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions

Comment on attachment 8994615 [details] Bug 1399147 - fix session.getRecentlyClosed to match documented api, https://reviewboard.mozilla.org/r/259148/#review267026 Besides the comment below, it looks that none of the existing tests for this API method is testing this scenario (https://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=browser%2Fcomponents%2Fextensions%2Ftest%2Fbrowser%2Fbrowser_ext_sessions_getRecentlyClosed), and so it would be great to also update some of those tests to include some existing session data and assert that we get the expected session data (which should not be include any closed windows and tabs that wasn't closed during the current session). ::: browser/components/extensions/parent/ext-sessions.js:20 (Diff revision 1) > + // SessionStore sessions are longer lived, we need to limit > + // data for extensions to the session since startup. > + let startupTime = Services.startup.getStartupInfo().process; > + > // Get closed windows > let closedWindowData = SessionStore.getClosedWindowData(false); > - for (let window of closedWindowData) { > + for (let window of closedWindowData.filter(w => w.closedAt >= startupTime)) { I'm a bit worried that we are going to compare the closedAt time with the startup time to select the recently closed windows and tabs, and if the system time changes (e.g. it was wrong and it is being adjusted, or the opposite) we may suddently return unexpected results again :-( It could be a good idea to try to double-check with someone who worked or works on the SessionStore internals if there are any better options that we may be missing (or if there is a better way to make the SessionStore able to produce the data we need doing this kind of filtering from inside the API implementation).

SessionStore has no notion of "current session since startup", only that of "current session" which means a) if "restore session" is false, session since startup b) if "restore session" is true, since pref was set, but limiting/pruning amount of data (e.g. 3 closed windows, regardless of age) So the choices here are: a) do nothing, document on mdn the difference between Firefox and other (at least Chrome) browsers. a.1) possibly make sure both startup time and timestamps are available to extensions, they can then pull out "current session" data on their own. b) add the notion of session since start to SessionStore c) do what we have here, checking timestamps against startup time * has issue of time changes on host machine, but that should be rare I'm not in favor of B since that could be a chunk of work in SessionStore only to have it rewritten later. @mikedeboer, any thoughts on this in relation to SessionStore? @mconca, thoughts around the behavior difference in the sessions api?

I'm not a fan of A since it 1) breaks compatibility with other browsers, and 2) pushes extra work on to every extension that wants to manage sessions. B would be nice, but is not feasible here (but is definitely something to keep in mind if/when sessionStore gets its long-awaited rewrite). So option C is probably preferred, even though Luca correctly identifies a time window where potential issues could occur. Thought - the original submission in comment 0 stated that the sessionID of the tab or window will be invalid if it is from a previously closed session. Is that true in all cases? If so, could that be used to filter out tabs/windows from before the current session started?

(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #10) > Thought - the original submission in comment 0 stated that the sessionID of > the tab or window will be invalid if it is from a previously closed session. > Is that true in all cases? If so, could that be used to filter out > tabs/windows from before the current session started? sessionID is created from closedId in our session data. My current session has closedId values for windows and tabs that I know were from prior to last-startup. There are instances where closedId is not set, I'm unclear to the reason for that right now, but it's certainly not due to being older than last-startup.

Well, I think b) might be a good enough stop-gap solution until we get to a point where we can load the previous session _always_, at which point the 'restore tabs at startup' option would basically become an option to start restoring that session automatically at startup. That way CMD+Shift+T will also just work between restarts. That would be sooo nice. Anyway, I don't see that happening soon. Also, I'm not sure what mconca is talking about with 'its long-awaited rewrite'. Perhaps that hinting to what :kmag mentioned of doing the tab content-restore in C++? In any case, the current state of sessionstore is quite decent, IMHO, but let's talk when there's data that I don't know about! ;-)