520178 - Minimized windows appear offscreen when restoring from session store

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Rob Arnold [:robarnold]]

STR:
1. Have Firefox remember your tabs from last time at startup.
2. Minimize a window.
3. Close the browser without activating the window.
4. Start the browser up
5. Window appears in alt-tab list and taskbar but is offscreen!

To restore offscreen window:
Alt-<space> -> Move -> hit an arrow key -> move the mouse to move the window -> click when done moving it.

Comment 1

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

For some reason screenX & screenY are each set to -32000. I don't have a Win7 build environment set up to properly debug so it might take some time before I know why. Height & width are very small too (160 & 27).

http://hg.mozilla.org/mozilla-central/file/a9128a07266a/browser/components/sessionstore/src/nsSessionStore.js#l2677 is what we use to get the values. We just get them off the window. So it's likely something is going wrong elsewhere.

We're also still getting window state = normal, which isn't right. It's almost as though Win7 is just moving everything way off screen when you minimize it.

Comment 3

[IU]

1. This regressed with bug 502713.
2. Please set Platform to "All," as XP is also affected

Comment 4

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

--> Core::Widget 32 as per comment 3

Comment 5

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

Blocking, suggest relnote for beta1

Comment 6

[Dimitry Andric]

The (-32000,-32000) coordinates are a rather nasty hack of the Windows
window manager to hide "minimized" windows; these are actually just
normal visible windows, but with their dimensions reduced to a small
title bar, and placed at (-32000,-32000) to get them off the visible
desktop.

See http://blogs.msdn.com/oldnewthing/archive/2004/10/28/249044.aspx
for some explanation behind this.

In short, you need to save the window's coordinates using
GetWindowPlacement(), and restore them using SetWindowPlacement().

Comment 8

[[not reading bugmail]]

I been debating about filing a separate bug for this, but I think I'm seeing this when I install partial nightlies on XP.  Session Restore may or may not be running, but after I do a restart and it installs the update, I have been getting just a taskbar item, but cannot access the window too.  

Though, the other day I tried something different for STR to get my window back.
I chose Restore from taskbar the I chose Maximize and I got the window to come up.  I forget to try it every time I run the partial update, but it worked the one day I did w/o restarting Minefield, then I didn't get the small window on a restart. 

Other times I've been seeing this I was just closing the window from the context menu and restarting Minefield out of frustration, then I have been getting what appears to be the small window size everyone has been seeing; which happens to be the same size almost every time, sort of like the window size just big enough for a Mobile device.  Then I manually resize the window to fill my screen.

Comment 9

[(not currently active) Ted Mielczarek]

This happens on XP as well. Not that we have an "All versions of Windows" field, but this looks confusingly like it's Win7-specific.

Comment 10

[Joe Drew (not getting mail)]

This is a blocker without an owner, so let's say that Jim Mathies will look at it? (hoping)

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #3)
> 1. This regressed with bug 502713.
> 2. Please set Platform to "All," as XP is also affected

Actually the bug was always there, this patch just exposed the problem in a more obvious way. Not sure yet if this is a widget problem or something else.

Comment 12

[Jim Mathies [:jimm]]

Attachment: revamp OnWindowPosChanged v.1

So our size mode was completely hosed. In looking at 1.9.1 source, it looks like the mode was originally being updated inadvertently through some events in OnWindowPosChanged prior to the focus manager changes. Those event were removed causing the size mode to end up in an incorrect state.

This patch does a couple things - first it re-arranges our handling of window position information so that frame change events are processed first, which eliminates the need for the abrupt return statement in the window size handler code. It also now correctly avoids sending an NS_SIZE gecko event when we minimize the window.

I wasn't sure how this would effect wince. Since we've run into so many collisions there I went ahead and split the original code out per platform.

There is however one remaining issue here related to focus that was being hidden by the original bug. With this patch, session restore correctly restores the minimization state shortly after the window becomes visible. However, on a subsequent timer event, the focus manager calls SetFocus on a child with raise=true, so we end up with a situation where the window displays, then minimizes, and then restores again.  I'll attach a stack on that. (Enn, any ideas on why this is happening with the window minimized?)

Comment 13

[Jim Mathies [:jimm]]

Attachment: focus stack

Comment 14

[Neil Deakin]

