346301 - Session restore restores windows in the wrong taskbar order

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Adam Spain]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.0.5) Gecko/20060719 Firefox/1.5.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060728 BonEcho/2.0b1

If you have two windows open, when you crash and restore the session, window 2 is restored first, then window one, so in Windows their positions in the taskbar are swapped to what they were before the crash.

Reproducible: Always

Steps to Reproduce:
1.Open two windows with various webpages and tabs open in each.
2.Crash Firefox
3.Start firefox and choose to restore the session when prompted.

Actual Results:  
The positions of the windows in the Windows taskbar are swapped.

Expected Results:  
It should restore windows in the correct order so they appear in the same order in the taskbar as before the crash.

Comment 1

[Simon Bünzli]

This can indeed happen, since the windows are saved in random order - although the most recently active window should always be restored on top. A fix would consist in adjusting the window order as it's returned by the GetZOrderDOMWindowEnumerator (which doesn't work on Linux and was thus originally not used - see bug 156333).

Comment 2

[Jesus Cea]

I find this "random window order" very disturbing and essentially ruins my experience. I have about 50 FF windows open, with several tabs each, and trying to find a particular window after each firefox restart, is an insane waste of time.

I'm sure session restore module could "memorize" window *CREATION* order when saving, instead of current z-order.

If other people value z-order instead creation time order, then, please, create a hide about:config configuration switch.

Please, Please, Please.

Comment 3

[Jesus Cea]

I've patched my firefox installation to avoid window reorder, and so keep window order in "creation time" order. The patch is fairly trivial:

"""
--- ../../firefox-2.0-2006101004/components/nsSessionStore.js   2006-10-10 14:29:19.000000000 +0200
+++ nsSessionStore.js   2006-10-16 11:25:18.000000000 +0200
@@ -1101,10 +1101,10 @@
     this._updateCookies(total);

     // make sure that the current window is restored first
-    var ix = activeWindow ? windows.indexOf(activeWindow.__SSi || "") : -1;
-    if (ix > 0) {
-      total.unshift(total.splice(ix, 1)[0]);
-    }
+    // var ix = activeWindow ? windows.indexOf(activeWindow.__SSi || "") : -1;
+    // if (ix > 0) {
+    //   total.unshift(total.splice(ix, 1)[0]);
+    // }

     // if no browser window remains open, return the state of the last closed window
     if (total.length == 0 && this._lastWindowClosed) {
"""

To do this behaviour switchable via a configuration variable seems fairly simple, actually.

I also opened a mozillazine thread to discuss this topic:
http://forums.mozillazine.org/viewtopic.php?p=2543665

Comment 4

[Mike Connor [:mconnor]]

Not going to block on this, we can partially fix, but doing it by window creation is not at all intuitive, and 50 windows is definitely a corner case

Comment 5

[WulfTheSaxon [:Wulf]]

Tabs are reopened in reverse order as well.

Comment 6

[Roman Bednarek]

