Session manager don't save state:"stopped" when closing session with one about:blank tab when Firefox open the next time, the about:sessionRestore tab is open Steps to reproduce: 1. open Firefox with one window with one about:blank tab 2. make sure you don't have any closed windows or tabs 3. close the window if you open sessionstore.js file you see "state":"running" in the file the same apply if user close all tabs before existing Firefox and browser.sessionstore.max_tabs_undo = 0 browser.sessionstore.max_windows_undo = 0 browser.newtab.url = about:blank look like the fix in 829568 part 1 trigger this bug

This problem also seems to be causing a problem each with a false crash recovery prompt when starting Firefox after closing Firefox 20 with only an about:blank tab. Steps to reproduce: 1. open Firefox with one window with one about:blank tab 2. close the window 3. open Firefox 4. Crash recovery screen appears with one window and one about:blank tab If you close Firefox with one or more non-blank tabs, Firefox starts normally.

I can reproduce this issue on Windows with Firefox 21, 22.0b and 23.0a however in order to trigger this on windows you need to close Firefox (one window with one about:blank tab) with File > Close Window or with the window close button (top right), closing Firefox with File > Exit work ok

I can't reproduce with the STR in comment 0. Does it happen in Safe Mode (see https://support.mozilla.org/kb/troubleshoot-firefox-issues-using-safe-mode)? Does it happen with a new profile (see https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles)?

