645428 - "sessionstore-windows-restored" is not sent for empty sessions

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Steps to reproduce:

1.) Enable "Show my windows and tabs from last time"
2.) Close all tabs but one blank tab.
3.) Restart Firefox.

Actual result:

"sessionstore-windows-restored" is not fired.

Expected result:

"sessionstore-windows-restored" is fired.


This bug especially hits Jetpack because that waits until "sessionstore-windows-restored" is fired and loads the add-on afterwards - so that prevents Jetpack add-ons from being loaded.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522169 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=f919850a5c0a

Comment 3

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

Comment on attachment 522169 [details] [diff] [review]
patch v1

So this seems mostly reasonable, however I haven't actually been able to get my sessionstore.js into the state of no windows following your STR.

>+  ss.setBrowserState(JSON.stringify({ windows: [] }));

Calling this doesn't actually change the current state, so I wouldn't expect any notifications to go out.
I know the whole point of this is to notify with an empty session, but I wouldn't expect this case to notify. The startup case should always notify, but this doesn't actually cause any change in state, so a notification doesn't really seem right (though if using that as an async way to say "keep going" then perhaps...

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #3)
> So this seems mostly reasonable, however I haven't actually been able to get my
> sessionstore.js into the state of no windows following your STR.

With session restore enabled and only one blank tab shown I get the following sessionstore.js after closing the browser:

{"windows":[],"selectedWindow":0,"_closedWindows":[],"session":{"state":"stopped","lastUpdate":1303816713038}

> >+  ss.setBrowserState(JSON.stringify({ windows: [] }));
> 
> Calling this doesn't actually change the current state, so I wouldn't expect
> any notifications to go out.
> I know the whole point of this is to notify with an empty session, but I
> wouldn't expect this case to notify. The startup case should always notify, but
> this doesn't actually cause any change in state, so a notification doesn't
> really seem right (though if using that as an async way to say "keep going"
> then perhaps...

Yeah, ok that makes sense - if nothing is restored no notification is sent. So maybe the Addon-SDK should use a different way to determine when to load an add-on?

There are people out there that seem to clean up the browser window (close all their tabs and leave a blank one) but have session restore enabled. On the next browser start no jetpack add-ons are loading.

Comment 5

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

Ah, so further investigation has revealed why I couldn't repro... this only happens on Windows & Linux when quitting by closing the last window (lastwindow-close-requested). When doing normal quit (quit-application-requested), we save the session as a single tab with "about:blank" with closed tab data and everything still intact (proper behavior).

So I think we should probably fix this the "right" way and make sure sessionstore saves the file correctly or not at all.

The Addon-SDK is doing the right thing by waiting for that topic. That topic should go out every startup regardless of if a session is being restored or not. That topic is also what's used in nsBrowserGlue, so it definitely needs to be sent out - I just want to make sure it's fixed correctly.

Comment 6

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

Comment on attachment 522169 [details] [diff] [review]
patch v1

Based on my previous comment, I think we should fix this the right way since we can, so r-. If we can't by the time 6 freeze comes around, then I'd be willing to take this and followup with a proper fix.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #6)
> Based on my previous comment, I think we should fix this the right way since
> we can, so r-. If we can't by the time 6 freeze comes around, then I'd be
> willing to take this and followup with a proper fix.

So the actual problem is:

(1) Quitting the browser:
    * quit-application-requested
    * quit-application-granted
    * domwindowclosed
    * quit-application

(2) Closing the last window:
    * quit-application-requested
    * domwindowclosed
    * quit-application-granted
    * quit-application

So in (2) ss.onClose(win) is called _before_ ss._loadState is set to STATE_QUITTING at quit-application-granted. That is why the data gets overwritten and we get the blank state.

What's the right way to fix this? We can't really set STATE_QUITTING before it's granted. There is no notification of quitting being cancelled either.

Comment 8

[Dietrich Ayala (:dietrich)]

See also bug 671598, wherein Jetpack stops depending on this notification.

Comment 9

[Dietrich Ayala (:dietrich)]

If there are valid scenarios for this notification not being sent at startup with default prefs, can you describe those scenarios here: https://developer.mozilla.org/en/Observer_Notifications.

Comment 10

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

Comment on attachment 522169 [details] [diff] [review]
patch v1

We're now at 8, so let's take this. While it's not the ideal situation, it's better than nothing.

Comment 11

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/2ca877908e8d
and then I pushed a change to the test to keep it simple
http://hg.mozilla.org/integration/mozilla-inbound/rev/36b6ae568a98

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/2ca877908e8d

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

and
http://hg.mozilla.org/mozilla-central/rev/36b6ae568a98