(In reply to comment #4)
> Not going to block on this, we can partially fix, but doing it by window
> creation is not at all intuitive, and 50 windows is definitely a corner case
> 
  I agree with Jesus Cea, window creation order is important, I have usually 3 firefox windows open under windows, and every time I start firefox their position on taskbar is different. The same was reported in the original bug report.

Comment 7

[Thomas Bertels]

Attachment: Don't restore the last active window first

As pointed out by Jesus Cea, the patch is trivial.
The question is : (why) does the last active have to be restored first ?
Doesn't it break the user experience ?

Comment 8

[Roman Bednarek]

Maybe recreate windows in the creation order(most important part), and restore also the z-order of windows(not so important to me)? Or at least put in front last active window? 

Comment 9

[Thomas Bertels]

(In reply to comment #8)
> recreate windows in the creation order
This is already the case now, but the last active window is currently restored first (so the order is broken).
The actual behavior looks like a cheap workaround to put in front the last active window.
So, it would be ideal to remember the last active window and then to use focus() to put it in the foreground. But is it possible ?

By the fact, is this Windows specific ?

Comment 10

[Simon Bünzli]

Comment on attachment 280593 [details] [diff] [review]
Don't restore the last active window first

This patch removes a hack without fixing the underlying issue. If you want to fix the problem, please use GetZOrderDOMWindowEnumerator  (see comment #1) and optionally consider doing restoration without the focus calls we currently use.

(In reply to comment #7)
> The question is : (why) does the last active have to be restored first ?

Because that's most likely the point where the user will want to continue at after restoration (same reason we restore the last active tab first).

Comment 11

[Thomas Bertels]

Well, since the windows order is correct, it would be fine if the last active window was focused on restore.

But is there any way to get the handle of the window opened in _openWindowWithState by openWindow, to be able to focus it ?

Comment 12

[Simon Bünzli]

Looks like I misread the issue of this bug.

What you'll have to do is thus remember the currently active window's index instead of shuffling window data around (which attachment 280593 [details] [diff] [review] prevents) and then store a global reference to that window when it's reopened and have all other windows re-focus that window after their window features have been restored (searching for ".focus" will lead you there). In the same time, you could get rid of passing the .opener window around (which so far was the last active window but with the changes outlined will now instead be the chronologically first opened window).

(In reply to comment #11)
> But is there any way to get the handle of the window opened in
> _openWindowWithState by openWindow, to be able to focus it ?

Have you actually had a look at _openWindowWithState? What do you think will "var window" contain? ;-)

Comment 13

[Thomas Bertels]

Attachment: Focus on restore the last active window (needs cleanup)

>What do you think will "var window" contain?
Sure, but I was caught by "aWindow.setTimeout(restoreHistoryFunc, 100, this);" :-(
I intended to focus the last active window only once, and be able to delete this.winHandle, but this setTimeout makes it difficult.

Otherwise, I've discovered an incompatibility with the extension Adblock Filterset.G Updater, which seems to break the order of the windows on restoration.

Comment 14

[Thomas Bertels]

Attachment: Testcase sessionstore.js with windows numbered from 1 to 5

Just copy this file in your Firefox profile then try to start Firefox with/without the extension Adblock Filterset.G Updater.

Comment 15

[Simon Bünzli]

Comment on attachment 280886 [details] [diff] [review]
Focus on restore the last active window (needs cleanup)

That's about the approach I had in mind. Some comments:

AFAICT this patch unnecessarily breaks in the case where the chronologically first opened window was the last active one (i.e. it does nothing when selectedWindow == 0).

>     if (ix > 0) {
>-      total.unshift(total.splice(ix, 1)[0]);
>+      debug("ix : "+ix);
>+      //total.unshift(total.splice(ix, 1)[0]);
>     }

In the end, you'll want to drop this completely.

>+    return { windows: total, selectedWindow: ix };

Please store |ix + 1| as we do for the selected tab, so that |if (data.selectedWindow)| is true if a value was set.

>+    this.saveHandle = false;

This property isn't needed at all. Just return the window handle from _openWindowWithState and ignore it when not further needed. Makes the code easier to read.

>-    if (aOpener) {
>-      aOpener.focus();

Please drop all other references to |aOpener| resp. |.opener| as well.

>+    if(this.winHandle) {
>+      this.winHandle.focus();

"winHandle" isn't as meaningful as it could be. What about "windowToFocus"? Also make sure that you delete this property before you start restoring a session state. And as a bonus you should check onClose whether the closed window is the one you hold a reference to.

(In reply to comment #13)
> I intended to focus the last active window only once

Yeah, we'd need a notification when session restoration has been complete to achieve this, though...

> Otherwise, I've discovered an incompatibility with the extension Adblock
> Filterset.G Updater

That sounds like something you'll have to tell the author(s) of that extension. Should they think that there's a technical limitation in our code, they can always file a new bug, otherwise it's their issue to fix.

Comment 16

[Thomas Bertels]

Attachment: Focus on restore the last active window

Comment 17

[Simon Bünzli]

Comment on attachment 280922 [details] [diff] [review]
Focus on restore the last active window

>+    ix = activeWindow ? windows.indexOf(activeWindow.__SSi || "") : -1;

Now that this line stands alone, please move it right in front of the return statement (except for one empty line), so that it's clearer what it's used for.

>+    var winHandle;

"Handle" sounds quite like a Win32 term. Just call the variable "window" as we do everywhere else (and move the declaration into the loop if it's not used anywhere outside).

>+    if (aState.selectedWindow == 1) {

This will yield a strict warning if .selectedWindow isn't defined. Either do |if (aState.selectedWindow && aState.selectedWindow == 1)| or start with |if (!aState.selectedWindow) aState.selectedWindow = 0;| before you need .selectedWindow the first time.

>+      this.windowToFocus = aWindow;

You currently never delete .windowToFocus which will lead to errors when either the windowToFocus is closed before all windows have been restored or when the SessionStore API is used to restore a session without selectedWindow after a session with selectedWindow was restored.

So please delete it at the start of restoreWindow and also in onClose if .windowToFocus == aWindow.

>-  restoreDimensions_proxy: function sss_restoreDimensions_proxy(aWindow, aOpener, aWidth, aHeight, aLeft, aTop, aSizeMode, aSidebar) {

While you're at it: could you please remove the _proxy bit here? It meant that |this| inside the function wasn't the SessionStore object - which it is now.

>+    if(this.windowToFocus) {

Nit: Space after |if|.

>     }, true);
>+    return window;

Nit: Empty line before return statement (makes it easier to spot).

Comment 18

[Thomas Bertels]

(In reply to comment #17)
> >+      this.windowToFocus = aWindow;
> 
> You currently never delete .windowToFocus which will lead to errors when either
> the windowToFocus is closed before all windows have been restored or when the
> SessionStore API is used to restore a session without selectedWindow after a
> session with selectedWindow was restored.
> 
> So please delete it at the start of restoreWindow and also in onClose if
> .windowToFocus == aWindow.

Well, delete it in onClose is not a problem, but it stops working if it's also done at the start of restoreWindow.

Comment 19

[Simon Bünzli]

(In reply to comment #18)
> it stops working if it's also done at the start of restoreWindow.

Yeah, you'll have to add a fourth parameter to restoreWindow to determine whether a call belongs to an already started restoration or whether it's a new one (and .windowToFocus can/should be reset). As it looks, the parameter is even already correctly passed - it's just not used yet.

Comment 20

[Thomas Bertels]

Attachment: Focus on restore the last active window (2)

>you'll have to add a fourth parameter to restoreWindow to determine
>whether a call belongs to an already started restoration or whether it's a new
>one
I guess you're talking about ._lastSessionCrashed ?

Comment 21

[Simon Bünzli]

Comment on attachment 281006 [details] [diff] [review]
Focus on restore the last active window (2)

Looking good. A few details and this should be ready:

>+    if (this.windowToFocus == aWindow) {

This will yield a strict warning if .windowToFocus isn't defined.

>+  restoreWindow: function sss_restoreWindow(aWindow, aState, aOverwriteTabs, lastSessionCrashed) {

Resetting .windowToFocus doesn't depend on whether the last session crashed but on whether this call to restoreWindow is part of a restoration operation or whether this is the first call to it. The only place where that fourth argument should be set to true is inside _openWindowWithState (in all other places either set it to false or completely omit it).

Nit: Function arguments always start with "a".

>+    for (var w = 1, window; w < root.windows.length; w++) {

Why not just |var window = ...;| where you first need it?

>-    // since resizing/moving a window brings it to the foreground,
>-    // we might want to re-focus the window which created this one

Please keep the bits of this comment which still apply. If this specific issue didn't exist, we could just focus the selected window once in restoreWindow and do without .windowToFocus.

Comment 22

[Thomas Bertels]

Attachment: Focus on restore the last active window (3)

>Why not just |var window = ...;| where you first need it?
Thought it would slow down execution.

Comment 23

[Simon Bünzli]

Comment on attachment 281064 [details] [diff] [review]
Focus on restore the last active window (3)

Two final details and this is good for check-in:

>+  restoreWindow: function sss_restoreWindow(aWindow, aState, aOverwriteTabs, aFirstCall) {

When is aFirstCall set to true? Please rename the parameter appropriately and adjust the comment for restoreWindow with an explication to what it does.

BTW: I've just noted that there are also still comment references to the aOpener arguments which you're removing.

>+    if (!aFirstCall && this.windowToFocus && this.windowToFocus == aWindow) {

Why the third condition? Wouldn't this lead to the following regression: 1. Restore a two window session; 2. use the SessionStore API to restore a window state without .selectedWindow into the window previously _not_ focused; 3. watch an apparently non-related window suddenly pop up (because .windowToFocus == aWindow was false, but .windowToFocus never was overwritten or cleared)?

Comment 24

[Simon Bünzli]

Hm, it might actually be better to always set .windowToFocus to aWindow instead of deleting it so that when .selectedWindow is missing we get the same behavior as with Firefox 2 (i.e. the first restored window is focused).

Comment 25

[Thomas Bertels]

> When is aFirstCall set to true? Please rename the parameter appropriately and
> adjust the comment for restoreWindow with an explication to what it does.

What do you suggest ? aCalledByAPI ? Or aCalledBy_openWindowWithState ? Feel free to tell me if you know what. ;-)

> Why the third condition?

I misunderstood you in comment 17.

Otherwise, I've noticed that selectedWindow is set to 0 (so no focus on restore) when the session state is "stopped". So when we choose to save the session state on exit or when the browser is automatically restarted.
Do you have any idea of why windowMediator.getMostRecentWindow("navigator:browser") is (I guess) null in this case ?

Comment 26

[Simon Bünzli]

(In reply to comment #25)
> What do you suggest ? aCalledByAPI ?

What about "aFollowUp" or "aPartOnly"?

> Do you have any idea of why
> windowMediator.getMostRecentWindow("navigator:browser") is (I guess) null in
> this case ?

Oh, another new bug. That's because at the time of the last state update all windows have already been closed. To work around that issue, we'd have to cache the __SSi of the last active window and fall back to try to using that when no open window was found. That can happen in a follow up bug, though.

Comment 27

[Thomas Bertels]

Attachment: Focus on restore the last active window + save on exit bug workaround

Comment 28

[Simon Bünzli]

Comment on attachment 282117 [details] [diff] [review]
Focus on restore the last active window + save on exit bug workaround

Looking good. One more detail:

>+    if (activeWindow) {
>+      ix = windows.indexOf(activeWindow.__SSi || "");
>+      this.ixCache = ix;
>+    }

|windows| isn't guaranteed to always be in the same order - |ix| has thus to be re-determined whenever _getCurrentState is called. You'll rather have to cache the active window's ID (.__SSi) instead.

>+      aState.selectedWindow = 0

Nit: Missing semicolon.

>+    if (aState.selectedWindow == 1) {
>+      this.windowToFocus = aWindow;
>+    }

Nit: This has become a no-op (.windowToFocus is already aWindow if .selectedWindow < 2) and can be replaced with a comment (or dropped completely).

Comment 29

[Thomas Bertels]

Attachment: Focus on restore the last active window + save on exit bug workaround (2)

Comment 30

[Simon Bünzli]

Comment on attachment 282164 [details] [diff] [review]
Focus on restore the last active window + save on exit bug workaround (2)

>+    if (activeWindow) {
>+      this.activeWindowSSiCache = activeWindow.__SSi;
>+    }
>+    ix = this.activeWindowSSiCache ? windows.indexOf(this.activeWindowSSiCache || "") : -1;

Final nit: The || "" must still go after activeWindow.__SSi (since that property might not be defined - this.activeWindowSSiCache will be defined for .indexOf as you've already checked that it is before).

r+ with that nit fixed (please attach a final patch and then request approval1.9 for it). Thanks for fixing this, Thomas.

Comment 31

[Thomas Bertels]

Attachment: Focus on restore the last active window + save on exit bug workaround (3)

>The || "" must still go after activeWindow.__SSi

I was thinking it was something specific to indexOf. So it's a way to avoid using |if (activeWindow && activeWindow.__SSi)| ?

>Thanks for fixing this, Thomas.

Thank you for your help. :-)

Comment 32

[Reed Loden [:reed]]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.78; previous revision: 1.77
done

Comment 34

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre ID:20090617031528

Comment 35

[Tamar Schiller]

Did I ever say how much I love open source?
You guys rock. Thanks for fixing!

Comment 36

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7790