I can reproduce it on every build from Firefox 20 up on new profile in safe mode! 1. create new profile 2. start Firefox in safe mode 3. open NEW about:blank tab (don't load about:blank on existing tab) 4. select the new tab and close other tabs 5. close the window by using File > Close Window or with the window close button (top right). if you open sessionstore.js file you see "state":"running" in the file

(In reply to onemen.one from comment #4) > 3. open NEW about:blank tab (don't load about:blank on existing tab) How can you do that? Opening a new tab opens the New Tab page with thumbnails. To get a blank page, I need to type about:blank in an existing page.

type about:blank in the address bar and click Alt+Enter to open it in new tab

I can see "state":"running" in sessionstore.js but no about:sessionrestore at the next startup.

toolkit.startup.max_resumed_crashes controls how many times Firefox will restore the previous session automatically without opening about:sessionrestore. just repeat the process few times

(In reply to onemen.one from comment #8) > just repeat the process few times I've got the Session Restore page after a few times. (In reply to onemen.one from comment #0) > look like the fix in 829568 part 1 trigger this bug Alice, can you confirm?

Good: http://hg.mozilla.org/mozilla-central/rev/e8f8a3f6f1f6 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130218 Firefox/21.0 ID:20130218212328 Bad: http://hg.mozilla.org/mozilla-central/rev/3f0f2fc4bd0f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130219 Firefox/21.0 ID:20130219014027 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8f8a3f6f1f6&tochange=3f0f2fc4bd0f

It looks like this was fixed for OS X by bug 845681 by #defining out the code added by bug 829568 part 1. Note, the underlying problem is that SessionStore won't save windows with nothing but a blank tab (with no history) in them. See: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#4085 Called from: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#950 As such when shutting down the browser with a single blank tab, the saveState procedure returns without saving: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#3686 Removing the code added by bug 829568 part 1 will fix this problem. If that isn't desirable, then only executing that code if "this._loadState == STATE_RUNNING" should also work.

The code added by bug bug 829568 part 1 just reveals another bug when closing a non-private window with one about:blank or about:newtab tab in single window session then is saveState we call getCurrentState > var oState = this._getCurrentState(aUpdateAll); oState returned by _getCurrentState to saveState have: oState.windows.length = 0 oState._closedWindows.length = 0 in that case when we get here > if (oState.windows.length == 0) { > TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_MS"); > TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS"); > return; > } we finish the function without saving the up-to-date data (remember the window was not private) and we end up the session with state: "stopped" never save to disk!! if we dig more into the code we get to onClose: > if (this._loadState == STATE_RUNNING) { // window not closed during a regular shut-down > // update all window data for a last time > this._collectWindowData(aWindow); > ...... > // Save the window if it has multiple tabs or a single saveable tab and > // it's not private. > if (!winData.isPrivate && (winData.tabs.length > 1 || > (winData.tabs.length == 1 && this._shouldSaveTabState(winData.tabs[0])))) { > // we don't want to save the busy state > delete winData.busy; > > this._closedWindows.unshift(winData); > this._capClosedWindows(); > } > > // clear this window from the list > delete this._windows[aWindow.__SSi]; > delete this._internalWindows[aWindow.__SSi]; it is obvious that after this block, that window with one tab with shouldSaveTabState false wasn't saved in _closedWindows when closing a non-private window with one about:blank or about:newtab tab in single window session both this._windows and this._closedWindowsis are empty one possible fix is to add this to onClose: > > // clear this window from the list > delete this._windows[aWindow.__SSi]; > delete this._internalWindows[aWindow.__SSi]; >+ >+ if (!winData.isPrivate && >+ !Object.keys(this._windows).length && !this._closedWindows.length) { >+ let emptyWinState = { tabs: [{ entries: [{ url: "about:blank" }] }] }; >+ this._closedWindows.unshift(emptyWinState); >+ } > > // save the state without this window to disk > this.saveStateDelayed(); > }

Simple fix will be to change the code added by bug 829568 part 1. > // Don't save invalid states. > // Looks like we currently have private windows, only. >- if (oState.windows.length == 0) { >+ if (oState.windows.length == 0 && !this._nonPrivateSession) { > TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_MS"); > TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS"); > return; > } >+ // We are not in private session anymore. >+ this._nonPrivateSession = true; This fix can also replace the fix done by bug 845681

Hi Alice, thanks for the nom. Why do you believe this issue is critical enough to track for upcoming releases? Doesn't appear to be a recent regression or very user impactful, so we'd like more context. Thanks!

(In reply to Alex Keybl [:akeybl] from comment #14) > Hi Alice, thanks for the nom. Why do you believe this issue is critical > enough to track for upcoming releases? Doesn't appear to be a recent > regression or very user impactful, so we'd like more context. Thanks! This bug was reported in 2013-03-22. And was regressed in 2013-02-19. I think it enough.

Not only that but the cause of the bug is known. See comment #11 As for user impact, it can cause Firefox to think the browser crashed when it did not. Similar to bug 845681, the fix for which caused this bug.

Needinfo'ing :gavin.:ttaubert here to get some help with prioritization here given this a fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=829568 & its on the Firefox team's radar. From Release stand point given this has been there for a couple of versions in Firefox it would not block on shipping Firefox 24 so not tracking this as a blocker but happy to uplift if a low risk fix was found.

This will be fixed by bug 888373 when we have a better "crash detection" API at hand. The current crash detection mechanism in SessionStore has a lot of drawbacks.

Firefox 24 is the ESR and enterprise folks are are already seeing this problem in their testing. This isn't a problem people would like to have for almost a year...

This appears to have gotten worse in 24.0. It now happens to me when the tab is about:newtab as well as about:blank (previously, closing on newtab had been my workaround). I now get a false crash prompt almost every time I start Firefox.

This affects Linux as well.

Just to add a 'me too' - I can reproduce this problem with ESR 24.0 on Linux Whatever the 'fix' is, it will need to be back ported to the ESR 24.0 stream

Please don't add any further 'me too's. That's really not helpful, we are working on a patch already and know that people are affected.

The follow-up fix for bug 845681 needs to be revisited as well. Also on Mac we can start Firefox with only a private window - by running "firefox -private-window" - and that means we can't just exclude OSX here.

For a start, we should definitely revert the changes made in bug 829568 part 1. With the current crash detection (and until bug 888373 lands) we should always save to disk.

(In reply to Tim Taubert [:ttaubert] from comment #26) > For a start, we should definitely revert the changes made in bug 829568 part > 1. With the current crash detection (and until bug 888373 lands) we should > always save to disk. I concur.

Tim, what would happen if we removed the file instead of returning if there is no window?

(In reply to David Rajchenbach Teller [:Yoric] <on summit, unavailable until October 8th> from comment #28) > Tim, what would happen if we removed the file instead of returning if there > is no window? That wouldn't really work as we have to write at least once to create a backup of the sessionstore.js file that we would fall back to on the next start. Also removing the file sounds weird to me and might have other undesired effects that don't come to mind right now.

This patch removes the code that made us bail out on non-mac systems. We always save the sessionstore file but modify it before writing to include the original session state in case the user didn't open a single non-private window, yet. This was a little harder to fix because _prepDataForDeferredRestore() unfortunately modifies and destroys the state for deferred sessions. I worked around this by just cloning the session state using JSON.parse/stringify which is explained by the comment in the patch. We should file follow-up bugs to make _prepDataForDeferredRestore() and _splitCookiesFromWindow() as pure as possible. It really bothers me for quite some time now that those functions have side-effects. The patch is locally passing all restart tests I'm currently working on in bug 923606. I wrote tests specifically for this bug to ensure we don't lose sessions that are automatically restored as well as deferred sessions.

Comment on attachment 814011 [details] [diff] [review] Ensure we don't lose sessions when starting with a private window only Review of attachment 814011 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +727,5 @@ > if (isPrivateWindow) { > // We're starting with a single private window. Save the state we > // actually wanted to restore so that we can do it later in case > // the user opens another, non-private window. > + this._deferredInitialState = gSessionStartup.state; Do I understand correctly that the difference between gSessionStartup.state and aInitialState is the fact that the latter is optional? @@ +3530,5 @@ > * @param state > * The state, presumably from nsISessionStartup.state > * @returns [defaultState, state] > */ > _prepDataForDeferredRestore: function ssi_prepDataForDeferredRestore(state) { Side-note: the documentation of this method is a tad weird. We should improve it once we have the opportunity. @@ +3536,5 @@ > + // nsSessionStartup.state. Converting the object to a JSON string and > + // parsing it again is the easiest way to do that, although not the most > + // efficient one. Deferred sessions that don't have automatic session > + // restore enabled tend to be a lot smaller though so that this shouldn't > + // be a big perf hit. It would be nice to have telemetry on this, but this can wait.

(In reply to David Rajchenbach Teller [:Yoric] from comment #31) > > if (isPrivateWindow) { > > // We're starting with a single private window. Save the state we > > // actually wanted to restore so that we can do it later in case > > // the user opens another, non-private window. > > + this._deferredInitialState = gSessionStartup.state; > > Do I understand correctly that the difference between gSessionStartup.state > and aInitialState is the fact that the latter is optional? Ummm... yeah. aInitialState is only passed when we call onLoad() for the first time with an initial session read from disk.

This should be fixed in Nightly after this has been merged to central. In the meantime it would be great if someone could verify that this works as expected: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-linux/1381864971/ http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-linux64/1381864971/ http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-macosx64/1381864971/ http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-win32/1381864971/ Thanks!

Has this also been fixed in ESR 24 ?

Since this problem seems to have gotten worse in FF 24, it would be nice to have this on the ESR so we don't have to live with it for a year. How low risk is the fix?

The fix for this issue has landed in today's Nightly. Can someone please verify that indeed this has been fixed? We also need to make sure this doesn't regress bug 829568 and fixes bug 918276 as well. I will then request approval for uplifts.

I can't reproduce the problem with the latest Nightly on Linux (i386)

Comment on attachment 814011 [details] [diff] [review] Ensure we don't lose sessions when starting with a private window only [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 829568 User impact if declined: Misbehaving crash detection showing about:sessionstore when quitting the browser normally with blank tabs. Testing completed (on m-c, etc.): Landed on m-c without issues. Fixes this and bug 918276 without regressing bug 829568. Risk to taking this patch (and alternatives if risky): Risk should be fairly low. I wrote a bunch of marionette tests that test our shutdown and restart paths (for bug 853779, bug 918276, and bug 919532) and they all pass locally. They're not in our test infrastructure as we're missing some test framework patches that haven't landed yet. String or IDL/UUID changes made by this patch: None Asking for approval because we're tracking bug 918276 for Fx 25 + 26.

Thanks for the uplift request. Too close to release to take a non-critical fix. I'm thinking we should resolve on 26, but I'll leave it up to that triage.

(In reply to Alex Keybl [:akeybl] from comment #41) > Thanks for the uplift request. Too close to release to take a non-critical > fix. I'm thinking we should resolve on 26, but I'll leave it up to that > triage. I think it definitely should be as currently every morning when I launch Firefox, all the popups I closed from the day before are restored because Firefox thinks it crashed and because of the way restore works when there is no current window tabs, it restores the closed windows. It's really annoying.

Comment on attachment 814011 [details] [diff] [review] Ensure we don't lose sessions when starting with a private window only OK for uplift on Aurora - thanks for the extra tests

Thanks for the approval! https://hg.mozilla.org/releases/mozilla-aurora/rev/7453a764f9a9

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 Verified as fixed on latest Aurora 26.0a2 (buildID: 20131023004005) and latest Nightly 27.0a1 (buildID: 20131022030202). Also verified that bug 918276 is fixed as well. The issue still exists in 24.1.0ESR (buildID: 20131021230807).

Any news on a fix for ESR 24 ?

Tim can you please help with a backport on esr24 branch here ? Thanks !

Took me a while to get our marionette restart/shutdown tests running on esr24 but the patch is looking good. It's even a tad smaller than the original patch.

Comment on attachment 829224 [details] [diff] [review] backport for esr24 Review of attachment 829224 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Comment on attachment 829224 [details] [diff] [review] backport for esr24 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: I think bajaj may have more to say about that? User impact if declined: Misbehaving crash detection showing about:sessionstore when quitting the browser normally with blank tabs. Fix Landed on Version: 27, uplifted to 26. Risk to taking this patch (and alternatives if risky): Risk is quite low. The patch is running fine on 26-28 for quite some time now and it's passing all our shutdown/restart test as well as the unit test suite. String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

(In reply to Tim Taubert [:ttaubert] from comment #50) > Comment on attachment 829224 [details] [diff] [review] > backport for esr24 > > [Approval Request Comment] > If this is not a sec:{high,crit} bug, please state case for ESR > consideration: I think bajaj may have more to say about that? Although this does not typically fall in the general criteria for landing a patch on esr as stated in (https://wiki.mozilla.org/Release_Management/ESR_Landing_Process) this is a occasional exception we are considering due to: a) feedback on this issue on enterprise mailing list and the demand for resolving this bug by people who actually are running into this from deployments b) this is a Fx 21 regression and was reported strongly when Firefox 24 esr was out as admin's were in the midst of qualifying the migration from esr 17-> esr 24 c) low risk and well baked on Fx 26 > User impact if declined: Misbehaving crash detection showing > about:sessionstore when quitting the browser normally with blank tabs. > Fix Landed on Version: 27, uplifted to 26. > Risk to taking this patch (and alternatives if risky): Risk is quite low. > The patch is running fine on 26-28 for quite some time now and it's passing > all our shutdown/restart test as well as the unit test suite. > String or UUID changes made by this patch: None > > See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more > info.

Matt - we're getting reports of issues with tab restore (for some, not all) of our ESR 24.2 users - can you look into this bug and double check that you can verify it fixed based on the STR?

I reproduced this issue on FF24 and FF24.0esr. I then tried it on FF24.2.0esr - Win and Mac - and was not able to reproduce. I tried many times. If the steps have changed, or if there is more specific info available that can help us find this, I'd love to know. In the meantime, I have not been able to see it.