677424 - If a tab is closed between quit-application-requested and quit-application-granted then it is still restored when the application restarts

Status: REOPENED

(Firefox :: Session Restore, defect)

Description

[Dave Townsend [:mossop]]

In bug 476430 I have an in-content UI that wants to close itself and restart the application. It sends quit-application-requested first because it doesn't want to close itself if the request is denied, then it closes itself, then it restarts the application which sends quit-application-granted.

Session store gathers its last set of data in quit-application-granted and so it ignores the fact that a tab goes away immediately after that

Comment 1

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

Attachment: Patch v0.1

The alternative to this felt to focused on the case presented here so this is a bit more general purpose (I was going to delete the tab from window.tabs and adjust window.selected in onTabRemove).

I think this will work, but no test yet (well, Dave can say if it works). If I write a test that notifies quit-application-requested, is that going to mess things up? Actually I'm not sure we can test this properly since we really need to be quitting :/

Comment 2

[Dave Townsend [:mossop]]

If any code breaks after receiving multiple quit-application-requested notifications then that code is itself broken

Comment 3

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

(In reply to Dave Townsend (:Mossop) from comment #2)
> If any code breaks after receiving multiple quit-application-requested
> notifications then that code is itself broken

I wasn't worried so much about that, just wasn't sure if it would trigger anything (eg. the quit dialog) that wasn't something we could pref off.

Comment 4

[Dave Townsend [:mossop]]

Oh, good point. Yes it is bound to do that in some cases, but you can handle that in the test.

To really handle the shutdown/restart properly we probably need mozmill tests for session restore, maybe some even already exist

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

The previous patch doesn't work because the tab is still in tabbrowsers list of tabs at the time the TabClose event is dispatched. This works by removing the tabs data from the cached window state when it is removed.

Comment 6

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

Comment on attachment 552242 [details] [diff] [review]
patch rev 1

This is going to make the e10s work a little bit more interesting with the currentURI check, but that problem is for future-Paul/Jez to figure out. This will need tweaking when app tabs happen like I want them too (but that's just a comment for me to remember).

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/d980e7a3aa49

Comment 8

[Dave Townsend [:mossop]]

Backed out due to test failures (timeout in browser_500328.js on OSX debug builds)