408338 - Thunderbird should have session restore

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement, P2)

Description

[Dave Miller [:justdave]]

There's not a lot of state that gets lost when you restart Thunderbird, but there's a few things that would be nice to preserve across a restart:

1) scroll positions within mailboxes
2) "unseen" message indicators on folders (as opposed to "unread"). In current incarnations of Thunderbird this is the blue highlight on a folder. It gets lost when you restart Thunderbird, whether you actually looked at the mailbox or not.
3) which mailbox you currently had open
4) any compose windows you might have had open at the time
5) message windows that were open for reading

Comment 1

[Omar Bajraszewski]

The things should be also restored after Thunderbird crash

Comment 2

[Dave Miller [:justdave]]

FWIW, this is a huge roadblock to getting daily nightly build testing from me.  I'll often delay for days on end without letting Software Update install a nightly because it means losing all of my "unseen" folder indications, so I need to go through and make sure I've seen everything that's important to see before I can allow Thunderbird to quit and restart in order to install the update.

Comment 3

[Wayne Mery (:wsmwk)]

Dave. 
So this capability/bug for you is dogfoodish?  
This might help bring more people to testing TB trunk! interesting idea.

I know session restore greatly eased my pain of running FF trunk, having run trunk for about half a year in 2006, then left for along time when it became painful with their unfixed crashes and memory consumption problems.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

I certainly agree that this would be nice to have, but it's not really clear to me how important this is.  I'd be interested in hearing Bryan's thoughts here.  

Comment 5

[Wayne Mery (:wsmwk)]

couple additional electrons - perhaps this is beyond the scope of TB3. However, if the concept has merit, now is the time to start talking what might be needed to make it work, given the likelihood that some architectural changes might be required.

Having said that, it strikes me TB is at least presently a much different animal than FF. Unless there are many users who have open many message panes, windows and compose windows then the value here might not be worth implementing, depending on the amount of work required.  

There is also an opportunity cost to be considered - are we better off putting planning and man hours into architecural changes and processes which improve quality that in turn reduce the need for session restore?  (this is a dead argument of course if that effort isn't applied)

Also we already have autosave for draft messages.

Comment 6

[Jeff Beckley]

This is something we've already got partially implemented in Penelope, although it's more limited in scope.  In Penelope it's just the top-level windows, and not the unseen info or scroll positions.  It also needs to be updated for tabs.

Comment 7

[David Ascher (:davida)]

Jeff, any pointers to code or bugs that we could look at?

Comment 8

[Jeff Beckley]

I don't have anything checked in yet.  I'll let you know when I do.

Comment 9

[Jeff Beckley]

btw, the Penelope tracking bug for this is bug 366863.

Comment 10

[WADA:World Anti-bad-Duping Agency]

panacea.dat is currently used as central data base for this purpose.
 - "Account was collapsed or expanded" is lost if panacea.dat is deleted.
panacea.dat is mork DB. Transfer to sql-lite db was tried once but changed back to mork DB soon for performance reason, mainly by severe slowness in mail DB open/close area.

I think following is possible.
 (1) Introduce new DB of sql-lite for status save of non-frequent change.
 (2) Shift to the new sql-lite DB when status of non-frequent change.
     - Account is collapsed or expanded, last chosen folder, ...
     - View choice, Column choice/posision,width,...
 (3) Add status save/restore logic for other fields/choices/settings.
i.e. Use panacea.dat(mork) for cache like purpose, and use sql-lite DB for status save/restore purpose. 

(1) is easy, because "transfer from mork to sql-lite" was tried once and it worked well, and there is no problem in (3) because new logic.
(2) is very tough or hard work?

Comment 11

[WADA:World Anti-bad-Duping Agency]

FYI.
Bug 418551 is for transition from panacea.dat(mork) to msgFolderCache.sqlite.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

This is going to be very important once we have the tabbed interface model worked out.  Part of the cause for having tabs is to allow people to keep their existing context in the window they are currently using.  (i.e. if you're reading mail and then need to perform a search of mail you shouldn't have to decide to lose your reading position in order to make that search)  So assuming people have searches and other views opened up in tabs inside Thunderbird we'll want to keep that full context if they shut down thunderbird.

Comment 13

[Dave Miller [:justdave]]

(In reply to comment #5)
> Also we already have autosave for draft messages.

Except that it doesn't remind you that the drafts are there.  I've had a few times recently where I was in the middle of composing a message, got distracted by another incoming email, and the compose window moved to the background.  I would have eventually found it there and continued, except that something came up that made me restart Thunderbird...  since the message was auto-saved, I didn't get prompted to save it.  But when Thunderbird restarted, it didn't reopen the compose window, either.  So then it sits in the drafts folder (for a couple weeks in one case) until I suddenly notice the message counter in the drafts folder and go look at it.

Comment 14

[Eric Shepherd [:sheppy]]

Has there been any movement on this? With the addition of saved searches and tabs, session restore seems pretty critical to the workflow of users.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

As much as we'd like to see this, it's not currently a blocker for Tb3.  If someone is interested in stepping up to work on it, though, that'd be great.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

The fix for bug 498106 which implements tab persistence has landed.  This does not do full "session restore", so I am not marking this bug fixed.  All the current fix handles is the tabs for one 3-pane window.  Message windows are not covered, additional 3-pane windows are not covered.  Window positions are not handled.  Things like scroll-bar positions are left up to the tabs themselves, so I'm not sure that's really the domain of this bug... seems more like a tracking bug thing.

Comment 17

[Wei Xian Woo]

Hi,
I'd be happy to take this on. I'm a student from Waterloo working on Thunderbird as part of an open source distributed development course this Winter term.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Hello and thanks for your interest!

The current state of things is as per my previous message.  When each 3-pane window gets unloaded as part of a normal (non-crash) shutdown process, it calls persistTabState which relies on the tabmail implementation's persistTabs function to serialize the state of the open tabs for that window.  persistTabState writes to the file we save to directly, so if there are multiple 3-pane windows, they will fight each other to save to that file.  At startup, we create a 3-pane window by default, and during the load process of that window "atStartupRestoreTabs" assumes it is the only 3-pane window and grabs the contents of the file we saved before and passes that data to tabmail's restoreTabs implementation.

The two major deficiencies are thus:

1) We only save tab state at clean shutdown.  If Thunderbird crashes, we do not save the state.  This was done somewhat intentionally to avoid a cycle where we save a state that causes Thunderbird to crash on startup.  This problem can probably be avoided by just deleting the session restore file after reading it but before attempting to restore from it.  That way if we crash during the restore, it won't be around the next time.

You would want to add code that periodically saves the state of the Thunderbird window(s) *if any changes have occurred*.  To avoid causing laptops to spin up their hard disk if no meaningful changes have occurred, it might make sense to keep a copy of the file contents that we write to disk in memory and only write a new revision of the file if those contents have changed.  It would probably make sense to put the timer-based code in a JS module that gets initialized by Thunderbird at startup (when it checks to restore the state) that uses an nsITimer to periodically run the function that checks the current state of things.

2) That we assume we only have a single 3-pane window.  This is not a major concern to me, but it's solvable.  There is a window mediator service that allows the JS code to get a list of the active windows.  If you created a JS module as mentioned in the previous point, it could use the service to find all active mail:3pane windows and combine their persisted states into a single file.  Besides doing this on a timer (as for the previous point), you would probably want to trigger this logic when the user chooses to quit Thunderbird from the file menu.


