524745 - Session restore sets focus to minimized windows

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Jim Mathies [:jimm]]

STR:
1. Have Firefox remember your tabs from last time at startup.
2. Minimize a window.
3. Close the browser without activating the window (right click taskbar entry, close)
4. Start the browser up
5. Window displays, minimizes and then restores.

[depends on the patch in bug 520178]

This is a spin off from bug 520178. On windows, this problem is hidden by another bug where session store didn't know the correct size mode of windows.

Once the patch in 520178 lands, this bug will be exposed.

Stepping through with venkman, this focus call is the cause -

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2440

Comment 1

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

So the actual problem is not the focus call. That's doing exactly what it's supposed to be doing. The problem is that we think the minimized window is the selected window. As a side note, it's not consistent. Still a problem and should definitely block though.

I'll look into it.

Comment 2

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

STR more in terms of sessionstore.
0. Have windows 1 & 2, settings for restore session
1. Make sure window 2 is selectedWindow (do something to trigger a save [eg scroll], wait a few seconds to ensure save)
2. Minimize window 2
3. Quit without doing anything that would trigger a session save with file|exit, or right click, close. Doesn't matter if you activate the window.
4,5 from above

What happens behind the scenes is that we cache the id of the last active window when we save state. We update that whenever we subsequently save state. The active window is gotten through WindowMediator.getMostRecentWindow.

Now when we quit, active window is null because getMostRecentWindow returns null. So we use the cached id, which still points to window 2.

See it all: http://hg.mozilla.org/mozilla-central/file/tip/browser/components/sessionstore/src/nsSessionStore.js#l1691

On the plus side, this isn't a side effect of bug 520178. Firefox 3.5 actually breaks like this too (is that a plus? probably not).

So I'm going to say that while it's certainly not great behavior, it's not a regression. If I were a driver I'd say wanted, but won't block.

As a final note for a potential fix (before I lose the thought). We can listen to the activate event (bug 511503) and then saveStateDelayed if it would invalidate the cached id.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

While not a driver, Paul could play one on TV. Odd, would take a patch, should probably do, doesn't block this late in the game. Adding to the 3.7 cleanup list as well to make sure we don't miss it.

Comment 4

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

This happens on more than just Windows. It's not as noticable on OS X because setting setting the focus doesn't restore the window. However the value of selectedWindow is wrong when we save.