Looks like a script is calling setTimeout(function() window.focus(), 0)

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #14)
> Looks like a script is calling setTimeout(function() window.focus(), 0)

So maybe this is a bug in session restore. I'll see if I can track down the actual script doing it.

Comment 16

[[not reading bugmail]]

Maybe it has something to do with hidden windows or multiple windows or popup dialogs being open, yet I think Paul fixed the latter from being restored.  I find bug 343871 or bug 220546 or bug 521501 might be of interest here among others I found while searching for session+restore or minimize*+window* bugs.  

We also know there is also a hack somewhere for suppression windows from opening and being restored from the taskbar when loading pages.  It sounds like from all the bugs I've looked at, we have hacks to restore windows from issues being stuck and not restored and hacks to keep windows minimized or closed to not be restored.  

Bunzli might be able to help too.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> We also know there is also a hack somewhere for suppression windows from
> opening and being restored from the taskbar when loading pages.  It sounds like
> from all the bugs I've looked at, we have hacks to restore windows from issues
> being stuck and not restored and hacks to keep windows minimized or closed to
> not be restored.  

bug 509449 addressed a problem with minimized windows restoring during page load, which has landed on trunk.

Comment 18

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

(In reply to comment #15)
> (In reply to comment #14)
> > Looks like a script is calling setTimeout(function() window.focus(), 0)
> 
> So maybe this is a bug in session restore. I'll see if I can track down the
> actual script doing it.

Session Restore isn't calling window.focus() directly anywhere. window.content.focus(), yes. This happens in restoreDimensions which is called from a setTimeout(__, 0). We only focus into the window that was focused at quit though. And focus is called immediately after setting dimensions, not in another setTimeout.

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

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 408608 [details] [diff] [review]
revamp OnWindowPosChanged v.1

Thanks Neil!

Comment 20

[Jim Mathies [:jimm]]

(In reply to comment #18)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Looks like a script is calling setTimeout(function() window.focus(), 0)
> > 
> > So maybe this is a bug in session restore. I'll see if I can track down the
> > actual script doing it.
> 
> Session Restore isn't calling window.focus() directly anywhere.
> window.content.focus(), yes. This happens in restoreDimensions which is called
> from a setTimeout(__, 0). We only focus into the window that was focused at
> quit though. And focus is called immediately after setting dimensions, not in
> another setTimeout.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2384

Filed bug 524745 on this.

Comment 21

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

Relnote added.

Comment 22

[Jim Mathies [:jimm]]

Attachment: revamp OnWindowPosChanged v.2

- disabled debug code by default
- added sanity tests on size mode changes

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 409423 [details] [diff] [review]
revamp OnWindowPosChanged v.2


With your patch, on shutdown, I see this assertion:

###!!! ASSERTION: Uh, IsInModalState() called w/o a reachable top window?: 'Erro
r', file c:/Users/dougt/builds/firefox/mozilla-central/dom/base/nsGlobalWindow.c
pp, line 5829

Comment 24

[Doug Turner (:dougt)]

Comment on attachment 409423 [details] [diff] [review]
revamp OnWindowPosChanged v.2

the assertion is probably unrelated (bug 404828).

if we resize to something that is both wider and taller, do we 
generate two redraws?

Comment 25

[Jim Mathies [:jimm]]

(In reply to comment #24)
> (From update of attachment 409423 [details] [diff] [review])

> if we resize to something that is both wider and taller, do we 
> generate two redraws?

No, since we don't pass RDW_UPDATENOW to RedrawWindow. (will confirm and clean up the casting stuff mentioned on irc.)

Comment 26

[Jim Mathies [:jimm]]

Attachment: revamp OnWindowPosChanged v.3

Comment 27

[Jim Mathies [:jimm]]

crap, lost my tests.

Comment 28

[Jim Mathies [:jimm]]

Attachment: revamp OnWindowPosChanged v.3

w/tests.

Comment 29

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

Can we get this baking on trunk so we can take it on 1.9.2 if it's clean?

Comment 30

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/18afb618437c

Comment 31

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d83f0eb3a755

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 411274 [details] [diff] [review]
revamp OnWindowPosChanged v.3

>diff --git a/widget/tests/window_state_windows.xul b/widget/tests/window_state_windows.xul
>new file mode 100644
>--- /dev/null
>+++ b/widget/tests/window_state_windows.xul
>@@ -0,0 +1,122 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+   -
>+   - The contents of this file are subject to the Mozilla Public License Version
>+   - 1.1 (the "License"); you may not use this file except in compliance with
>+   - the License. You may obtain a copy of the License at
>+   - http://www.mozilla.org/MPL/
>+   -
>+   - Software distributed under the License is distributed on an "AS IS" basis,
>+   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+   - for the specific language governing rights and limitations under the
>+   - License.
>+   -
>+   - The Original Code is Native Menus Test code
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Mozilla Corporation.
>+   - Portions created by the Initial Developer are Copyright (C) 2009
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -   Jim Mathies <[e-mail]>
>+   -
>+   - Alternatively, the contents of this file may be used under the terms of
>+   - either the GNU General Public License Version 2 or later (the "GPL"), or
>+   - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+   - in which case the provisions of the GPL or the LGPL are applicable instead
>+   - of those above. If you wish to allow use of your version of this file only
>+   - under the terms of either the GPL or the LGPL, and not to allow others to
>+   - use your version of this file under the terms of the MPL, indicate your
>+   - decision by deleting the provisions above and replace them with the notice
>+   - and other provisions required by the GPL or the LGPL. If you do not delete
>+   - the provisions above, a recipient may use your version of this file under
>+   - the terms of any one of the MPL, the GPL or the LGPL.
>+   -
>+   - ***** END LICENSE BLOCK ***** -->
>+
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
>+
>+<window id="NativeWindow"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        width="300"
>+        height="300"
>+        onload="onLoad();"
>+        title="Window State Tests">
>+
>+  <script type="application/javascript"
>+   src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript"
>+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js" />
>+  <script type="application/javascript">
>+    <![CDATA[
>+
>+    let Cc = Components.classes;
>+    let Ci = Components.interfaces;
>+    let Cu = Components.utils;
>+    Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+    SimpleTest.waitForExplicitFinish();
>+
>+    function onLoad() {
>+      var wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
>+      var win = wm.getMostRecentWindow("navigator:browser");
>+
>+      /*
>+      switch(win.windowState) {
>+        case win.STATE_FULLSCREEN:
>+        dump("STATE_FULLSCREEN \n");
>+        break;
>+        case win.STATE_MAXIMIZED:
>+        dump("STATE_MAXIMIZED \n");
>+        break;
>+        case win.STATE_MINIMIZED:
>+        dump("STATE_MINIMIZED \n");
>+        break;
>+        case win.STATE_NORMAL:
>+        dump("STATE_NORMAL \n");
>+        break;
>+      }
>+      */
>+
>+      // Make sure size mode changes are reflected in the widget.
>+      win.restore();
>+      ok(win.windowState == win.STATE_NORMAL, "window state is restored.");
>+      win.minimize();
>+      ok(win.windowState == win.STATE_MINIMIZED, "window state is minimized.");
>+      
>+      // Windows resizes children to 0x0. Code in nsWindow filters these changes out. Without
>+      // this all sorts of screwy things can happen in child widgets.
>+      ok(document.height > 0, "document height should not be zero for a minimized window!");
>+      ok(document.width > 0, "document width should not be zero for a minimized window!");

As noticed by bz, should this be testing win.document.{height,width}?

>+      
>+      // Make sure size mode changes are reflected in the widget.
>+      win.restore();
>+      ok(win.windowState == win.STATE_NORMAL, "window state is restored.");
>+      win.maximize();
>+      ok(win.windowState == win.STATE_MAXIMIZED, "window state is maximized.");
>+      win.restore();
>+      ok(win.windowState == win.STATE_NORMAL, "window state is restored.");
>+
>+      /*
>+      dump(win.screenX + "\n");
>+      win.minimize();
>+      dump(win.screenX + "\n");
>+      win.restore();
>+      dump(win.screenX + "\n");
>+      */
>+      
>+      SimpleTest.finish();
>+    }
>+
>+  ]]>
>+  </script>
>+  <body xmlns="http://www.w3.org/1999/xhtml">
>+    <p id="display"></p>
>+    <div id="content" style="display: none"></div>
>+    <pre id="test"></pre>
>+  </body>
>+</window>