491590 - browser.sessionstore.max_windows_undo preference not properly obeyed for "Recently Closed Windows" feature

Status: NEW

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

Bug 394759 added a "Recently Closed Windows" menu to the History menu.  The number of closed windows remembered is controlled by the browser.sessionstore.max_windows_undo preference.  

There's two cases where the preference is not being obeyed:

1. If the preference is set to 0, then the "Recently Closed Windows" menu should be disabled.  Because of coded added to handle popups in Windows and Linux, this is not true and setting the preference to 0 is the same as setting it to 1.

2. The preference is completely ignored when the setBrowserState or setWindowState API functions are called with a state containing a closed window list.


The fix for this is fairly trivial:

1. The getClosedWindowCount API function should never return a value greater than browser.sessionstore.max_windows_undo.  So if the preference is 0, it should return 0.  This should not interfere with the popup processing.  The getClosedWindowData and undoCloseWindow API functions should properly also not return or do anything if the browser.sessionstore.max_windows_undo preference is set to 0.

2. In restoreWindow, after setting the _closedWindows, call the _capClosedWindows function.

Comment 1

[Simon Bünzli]

(In reply to comment #0)
> 1. The getClosedWindowCount API function should never return a value greater
> than browser.sessionstore.max_windows_undo.

It's designed to do that in the very specific situation where there's no non-popup browser window open but Firefox hasn't closed down yet. Note that max_windows_undo isn't meant to be a hard limit when popup windows come into play - also documented as such.

> 2. In restoreWindow, after setting the _closedWindows, call the
> _capClosedWindows function.

If we do this, we'll also want to do the same for _closedTabs for consistency.

Comment 2

[Michael Kraft [:morac]]

(In reply to comment #1)
> (In reply to comment #0)
> > 1. The getClosedWindowCount API function should never return a value greater
> > than browser.sessionstore.max_windows_undo.
> 
> It's designed to do that in the very specific situation where there's no
> non-popup browser window open but Firefox hasn't closed down yet. Note that
> max_windows_undo isn't meant to be a hard limit when popup windows come into
> play - also documented as such.
> 

(In reply to comment #1)
> (In reply to comment #0)
> > 1. The getClosedWindowCount API function should never return a value greater
> > than browser.sessionstore.max_windows_undo.
> 
> It's designed to do that in the very specific situation where there's no
> non-popup browser window open but Firefox hasn't closed down yet. Note that
> max_windows_undo isn't meant to be a hard limit when popup windows come into
> play - also documented as such.
> 

I'm not sure it's working correctly then.  For example take the following example:
1. Set browser.sessionstore.max_windows_undo to 0.
2. Open a new browser window using the Filel->New Window command
3. Close the newly opened window

At this point one would expect the Histoty->"Recently Closed Window" menu to be empty, yet it has the window that just closed in it.  Also the API functions behave as if browser.sessionstore.max_windows_undo is set to 1.  So basically setting browser.sessionstore.max_windows_undo to anything less than 1 is treated as if the user set it to 1.

Regardless of the fact that _closedWindows needs to keep the last non-popup window around, if browser.sessionstore.max_windows_undo is 0 that window should not show up in the Histoty->"Recently Closed Window" menu.  The easiest way to make that happen is to have getClosedWindowCount return a 0 in that case.


> > 2. In restoreWindow, after setting the _closedWindows, call the
> > _capClosedWindows function.
> 
> If we do this, we'll also want to do the same for _closedTabs for consistency.
>

You have a point.  Though for tabs, it's less likely to be noticed since when the next tab is closed the closed tab list will correct itself.  The same is true for windows, but I would think windows are closed with a lot less frequency than tabs.  In the end I guess it's up to you.  I was mainly trying to protect against a, albeit unlikely, case where someone could restore a few hundred closed windows even though the limit is set to 3.

Comment 3

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

(In reply to comment #2)
> I'm not sure it's working correctly then.  For example take the following
> example:
> 1. Set browser.sessionstore.max_windows_undo to 0.
> 2. Open a new browser window using the Filel->New Window command
> 3. Close the newly opened window
> 
> At this point one would expect the Histoty->"Recently Closed Window" menu to be
> empty, yet it has the window that just closed in it.  Also the API functions
> behave as if browser.sessionstore.max_windows_undo is set to 1.  So basically
> setting browser.sessionstore.max_windows_undo to anything less than 1 is
> treated as if the user set it to 1.
> 
> Regardless of the fact that _closedWindows needs to keep the last non-popup
> window around, if browser.sessionstore.max_windows_undo is 0 that window should
> not show up in the Histoty->"Recently Closed Window" menu.  The easiest way to
> make that happen is to have getClosedWindowCount return a 0 in that case.

I see where you're coming from here. In fact one of the iterations for the
implementation worked as such. The problem is that we'd be lying if we cut off
at what the pref was set at. Using your example above, somebody (current ui,
extension, etc) could still call sessionstore.undoCloseWindow and have it open
a window because we're keeping track of that window, even though
getClosedWindowCount was returning 0.

Now even if the pref was 3, we had to store 6 windows, but only showed 3 in the
menu (which I think is one solution you're proposing), the user could undo open
one, then expect to only see 2 in the menu. But there are still 3. Repeat.

It's a tough case to get perfectly right, but maybe special casing 0 is the
right way.

> > > 2. In restoreWindow, after setting the _closedWindows, call the
> > > _capClosedWindows function.
> > 
> > If we do this, we'll also want to do the same for _closedTabs for consistency.
> >
> 
> You have a point.  Though for tabs, it's less likely to be noticed since when
> the next tab is closed the closed tab list will correct itself.  The same is
> true for windows, but I would think windows are closed with a lot less
> frequency than tabs.  In the end I guess it's up to you.  I was mainly trying
> to protect against a, albeit unlikely, case where someone could restore a few
> hundred closed windows even though the limit is set to 3.

Like you said, this change would be mostly trivial. Regarding protecting
against the restoring 'a few hundred closed windows' case, it would take a very
deliberate "attack" to make that happen, but it could happen. The most likely
way would be editing sessionstore.js manually.

Comment 4

[Michael Kraft [:morac]]

I'm kind of confused as to why the closed window list was implemented the way it is actually.  Basically the way it is now, it's mainly being used to store the last closed window and associated popups for when the browser shuts down.  It seems like the "closed windows while running" part was kind of tacked on.

It seems to me that there really should be two separate lists, but obviously that's a waste of memory.  Still it seems like there should be a better way of separating the "closed window" list from the "closed windows needed for restoring windows on a restart" list.  

Just by playing with some things I've already triggered a case where windows that were displayed on a restart end up being in the closed windows list after the restart and vice-versa.

An easy way to trigger this is do to the following:
1. Set Firefox to display windows and tabs from last time.
2. Open 2 windows with different page.
3. Open the Error console and type the following on 1 line:
4. var win = Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).undoCloseWindow(0); win.close();
5. Closed the error console and then closed the open browser window.
6. Start Firefox again and you'll see that the closed window and open window have swapped.


I've also triggered cases where closed window were restored when Firefox was restarted using steps similar to the above.  What's really weird is that the sessionstore.js file looks fine after step 4 (even after I force it to update by doing something in the browser.)  After step 5 though it's wrong.  This might be a separate bug, but the trigger is the same.

Couldn't another variable be created that actually keeps track of the "real" closed windows (i.e. windows not associated with shut down)?  This list could just reference the _closedWindows array so that duplicate data wouldn't be needed.

Basically to prevent problems from occurring SessionStore needs to know the difference between windows closed at shutdown and normally run of the mill closed windows.

Comment 5

[Michael Kraft [:morac]]

I decided that the open/close swap should be it's own bug since even if the root cause is the same as this bug, it's possible to fix one and not the other.  See bug 491853.

Comment 6

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

Filed bug 493121 for the pref enforcement in setBrowserState (since that's not really what this bug is about)