Last Comment Bug 645428 - "sessionstore-windows-restored" is not sent for empty sessions
: "sessionstore-windows-restored" is not sent for empty sessions
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-26 18:37 PDT by Tim Taubert [:ttaubert]
Modified: 2011-08-16 04:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (4.06 KB, patch)
2011-03-26 18:46 PDT, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-03-26 18:37:24 PDT
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] 2011-03-26 18:46:49 PDT
Created attachment 522169 [details] [diff] [review]
patch v1
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-04-11 14:48:46 PDT
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] 2011-04-26 04:30:45 PDT
(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 reading much bugmail, email directly) 2011-04-26 11:42:15 PDT
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 reading much bugmail, email directly) 2011-05-10 13:33:44 PDT
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] 2011-05-12 08:21:58 PDT
(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) 2011-08-08 09:20:24 PDT
See also bug 671598, wherein Jetpack stops depending on this notification.
Comment 9 Dietrich Ayala (:dietrich) 2011-08-08 09:23:20 PDT
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 reading much bugmail, email directly) 2011-08-15 18:25:10 PDT
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 reading much bugmail, email directly) 2011-08-15 18:26:08 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-16 04:11:05 PDT
http://hg.mozilla.org/mozilla-central/rev/2ca877908e8d
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-16 04:11:24 PDT
and
http://hg.mozilla.org/mozilla-central/rev/36b6ae568a98

Note You need to log in before you can comment on or make changes to this bug.