Closed Bug 520178 Opened 11 years ago Closed 11 years ago

Minimized windows appear offscreen when restoring from session store

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: robarnold, Assigned: jimm)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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.
Flags: blocking-firefox3.6?
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.
Duplicate of this bug: 520483
Keywords: regression
1. This regressed with bug 502713.
2. Please set Platform to "All," as XP is also affected
--> Core::Widget 32 as per comment 3
Blocks: 502713
Component: Session Restore → Widget: Win32
Flags: blocking-firefox3.6?
Product: Firefox → Core
QA Contact: session.restore → win32
Target Milestone: --- → mozilla1.9.1b2
Blocking, suggest relnote for beta1
Flags: blocking1.9.2+
Keywords: relnote
Priority: -- → P2
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().
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.
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.
OS: Windows 7 → Windows XP
This is a blocker without an owner, so let's say that Jim Mathies will look at it? (hoping)
Assignee: nobody → jmathies
Target Milestone: mozilla1.9.1b2 → mozilla1.9.2
(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.
Attached patch revamp OnWindowPosChanged v.1 (obsolete) — Splinter Review
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?)
Attachment #408608 - Flags: review?(tellrob)
Attached file focus stack
Attachment #408608 - Flags: review?(doug.turner)
Attachment #408608 - Flags: review?(enndeakin)
Looks like a script is calling setTimeout(function() window.focus(), 0)
(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.
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.
(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.
(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
Blocks: 524745
Comment on attachment 408608 [details] [diff] [review]
revamp OnWindowPosChanged v.1

Thanks Neil!
Attachment #408608 - Flags: review?(enndeakin)
(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.
Relnote added.
Keywords: relnote
Attached patch revamp OnWindowPosChanged v.2 (obsolete) — Splinter Review
- disabled debug code by default
- added sanity tests on size mode changes
Attachment #408608 - Attachment is obsolete: true
Attachment #408608 - Flags: review?(tellrob)
Attachment #408608 - Flags: review?(doug.turner)
Attachment #409423 - Flags: review?(doug.turner)
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
Attachment #409423 - Flags: review?(doug.turner)
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?
Attachment #409423 - Flags: review?(doug.turner)
(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.)
Attached patch revamp OnWindowPosChanged v.3 (obsolete) — Splinter Review
Attachment #409423 - Attachment is obsolete: true
Attachment #411272 - Flags: review?(doug.turner)
Attachment #409423 - Flags: review?(doug.turner)
crap, lost my tests.
w/tests.
Attachment #411272 - Attachment is obsolete: true
Attachment #411274 - Flags: review?(doug.turner)
Attachment #411272 - Flags: review?(doug.turner)
Attachment #411274 - Flags: review?(doug.turner) → review+
Can we get this baking on trunk so we can take it on 1.9.2 if it's clean?
http://hg.mozilla.org/mozilla-central/rev/18afb618437c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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 <jmathies@mozilla.com>
>+   -
>+   - 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>
You need to log in before you can comment on or make changes to this bug.