Other things you could also consider doing would be increasing the 'fidelity' of the saved tab state as I think I alluded to in the previous message.

Comment 19

[Wei Xian Woo]

Thanks for the detailed info, Andrew!

Here is how I intend to start approaching this:

Create a new JS module (called sessionRestoreManager), which will be initialized on startup.

On startup, the sessionRestoreManager will:
- load "session.json" then delete it
- attempt to restore session data
   *** since we create a 3-pane window by default at startup, the session restore manager will simply restore the session data for this window. for other windows, the session restore manager will need to create those windows and then reload their state. (I have a question about this and will ask you on IRC)
- start a timer

When the timer "expires":
- get the list of windows from the window mediator service
- for each window, get the window's state and stick it in an array
- compare the array to a previously persisted array (in memory)
   *** not sure if it's worth hashing the array and only keeping the hash in memory since the array probably won't be very big
- if the arrays are different, write the array to disk and replace the previously saved array with this one

For my first patch, the only difference from the current behaviour is that session state will be persisted in the event of a crash.

I will implement the other things in subsequent patches for:
- persisting state for multiple 3-pane windows
- persisting state for other types of windows
- increasing the 'fidelity' of the saved tab state

Comment 20

[Wei Xian Woo]

Attachment: Session Store Manager implementation + Mozmill tests

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 427699 [details] [diff] [review]
Session Store Manager implementation + Mozmill tests

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -531,26 +523,34 @@
> function atStartupRestoreTabs(aDontRestoreFirstTab) {
>+  // determine whether the app is just starting up
>+  let startup = sessionStoreManager.startup();
>+  

There are some trailing spaces here, and in a couple of the other files, which should be removed.

>+  // initialize the session store module (in case it's not already running)
>+  sessionStoreManager.init();
>+  
>+  if (sessionStoreManager.doRestore()) {

Since you're only restoring if sessionStoreManager.state is available, what do you think about having sessionStoreManager.doRestore() return the state to restore?  It would let you avoid that second if statement below.

>+    let state = sessionStoreManager.state;
>+    if (state) {
>+      let tabsState = state.tabs;
>+      let tabmail = document.getElementById('tabmail');
>+      tabmail.restoreTabs(tabsState, aDontRestoreFirstTab);
>+      
>+      // if the last session crashed, restore other windows if there were any.
>+      // do this only at first window load, i.e. on app startup.
>+      if (startup) {
>+        if (sessionStoreManager.lastSessionCrashed()) {

I think this would be better expresed as:
>+      if (startup && sessionStoreManager.lastSessionCrashed()) {
again, saving an if statement.  ;)

>+          // XXX we might want to display a restore page and let the user decide
>+          // whether to restore the other windows, just like Firefox does.
>+          sessionStoreManager.recoverLastSession(window);

I think the sessionStoreManager.recoverLastSession should be responsible for displaying the restore pane, if it wants to.  (So you can move this comment into that function.

>+++ b/mail/base/modules/sessionStoreManager.js
>@@ -0,0 +1,261 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Wei Xian Woo <[e-mail]>

So, this lead me to an interesting question.  Who does own this code?

I posted to the mozilla.legal newsgroup, and the answer is:
>> The correct Initial Developer is whoever owns the copyright. I am
>> not a lawyer, but unless UT has some really onerous copyright
>> policy which assigns student work to the university I believe the
>> correct Initial Developer is the student.
> Or unless the student has assigned copyright to Mozilla Messaging,
> or unless the student started the file by copying some code from a
> file whose ID was Mozilla Messaging.

So, feel free to list yourself as the Initial Developer, if you'ld like.  Or, if you started by copying another file, or want to assign the copyright to Mozilla Messaging, feel free to leave it the way it is.

>+/* :::::::: The Module ::::::::::::::: */
>+
>+var sessionStoreManager =
>+{
>+  _startup: true,
>+  _timerInitialized: false,
>+  
>+  // the state to restore at startup
>+  _initialState: null,
>+  
>+  // the current state
>+  _currentStateString: null,
>+  
>+  /**
>+   * Initialize the module.
>+   */
>+  init: function()
>+  {
>+    // only read the session file at first window load, i.e. on app startup
>+    if (sessionStoreManager._startup) {

Is there any reason you're not using "this._startup"?  It could make a couple of your lines below much shorter, such as:
>+      this._timer.initWithCallback(this._timerCallback,
>+                                   this._timerInterval,
>+                                   Ci.nsITimer.TYPE_REPEATING_SLACK);
See?

>+      // get file references
>+      let sessionFile = Cc["@mozilla.org/file/directory_service;1"].
>+                        getService(Ci.nsIProperties).
>+                        get("ProfD", Ci.nsIFile);
>+      sessionFile.append("session.json");

I notice that you use this code a bunch in the tests, too.  Maybe it wants to live in its own method?

>+    if (!sessionStoreManager._timerInitialized) {
>+      // initialize the timer
>+      sessionStoreManager._timer.initWithCallback(
>+                                            sessionStoreManager._timerCallback,
>+                                            sessionStoreManager._timerInterval,
>+                                            Ci.nsITimer.TYPE_REPEATING_SLACK);
>+      sessionStoreManager._timerInitialized = true;
>+    }

So, why isn't this block within the "if (sessionStoreManager._startup)" block?
And doesn't it make more sense to set _timer to null if it's not initialized, instead of having another variable?

>+  /**
>+   * Deinitialize the module.
>+   */
>+  deinit: function()
>+  {
>+    // cancel the timer
>+    sessionStoreManager._timer.cancel();
>+    sessionStoreManager._timerInitialized = false;
>+  },
>+
>+/* ........ Timer ................*/
>+  
>+  _timerInterval: 5000, // 5 seconds ought to be plenty
>+  get _timer()
>+  {
>+    delete this._timer;
>+    return this._timer = Cc["@mozilla.org/timer;1"].
>+                         createInstance(Ci.nsITimer);
>+  },

This seems a little odd to me.  Why are we overriding the get method on _timer?  And will this get triggered in the deinit function above?

>+  /**
>+   * Write a state object to disk.
>+   */
>+  _saveStateObject: function(aStateObj)
>+  {
>+    let data = JSON.stringify(aStateObj);
>+    if (!data)
>+      return;

I wonder if it's worth returning some sort of status/error, in case there's a problem saving the state that the user should be warned about.

>+    let foStream = Cc["@mozilla.org/network/file-output-stream;1"].
>+                   createInstance(Ci.nsIFileOutputStream);
>+    foStream.init(sessionFile, 0x02 | 0x08 | 0x20, 0666, 0);

"0x02 | 0x08 | 0x20" should be the default, so you could pass "-1" here.  Similarly, the default permissions are 0664, which seem nicer than 0666, so you should be able to pass -1 for that parameter as well.

>+  startup: function()
>+  {
>+    return sessionStoreManager._startup;
>+  },

Does this really need to be a function, or could we just give people access to the attribute directly?  (Or, just a getter, like the function below.

>+  get state()
>+  {
>+    if (sessionStoreManager._initialState
>+          && sessionStoreManager._initialState.windows) {
>+      let state = sessionStoreManager._initialState.windows.pop();
>+      if (0 == sessionStoreManager._initialState.windows.length)
>+        sessionStoreManager._initialState = null; // XXX need delete?
>+           
>+      return state;

You seem to be assuming that the first window will always be the one you want to return.  Is that a safe assumption?

>+  doRestore: function()
>+  {
>+    return (null != sessionStoreManager._initialState);
>+  },

I think this could be a getter instead.

>+  /**
>+   * Save the current session state.
>+   */
>+  saveState: function(aWindow, aShutdown)
>+  {
>+    let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                         getService(Ci.nsIWindowMediator);
>+    
>+    let state = {
>+      rev: 0,
>+      state: aShutdown ? "stopped" : STATE_RUNNING_STR,

I think that if STATE_RUNNING_STR gets a constant, "stopped" should also get one.  And maybe we could just name it "STATE_RUNNING"?

>+++ b/mail/test/mozmill/session-store/test-session-store.js
>@@ -0,0 +1,195 @@
>+//Cu.import("resource://app/modules/sessionStoreManager.js");

I'm a little surprised that the tests run without loading in the sessionStoreManager code.

>+function test_simple_periodic_session_persistence() {
>+  // delete the session file if it exists
>+  let sessionFile = getSessionFile();
>+  if (sessionFile.exists())
>+    sessionFile.remove(false);

You seem to be doing this at the start of every test.  Is there some way you could clean the file up at the end of each test, so that you start from a known state?

>+  jumlib.assert(!sessionFile.exists(), "file should not exist");

I wonder if this will sometimes fail, if you happen to hit it at just the wrong time?  Perhaps we should add "pause" and "unpause" methods to the sessionStoreManager to make it more deterministic.

>+  controller.sleep(5000); // XXX use an observer instead

I think you should put these comments on the line above, instead of on the same line.  That would also let you change the lines:
>+  let loadedState = JSON.parse(data).
>+                    windows[0]; // XXX check JSON.parse returns valid object?
to:
>+  // XXX check JSON.parse returns valid object?
>+  let loadedState = JSON.parse(data).windows[0];
which seems nicer to me.

>+function test_single_3pane_periodic_session_persistence() {
[snip…]
>+  // parse into state object
>+  let loadedState = JSON.parse(data).
>+                    windows[0]; // XXX check JSON.parse returns valid object?

I think comparing the string equivalents will make sure it returns a valid object.

>+  // XXX there must be a better way to compare the state objects than
>+  // comparing their string representations.

I dunno.  That seems like a fairly good way to compare them to me.

>+function test_multiple_3pane_periodic_session_persistence() {
[snip…]
>+// XXX todo
>+// - clean shutdown test: this test should ensure that only the state of the
>+//                        last closed window is persisted. I removed this test
>+//                        case because closing all 3pane windows causes
>+//                        teardownModule to fail.
>+// - crash test: not sure if this test should be here. restoring a crashed
>+//               session depends on periodically saved session data (there is
>+//               already a test for this). session restoration tests do not
>+//               belong here. see test-message-pane-visibility. 

I'ld also like to see some tests of what happens when things go wrong.
Like, say, a test for when there's an invalid session.json file, which could also be a test of the recoverLastSession method.

But this is definitely a good start, and I look forward to reviewing the next version of this patch.

Thanks,
Blake.

Comment 22

[Wei Xian Woo]

Attachment: Revision of first patch

| Is there any reason you're not using "this._startup"?

I wanted to keep things consistent. Since appIdleManager.js does not use "this." for the same thing and it lives in the same folder as sessionStoreManager.js, I thought I shouldn't either.

| >+    if (!sessionStoreManager._timerInitialized) {
| >+      // initialize the timer
| >+      sessionStoreManager._timer.initWithCallback(
| >+                                            sessionStoreManager._timerCallback,
| >+                                            sessionStoreManager._timerInterval,
| >+                                            Ci.nsITimer.TYPE_REPEATING_SLACK);
| >+      sessionStoreManager._timerInitialized = true;
| >+    }
|
| So, why isn't this block within the "if (sessionStoreManager._startup)" block?

Because the |init| function is called on every 3-pane window creation but we only want to load the session store file when the very first 3-pane window is created. The timer is only initialized when there is at least one 3-pane window. Assuming all 3-pane windows are closed, but Thunderbird is still running because some other type of window is opened then the timer shouldn't be there because we don't currently care about other types of windows. This takes into account the possibility of relaunching a 3-pane window afterwards that is the only 3-pane window in Thunderbird at the time, but not the very first one in the session.

| >+    let foStream = Cc["@mozilla.org/network/file-output-stream;1"].
| >+                   createInstance(Ci.nsIFileOutputStream);
| >+    foStream.init(sessionFile, 0x02 | 0x08 | 0x20, 0666, 0);
|
| "0x02 | 0x08 | 0x20" should be the default, so you could pass "-1" here. 
| Similarly, the default permissions are 0664, which seem nicer than 0666, so you
| should be able to pass -1 for that parameter as well.

The original code that wrote the session file in msgMail3PaneWindow.js used these values, so I kept them.

| You seem to be assuming that the first window will always be the one you want
| to return.  Is that a safe assumption?

Right now, yes. When we start supporting other types of windows, we will also need to know the type of window in order to decide what to return.

| I wonder if this will sometimes fail, if you happen to hit it at just the wrong
| time?  Perhaps we should add "pause" and "unpause" methods to the
| sessionStoreManager to make it more deterministic.

There is a possibility that that might happen. I have taken care of that now.

| I'ld also like to see some tests of what happens when things go wrong.| 
| Like, say, a test for when there's an invalid session.json file, which could
| also be a test of the recoverLastSession method.

Right now, my tests are for session storage only because a recovery test for the 3-pane window is already implemented in test-message-pane-visibility. I'm not sure if recovery tests should be part of the sessionStoreManager's tests or should they be implemented as part of each window type's tests?

For the other questions, I agree with you and have made the appropriate changes.

Thanks,

Wei

Comment 23

[Wei Xian Woo]

Attachment: Revision of first patch (with minor corrections to braces for getters)

Sorry, minor correction to braces used for getters in sessionStoreManager.js.

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 429930 [details] [diff] [review]
Revision of first patch (with minor corrections to braces for getters)

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -531,26 +524,27 @@
>+  let startup = sessionStoreManager.startup;

Since we only use this once, we should just inline the call, and make it:
    if (sessionStoreManager.startup && sessionStoreManager.lastSessionCrashed)
      sessionStoreManager.recoverLastSession(window);

>+  let state = sessionStoreManager.state;
>+  if (state) {
>+    let tabsState = state.tabs;
>+    let tabmail = document.getElementById('tabmail');

We should use "s instead of 's here.

>+++ b/mail/base/modules/sessionStoreManager.js
>@@ -0,0 +1,285 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.

I've been told that this should _actually_ be "The Mozilla Foundation."

>+ * Portions created by the Initial Developer are Copyright (C) 2009

And this should probably be 2010.

>+    // XXX ideally, we shouldn't be writing to disk on the UI thread,
>+    // but the session file should be small so it might not be too big a
>+    // problem.
>+    let foStream = Cc["@mozilla.org/network/file-output-stream;1"].
>+                   createInstance(Ci.nsIFileOutputStream);

I think we put the . beside the "createInstance".  (And there are a couple
more places down below where we need the same change.)

>+/* ........ Public API ................*/
>+
>+  /**
>+   * Determine whether the app is starting up.
>+   */
>+  get startup()
>+  {
>+    return this._startup;
>+  },

So, why not just have a "startup" attribute that people can read, instead
of a getter?

>+  get lastSessionCrashed()
>+  {  

Trailing spaces here that should be removed.

>+    return (this._initialState
>+              && STATE_RUNNING == this._initialState.state);

I think we prefer to write this as:
    return (this._initialState &&
            STATE_RUNNING == this._initialState.state);

>+    if (!this._initialState
>+          || !this._initialState.windows
>+          || !this.lastSessionCrashed)

I think we usually write this as:
    if (!this._initialState ||
        !this._initialState.windows ||
        !this.lastSessionCrashed)

>+++ b/mail/test/mozmill/session-store/test-session-store.js
>@@ -0,0 +1,226 @@
>+var MODULE_NAME = 'test-session-store';
>+var RELATIVE_ROOT = '../shared-modules';
>+var MODULE_REQUIRES = ['folder-display-helpers', 'window-helpers'];

I think we should be using "s instead of 's here.

>+function readFile() {
>+  try {
>+    let data = IOUtils.loadFileToString(sessionStoreManager.sessionFile);
>+    if (data)
>+      return JSON.parse(data);
>+  }
>+  catch (ex) {
>+  }

Uh, really?  This sounds like we might want to report it as a test failure.

Other than that, I'm happy with it, so now you'll want to make those
changes, and post a new patch, asking someone else for a review.  asuth or
bienvenu might be good choices.

Later,
Blake.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #24)
> Other than that, I'm happy with it, so now you'll want to make those
> changes, and post a new patch, asking someone else for a review.  asuth or
> bienvenu might be good choices.

me, please.

Comment 26

[Wei Xian Woo]

(In reply to comment #24)
> Since we only use this once, we should just inline the call, and make it:
>     if (sessionStoreManager.startup && sessionStoreManager.lastSessionCrashed)
>       sessionStoreManager.recoverLastSession(window);

The reason for saving the value of sessionStoreManager.startup into a local variable is because we need to know the startup state before the call to sessionStoreManager.init() but only act on it after. sessionStoreManager.init() will change the startup state when its |init| function is called by the very first window in Thunderbird.

> So, why not just have a "startup" attribute that people can read, instead
> of a getter?

Is there a way to make an attribute read-only from the outside, but still writable within the module?

> >+function readFile() {
> >+  try {
> >+    let data = IOUtils.loadFileToString(sessionStoreManager.sessionFile);
> >+    if (data)
> >+      return JSON.parse(data);
> >+  }
> >+  catch (ex) {
> >+  }
> 
> Uh, really?  This sounds like we might want to report it as a test failure.

We return null when we can't parse the contents of the file into a valid JSON object. The current tests do assert that the return value of this function is non-null before proceeding, so we would still get a test failure if the session file is bad.

Comment 27

[Wei Xian Woo]

Attachment: Second revision of initial patch for session storage & restoration

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 432338 [details] [diff] [review]
Second revision of initial patch for session storage & restoration

Comments exported from the reviewboard review found here:
http://reviews.visophyte.org/r/432338/
(The comments are presented in a prettier context there and can even be used from emacs for addressing them if you are so inclined: http://www.visophyte.org/blog/2010/03/09/emacs-compilation-mode-support-for-reviewboard/ )

I suspect this isn't going to do the right thing for a session with multiple
windows where the user quits via the file menu (or any mechanism of quitting
other than closing all the windows).  Namely, I would expect that as each
window gets closed by the quit process we would persist the state, and so the
last window automatically closed would be the only one persisted.

You could deal with this by listening for the "quit-application" observer
event.  If you hear that and there are any windows currently open, you would
persist the state at that moment and then avoid persisting state when the
windows tell you they are unloading.  It is important to only persist state if
there are windows open because that lets you know the user quit via the menu. 
If the program is quitting because all the windows got closed, then
quit-application will still be generated but no windows will be open anymore
(and the hook on window unload would have already accomplished what we
wanted).


on file: mail/base/content/msgMail3PaneWindow.js line 475
>   //   is open, do not persist tab state.
>   // XXX do not assume there is only ever one 3-pane.
>   if (!FindOther3PaneWindow())
>     persistTabState();

This seems like logic that your patch should modify, namely... (see next
comment)


on file: mail/base/content/msgMail3PaneWindow.js line 480
>   sessionStoreManager.deinit();

I think deinit should be renamed "unloadingWindow" and take an argument of the
window type being unloaded.  Additionally, unloadingWindow should decide when
to persist the file as a perceived shutdown.

This is more appropriate because you are taking action based on the window
going away and most certainly not deinitializing the session manager.  (And if
you were, that would raise multiplicity questions.)


on file: mail/base/content/msgMail3PaneWindow.js line 502
> function persistTabState()
> {
>   sessionStoreManager.saveState(window, true);
> }

(This method would then go away because the session store manager is in charge
of when to do this.)


on file: mail/base/content/msgMail3PaneWindow.js line 507
> /**
>  * Returns the state of this 3-pane window.
>  */
> function getState()

I am assuming you are exposing this via a function rather than having the
session store manager directly reach in and grab the tabmail instance so that
the mechanism is extensible and other window types can use it.

In that case, I would suggest making the function name more uniquely
identifying and self descriptive.  For example
"getWindowStateForSessionPersistence" is unlikely to be anything but what it
says it is.

A good comment for that might mention how it is called by the session store
manager and that it needs to return a JSON-serializable object representation
(although the latter may be implied by the former).


on file: mail/base/content/msgMail3PaneWindow.js line 526
> function atStartupRestoreTabs(aDontRestoreFirstTab) {
>   // determine whether the app is just starting up.
>   // XXX need to do this here because startup state will change if this is the
>   // first window to call |init|.
>   let startup = sessionStoreManager.startup;
> 
>   // initialize the session store module (in case it's not already running)
>   sessionStoreManager.init();
> 
>   let state = sessionStoreManager.state;
>   if (state) {

The control flow and interaction of this function with the session store
manager are somewhat awkward.

I would propose the following, along the lines of my suggestion to change the
signature of deinit:

Have the method call sessionStoreManager.loadingWindow(window) and have that
method return the state (if any) that the window should restore.

This call would implicitly initialize the session store manager if it was not
already initialized.  If it was not initialized (and therefore this was the
startup window), it would trigger the opening of the other required windows as
well.  If the session store manager already was initialized, you can do the
logic where you pull the window states off the list of window states and
return that.

This should greatly simplify the method's logic.

Also, please document the return value in the documentation block for the
method.  (I realize it came that way, but it's often up to the next programmer
to fix the mistakes of the previous programmer :)


on file: mail/base/modules/sessionStoreManager.js line 59
>   _startup: true,

_initialized would be a more traditional name for such a variable, especially
one without a comment explaining what it means.  (Of course, then it should
start out false.)


on file: mail/base/modules/sessionStoreManager.js line 61
>   // the state to restore at startup
>   _initialState: null,

This needs more explanation, primarily because the name of the variable
suggests something unintuitive is going on that merits explanation.  (Namely,
why would initial state need to be persisted to a field when it presumably
could be done in a single function or a closed-over set of functions?)

Most specifically:
- How does the state get into this variable?
- How is the state represented?
- Does it ever get cleared?

Also, please use doxygen/javadoc/JSDoc toolkit style documentation blocks. 
(/** */, like you use elsewhere.)


on file: mail/base/modules/sessionStoreManager.js line 64
>   // the current state
>   _currentStateString: null,

If the comment has less information than the variable name, then you probably
should just delete the comment.

However, in this case, we really need to know what the variable is up to.  As
a developer, I would be interested to know that the string contains the JSON
stringified representation of the last state we wrote to disk so we can easily
check whether the state has changed and generally avoid holding onto object
references that might otherwise cause garbage collection problems.


on file: mail/base/modules/sessionStoreManager.js line 67
>   // the timer
>   _timer: null,
> 
>   // the timer interval
>   _timerInterval: 5000, // 5 seconds ought to be plenty

These comments are also not particularly helpful.  I would rename the
variables to be more specific and lose the comments.  _sessionAutoSaveTimer
and _sessionAutoSaveTimerIntervalMS seem like good names to me.  (Note the
addition of the units into the method name; this is a good idea because people
frequently get this wrong and it makes it more obvious to reviewers and others
when they do.)


on file: mail/base/modules/sessionStoreManager.js line 71
>   _timerInterval: 5000, // 5 seconds ought to be plenty

5 seconds is way too frequent.

I would suggest creating a constant in this file called
SESSION_AUTO_SAVE_DEFAULT_MS and referencing that value here.  I would add a
doxygen-style comment on that value that explains the current choice of value.

I would suggest making the value 5 minutes with a justification of "asuth
arbitrarily chose this value to trade-off powersaving, processor usage, and
recency of state in the face of the impossibility of our crashing; he also
worded this."  (Lest someone think that you are claiming it is impossible that
we crash.)

Note that I would not be opposed to certain significant changes in Thunderbird
state triggering persistence in an event-driven fashion, but using a 5-second
interval to attempt to approximate that event-driven implementation does not
work.  (Our simple state string comparison could result in our state changing
every 5 seconds under active usage, which is silly.)


on file: mail/base/modules/sessionStoreManager.js line 73
>   // the number of 3-pane windows tracked

(please change the comment style to /** comment */)


on file: mail/base/modules/sessionStoreManager.js line 79
>   init: function()

For this and all other functions in this file, please use the named format
syntax to aid in debugging.   This looks like:
  init: function sessionStoreManager_init()

SpiderMonkey is not clever enough to work this out for itself, and most
debugging stuff otherwise ends up thinking all of your functions are
anonymous.  The line numbers are still useful, but it's nice to be able to
avoid that.


on file: mail/base/modules/sessionStoreManager.js line 129
>    * Write a state object to disk.

I would change "a" to "the" here since "a" would suggest you might call this
method with multiple different state objects, but since _currentStateString is
accessed, that is not the case.


on file: mail/base/modules/sessionStoreManager.js line 134
>     if (!data)
>       return;

I don't see how this would happen; you can lose this.


on file: mail/base/modules/sessionStoreManager.js line 166
>   get state()
>   {
>     if (this._initialState && this._initialState.windows)
>       return this._initialState.windows.pop();

Per my previous request, this should go away, but I think it's important to
say that a getter should never have side-effects like this.  Use a method in
such cases.


on file: mail/base/modules/sessionStoreManager.js line 198
>   get periodicSaveInterval()

Given that this is just surfaced for the benefit of the unit test, I would
lose the getter.  Either rename the variable to seem more public or have the
unit test access it even though it looks private-ish.


on file: mail/base/modules/sessionStoreManager.js line 246
>     if (aWindow && aWindow.getState)
>       state.windows.push(aWindow.getState());
> 
>     // XXX we'd like to support other window types in future, but for now
>     // only get the 3pane windows
>     let enumerator = windowMediator.getEnumerator("mail:3pane");
>     while (enumerator.hasMoreElements()) {
>       let win = enumerator.getNext();
>       if (aWindow != win) {
>         if (win && win.getState)
>           state.windows.push(win.getState());

I suggest losing the first bit here where you add the window so you can avoid
having to filter it out in the loop.  If you are trying to control the
ordering in the file for some reason, you should add a comment to that effect
(and in which case you'd probably want to use the z-order anyways).  (Note:
the z-order stuff might be broken on windows, so probably don't try that; I'm
saying that based on my memory of something sid0 wrote; that might no longer
be the case.)


on file: mail/test/mozmill/session-store/test-session-store.js line 79
> function waitForFileRefresh() {
>   controller.sleep(sessionStoreManager.periodicSaveInterval);
>   jumlib.assert(sessionStoreManager.sessionFile.exists(),
>                 "file should exist");
> }

The unit test should clobber the interval used by the timer so that this ends
up being as close to instantaneous as possible.  We don't need a unit test to
sleep 5 seconds when it can sleep 0 or 10 milliseconds.


on file: mail/test/mozmill/session-store/test-session-store.js line 182
>                              "chrome://messenger/content/", "",

why isn't messenger.xul specified here?  is there some magic?  if so, a
comment is needed.

Comment 29

[Wei Xian Woo]

Attachment: Another revision of the session storage & restoration patch

Thanks, Andrew. I have made the required changes in this revision of the patch.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

Attachment: de-bitrotted attachment 435394 "Another revision of the session storage & restoration patch"

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 435426 [details] [diff] [review]
de-bitrotted attachment 435394 [details] [diff] [review] "Another revision of the session storage & restoration patch"

Annotated review exported from:
http://reviews.visophyte.org/r/435426/

This is looking great and pretty much there!  Just a few comment nits and some testing related things and it'll be good to go.

on file: mail/base/modules/sessionStoreManager.js line 101
>         // parse the session state into JS objects
>         this._initialState = JSON.parse(data);

JSON.parse can throw a SyntaxError; this should go in a try/catch block.


on file: mail/base/modules/sessionStoreManager.js line 106
>     // XXX we listen for "quit-application-granted" instead of
>     // "quit-application-requested" because other observers of the
>     // latter can cancel the shutdown.

This comment does not need an XXX.  We use XXX as a marker that there is a
known deficiency/limitation in the code that it might be a good thing to
resolve.  The fact that I erroneously suggested using
quit-application-requested instead of quit-application-granted is a limitation
with me, not the code ;)


on file: mail/base/modules/sessionStoreManager.js line 185
>       if (win && win.getWindowStateForSessionPersistence)

You probably also want to skip windows whose document.readyState is not
"complete".  I saw a lot of errors spit out by the mozmill tests where
_saveState kept trying to persist the state of the window as it loaded and
threw exceptions because the tabmail DOM node did not yet exist.  (I've made
comments on the tests about reducing the time window during which the timer is
active.)

https://developer.mozilla.org/en/DOM/document.readyState


on file: mail/base/modules/sessionStoreManager.js line 211
>         // XXX this is to ensure we don't clobber the saved state when the
>         // 3pane windows unload.

XXX not required here either.


on file: mail/base/modules/sessionStoreManager.js line 229
>     let firstWindow = !this._initialized;
>     if (firstWindow)
>       this._init();

So, mail/test/mozmill/folder-display/test-quick-search.js's
test_search_mode_persistence_subject_to_cc_filter test actually is breaking
because this code is not capable of coming back from a _shutdownStateSaved
situation.  The test closes the last 3pane (while stopping the app from
quitting by having an open window of a different type), and then opens a new 3
pane back up.

I'm torn as to whether it's worth the complexity to the code here to handle
that testing situation here... maybe you could just make that test case poke
the session manager to make it think it's in a fresh state?


on file: mail/base/modules/sessionStoreManager.js line 265
>         // XXX last chance to save any state for the current session since

XXX not required.


on file: mail/test/mozmill/session-store/test-session-store.js line 99
>   // clobber the default interval used by the session autosave timer so the
>   // unit tests end up being as close to instantaneous as possible 
>   sessionStoreManager._sessionAutoSaveTimerIntervalMS = 10;

we probably want to set this back to its default value in a teardownModule
function to avoid generating undesired load if there are any other tests run
in the same mozmill session (or if someone runs this interactively!).


on file: mail/test/mozmill/session-store/test-session-store.js line 105
>   // temporarily stop periodic session persistence
>   sessionStoreManager.stopPeriodicSave();

Along the same lines I would disable the timer in the setup Module and only
enable it within tests as needed and then make sure it is disabled in each
test, even perhaps via a teardownTest method too.


on file: mail/test/mozmill/session-store/test-session-store.js line 127
> function test_bad_session_file_simple() {

I don't understand the point of this test as implemented.  It seems like a
test of the readFile function in this file, not of the session manager.

I agree that having a test that verifies that the session manager does not
explode when it encounters a gibberish session store file (and break the rest
of the UI) is useful.  Perhaps move the code in the manager that loads the
file into its own function and call that from the test and make sure that:
1) There is no gibberish state assigned to the state field.
2) The bad file did in fact get deleted.


on file: mail/test/mozmill/session-store/test-session-store.js line 223
> // XXX todo

I think it would make sense to add a test that verifies that the "do not write
the state if it hasn't changed" logic actually works.  This would take the
form of deleting the file after a waitForFileRefresh() then waiting for
another write interval and making sure the file didn't come back.


on file: mail/test/mozmill/session-store/test-session-store.js line 224
> // - clean shutdown test: this test should ensure that only the state of the
> //                        last closed window is persisted. I removed this test
> //                        case because closing all 3pane windows causes
> //                        teardownModule to fail.

The key thing is to avoid having no top-level windows.  Having no top-level
windows causes the appshell to trigger shutdown.  You could open a window
that's not a 3-pane to test this if you wanted (and then close all the
3-panes).

Mozmill also supports some kind of restart testing if we wanted to go all
crazy thorough.  I don't think we need to do that at this point.


on file: mail/test/mozmill/session-store/test-session-store.js line 228
> // - crash test: not sure if this test should be here. restoring a crashed
> //               session depends on periodically saved session data (there is
> //               already a test for this). session restoration tests do not
> //               belong here. see test-message-pane-visibility.
> //               when testing restoration in test-message-pane-visibility, also
> //               include test of bad session file.

The fact that some session store related tests exists elsewhere seems like it
should go up top in the doc block there.

I don't have a preference about moving them; that seems like the kind of thing
we can defer until we implement persistence for non-3pane windows and it
becomes more of an issue.

Comment 32

[Wei Xian Woo]

Attachment: Minor changes to session store manager and unit tests per asuth's last review. http://hg.mozilla.org/comm-central/rev/3f17f9b73878

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 435817 [details] [diff] [review]
Minor changes to session store manager and unit tests per asuth's last review. http://hg.mozilla.org/comm-central/rev/3f17f9b73878

Looks great.  Thanks for going the extra mile and adding an extra test I didn't even ask for!

I made the very minor change of moving the timer enables in the unit tests to be right before the wait logic, otherwise landed as-is apart for the minor bit-rot I fixed last time around too.  (Your comm-central checkout hasn't been updated in the past few days, it would appear.)

pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3f17f9b73878

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

Marking fixed.  Known limitations still include only persisting 3-pane windows and not persisting window positions on a per-window basis.  If someone files bugs for those (separate bugs, please), please do mention them here.

Comment 35

[Andrew Sutherland [:asuth] (he/him)]

The test-quick-search.js mozmill failure I thought the patch fixed seems to still be happening on the tinderboxen.  I'm going to take a look.

Comment 36

[Andrew Sutherland [:asuth] (he/him)]

bustage-fix pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/76ed45533954

Comment 39

[Justin Bell]

Is this fixed in the shipping version? I just encountered the problem of TB forgetting all my open tabs again due to a power failure reboot of the machine.

Comment 40

[Blake Winton (:bwinton) (:☕️)]

It looks like it's fixed in Thunderbird 3.1.  If that's the version you're running, please open a new bug with the details of the problem you saw, and include your operating system.

Thanks,
Blake.

Comment 41

[Justin Bell]

This is still happening in the latest shipping build
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.8.1.23) Gecko/20090812 Lightning/0.9 Thunderbird/2.0.0.23 Mnenhy/0.7.5.0

Comment 42

[Justin Bell]

This error is still happening, happened last night with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.8.1.23) Gecko/20090812 Lightning/0.9 Thunderbird/2.0.0.23 Mnenhy/0.7.5.0

Comment 43

[Wayne Mery (:wsmwk)]

(In reply to comment #42)
> This error is still happening, happened last night with Mozilla/5.0
> (Windows; U; Windows NT 6.1; en-US; rv:1.8.1.23) Gecko/20090812
> Lightning/0.9 Thunderbird/2.0.0.23 Mnenhy/0.7.5.0

Justin, 

It's time to stop your comments in this bug, per instructions in comment 40. 

You have already filed bug 621908.  And if you have other problems then please file a new bug report if the issue isn't already known in one of the bugs in this query https://bugzilla.mozilla.org/buglist.cgi?negate0=1&list_id=558020&short_desc=restor&field0-0-0=bug_id&resolution=---&query_format=advanced&short_desc_type=allwordssubstr&type0-0-0=anywordssubstr&value0-0-0=322324%20510366%20529763%20296935%20536041%20504989%20598625%2093923%20612500%20275785%20&product=MailNews%20Core&product=Thunderbird

Comment 44

[Steve McMillen]

Is this really fixed?  Running Thunderbird 10.0.2 I shutdown and restart and none of my open windows are re-opened - The tabs do restore. I even get nagged about every saved windows and most annoyingly for every unsaved window the shutdown process is stopped.

I was reading thru the thread but its quite long. The bug starts out suggesting restoring of open windows (compose and reader) but leaves it vague as if this is part of this issue.  

IMO, a full restore is a very important feature.  Not having this means you have to leave your mail client always open as long as you have just one message open that you are still composing.  I think this is a common scenario amongst busy people.  I could save to drafts but its much easier just leaving the windows open.  I may be an anomaly but there are plenty of days when I have 10 or so messages across different desktops in compose state and if my machine starts to get wonky then I have to live with it until I feel I am in a state where the most important messages are addressed and I can safely restart.

You will find that in the Apple Mail client it restores ALL open windows.  It’s a great feature.  I never have to worry about open messages.  Behavior should be:
- when you quit, no prompts to save - just exit!
- when you open, just restore all open windows as they were (3Pane, compose and reader)

For this reason and the search abilities I'd use Apple Mail if it were not for Apple's poor support of an open (html) message format.

I searched both bugzilla and google and to my surprise much focus on restoring tabs but nothing on restoring the full session.  If anyone does read this, please advise and I'll be happy to open a new ticket or however this request might be best submitted.

p.s. I think Thunderbird is behind on this feature.  Outlook 2011 and Apple Mail client have restore functionality (though at varying levels).

Comment 45

[Dave Miller [:justdave]]

As the person who originally filed this bug, I have to agree that it is not yet fixed.

Two of the items I originally filed it for (the two most important ones for me, in fact) were open compose windows and the blue highlighting on folders with unseen new messages.  For whatever else was done on this bug, those two items still don't restore when you quit and restart the app, which means this isn't fixed yet.

Comment 46

[Dave Miller [:justdave]]

*sigh* per comment 34 the developer fixing this stated it was not completely fixed and requested new individual bugs to be filed for the remaining specific pieces.  But did not completely enumerate all of those pieces nor file said bugs for the ones he did enumerate (and it appears no one else did either).

Given those conditions, I will consider this bug resolved when said bugs have been filed.  I'm attempting to come up with a list, and will be back here shortly with lists of either existing bugs or newly-filed ones which cover those pieces.

Comment 47

[Steve McMillen]

Thanks Dave!  For the one about restoring compose windows, it would be great if you can add the bit about closing w/o nagging to save windows if autosave is enabled and has run since the message was last edited.

Comment 48

[Dave Miller [:justdave]]

The five items listed for the session restore request in comment 0:

>1) scroll positions within mailboxes

NOT fixed.  Mailboxes always go back to the top (I have new messages sorted to the bottom) when freshly opened after a restart.

This problem actually appears to be wider-spread than just session restore and appears to periodically get lost during the same session as well.

Bug 487386 - tabs don't maintain exactly what was shown when switching tabs (e.g. reload feed page, loss of scroll position, text selection, focus, attachment pane UI state) [not remember, lose, forget, unwanted display refresh changes, reset]

>2) "unseen" message indicators on folders (as opposed to "unread"). In current
>   incarnations of Thunderbird this is the blue highlight on a folder. It gets lost
>   when you restart Thunderbird, whether you actually looked at the mailbox or not.

NOT fixed. Bug filed.

Bug 737880 - unseen message indicators (blue folder highlight) not preserved during session restore / restart

>3) which mailbox you currently had open

This appears to be fixed.

>4) any compose windows you might have had open at the time

NOT fixed. Bug filed.

Bug 737884 - previously open compose windows not restored during session restore / restart

>5) message windows that were open for reading

NOT fixed. Bug filed.

Bug 737887 - open message windows are not restored during session restore / restart

Comment 49

[insaner]

Just ran into this. There should be a way to pin comment 48 to the top so people won't have to read through all the others to know what the state of the bug(s) is (are).

I have updated Bug #737887 with my comments. It would be great to finally see this fixed.