The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 522169 [details] [diff] [review]
patch v1
Attachment #522169 - Flags: review?(paul)
(Assignee)

Comment 2

6 years ago
Comment on attachment 522169 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=f919850a5c0a
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...
(Assignee)

Comment 4

6 years ago
(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.
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 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.
Attachment #522169 - Flags: review?(paul) → review-
(Assignee)

Comment 7

6 years ago
(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.
See also bug 671598, wherein Jetpack stops depending on this notification.
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 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.
Attachment #522169 - Flags: review- → review+
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
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/2ca877908e8d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
and
http://hg.mozilla.org/mozilla-central/rev/36b6ae568a98
You need to log in before you can comment on or make changes to this bug.