(In reply to comment #2)
> As a final note for a potential fix (before I lose the thought). We can listen
> to the activate event (bug 511503) and then saveStateDelayed if it would
> invalidate the cached id.

This works, but I'm thinking there should be a better way to do it.

Comment 5

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

Attachment: Patch v0.1

(In reply to comment #4)
> This works, but I'm thinking there should be a better way to do it.

So I think the "better way to do it" would be to just set the activeWindowSSiCache instead of doing saveStateDelayed. Backing this up with on the fact that we haven't seen this reported before, it seems to be a rare enough case that it's not worth writing to disk immediately for.

Just updating activeWindowSSiCache is good enough for covering the quit case. The crash case won't be updated, but that might be ok...

Comment 6

[Simon Bünzli]

Comment on attachment 414068 [details] [diff] [review]
Patch v0.1

(In reply to comment #2)
> 2. Minimize window 2
> 3. Quit without doing anything that would trigger a session save with
> file|exit, or right click, close. Doesn't matter if you activate the window.

How will calling saveStateDelayed fix this issue? If we quit before the delay times out, we won't update the selected window (and even if we did, it could still be a minimized one, couldn't it?).

Why not just do the obvious and either make sure that selectedWindow isn't minimized (setting it to 0 if it is), or - when restoring - not focus a minimized window, or even enforce the window to be focused to be either restored or maximized?

Finally, this should be testable, so no r+ without tests.

Comment 7

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

(In reply to comment #6)
> (From update of attachment 414068 [details] [diff] [review])
> (In reply to comment #2)
> > 2. Minimize window 2
> > 3. Quit without doing anything that would trigger a session save with
> > file|exit, or right click, close. Doesn't matter if you activate the window.
> 
> How will calling saveStateDelayed fix this issue? If we quit before the delay
> times out, we won't update the selected window (and even if we did, it could
> still be a minimized one, couldn't it?).

I was thinking saveStateDelayed saved immediately if we're passed a normal save interval, but there's a 2 second delay by default.

So we could use my 2nd idea of just setting the cache id instead (and still call saveStateDelayed?). This covers the quit case properly.

> Why not just do the obvious and either make sure that selectedWindow isn't
> minimized (setting it to 0 if it is), or - when restoring - not focus a
> minimized window, or even enforce the window to be focused to be either
> restored or maximized?

None of these solutions necessarily select the right window. It'll select window 0 (or we'd select the first non-minimized window), which isn't always going to be the window you had focused at quit. The examples above only have two windows, but it happens with 3.

On OSX: have windows 1,2,3. Minimize 2, 3 gets focused automatically. Focus 1. On restore, 3 is focused.

> Finally, this should be testable, so no r+ without tests.

So it's not really testable because it's only when quitting that __getMostRecentBrowserWindow returns null (which is the only time the cached value isn't invalidated), so using ss.getCurrentState wouldn't actually be testing this.

Comment 8

[Simon Bünzli]

(In reply to comment #7)
> I was thinking saveStateDelayed saved immediately if we're passed a normal save
> interval, but there's a 2 second delay by default.

No, there's always a delay (unless you pass it a desired 0 ms delay).

> So we could use my 2nd idea of just setting the cache id instead (and still
> call saveStateDelayed?).

Setting it to what? Couldn't the activated window continue to be minimized?

> None of these solutions necessarily select the right window.

They however make sure not to select the wrong window. Might be good to check besides listening for "activate".

> So it's not really testable because it's only when quitting that
> __getMostRecentBrowserWindow returns null (which is the only time the cached
> value isn't invalidated), so using ss.getCurrentState wouldn't actually be
> testing this.

What happens when you minimize all windows and then call getBrowserState? Wouldn't restoring that state lead to a minimized window being focused?

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > I was thinking saveStateDelayed saved immediately if we're passed a normal save
> > interval, but there's a 2 second delay by default.
> 
> No, there's always a delay (unless you pass it a desired 0 ms delay).

My mistake. Also, even if you pass in 0, it'll get the delay - 0 || 2000 = 2000... I'd have to pass in -1

> > So we could use my 2nd idea of just setting the cache id instead (and still
> > call saveStateDelayed?).
> 
> Setting it to what? Couldn't the activated window continue to be minimized?

In the case where you only have 1 browser window yes, but the activate event should be fired for the window that gets focused after you minimize another.

> > None of these solutions necessarily select the right window.
> 
> They however make sure not to select the wrong window. Might be good to check
> besides listening for "activate".

I guess I was just targeting the case where you aren't quitting with all your windows minimized. I guess we would still want to check to make sure the that "active" window didn't get minimized without activating another. We could do that 2 ways:
(1) listen for deactivate and clear the cached id, don't update the cached id in getCurrentState (that should be enough - we'd save selectedWindow as 0)
(2) check the state of the selected window on quit/startup like you suggest.

> > So it's not really testable because it's only when quitting that
> > __getMostRecentBrowserWindow returns null (which is the only time the cached
> > value isn't invalidated), so using ss.getCurrentState wouldn't actually be
> > testing this.
> 
> What happens when you minimize all windows and then call getBrowserState?
> Wouldn't restoring that state lead to a minimized window being focused?

Yea, so long as we check/don't update the cached id in getCurrentState. We can check for a selectedWindow value > 0. Actually restoring the state & checking that will depend on the platform.

Comment 10

[Simon Bünzli]

(In reply to comment #9)
> (2) check the state of the selected window on quit/startup like you suggest.

This seems like the simpler solution and would thus be my preference.

Comment 11

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

(In reply to comment #10)
> (In reply to comment #9)
> > (2) check the state of the selected window on quit/startup like you suggest.
> 
> This seems like the simpler solution and would thus be my preference.

I still like (1) better because it'll save the right focus as opposed to just clearing focus. I guess it's not that big a deal since it's not likely you'll switch windows and quit without doing something else to trigger a save.

I'll do (2) for now and write the test, but I might revisit this if some other ideas get picked up (eg, if we take prioritization even further).

Comment 12

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

Attachment: Patch v0.2

As simple as it gets. With tests (the important one does fail pre-patch).

Also got rid of the ternary since indexOf would return -1 anyway (though maybe I shouldn't have if indexOf could be costly... but windows is just an array of ids at that point, so it shouldn't be)

Comment 13

[Simon Bünzli]

Comment on attachment 414200 [details] [diff] [review]
Patch v0.2

r+=me and thanks.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 414200 [details] [diff] [review]
Patch v0.2

>+      switch(aTopic) {
>+        case "domwindowopened":
>+          window_B.addEventListener("load", function () {
>+            window_B.removeEventListener("load", arguments.callee, false);

...

>+
>+        case "domwindowclosed":
>+          ww.unregisterNotification(this);
>+          executeSoon(function() {
>+            is(browserWindowsCount(), 1,
>+               "Only one browser window should be open eventually");
>+            finish();

Please don't use the window watcher for this, as it will cause random failures if you get domwindowclosed from a previous test... See bug 518970 comment 163. Listening to the load event right after openDialog() and calling finish() right after close() should be fine.

>+              // Without the executeSoon, there are errors due to closing the window too quickly
>+              executeSoon(function() window_B.close());

Are you sure executeSoon isn't racing with delayedStartup, making the errors go away only occasionally? The errors don't matter for this test anyway (lots of other tests have them as well), and we should probably fix browser.js to prevent them.

Comment 15

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

(In reply to comment #14)
> Please don't use the window watcher for this, as it will cause random failures
> if you get domwindowclosed from a previous test... See bug 518970 comment 163.
> Listening to the load event right after openDialog() and calling finish() right
> after close() should be fine.

Ah, right. I liked openDialog better, but remembered the windowWatcher use cleaned up some previous issues. I'll go back to openDialog.

> Are you sure executeSoon isn't racing with delayedStartup, making the errors go
> away only occasionally? The errors don't matter for this test anyway (lots of
> other tests have them as well), and we should probably fix browser.js to
> prevent them.

It probably is.

Comment 16

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

Attachment: Patch v0.3 (for check-in)

Cleanup the test per comment #14

Comment 17

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/7856366bcd76

Comment 18

[Reed Loden [:reed]]

Comment on attachment 415206 [details] [diff] [review]
Patch v0.3 (for check-in)

Requesting approval1.9.2 for this simple fix to ensure focus is not restored to a minimized window.

Comment 19

[Peter Van der Beken [:peterv]]

I backed this out from trunk, the test time out and turned the tree orange.

Reed: checking in unapproved patches in a restricted tree, even though you were explicitly told it was not ok, is not cool. Walking away before the tree has turned green is not cool either.

Comment 20

[Dão Gottwald [:dao]]

Running chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js...

TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js | Only one browser window should be open initially

TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js | selectedWindow is window_B

TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js | Console message: [JavaScript Error: "selectedWindow is undefined" {file: "chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js" line: 79}]

NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js | Timed out

Comment 21

[Reed Loden [:reed]]

(In reply to comment #19)
> Walking away before the tree has turned green is not cool either.

I didn't walk away purposely or anything like that... Just fell asleep at my laptop while watching the tree (you can see I was tagging things). I've gone back and tagged most of the oranges with various [orange] bugs.

Comment 22

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

Attachment: Patch v0.4 (actually for check-in)

Added a waitForFocus after minimizing the first window. I reproduced the error/timeout from comment #20 on try, but they went away with this.

Comment 23

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

Pushed http://hg.mozilla.org/mozilla-central/rev/2d2c60ed72ce

Comment 24

[u279076]

Verified fixed.