688929 - restoring the browser often leaves windows offscreen (on windows)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Christopher Blizzard (:blizzard)]

This bug is back, but I can't find the original.

Running the 7 betas here.  It's very often on restart that a two or three of the five windows that I have open will end up offscreen.  I have to go into my sessionstore.js file and edit it by hand to change the screenX and screenY values from -32000 to 0 so that I can see them again.

Comment 1

[Dietrich Ayala (:dietrich)]

cc'ing Neil and Paul for input on how window positions could be getting scrambled in the restart-and-restore cycle.

Comment 2

[Jan Rieke]

I am also having this bug on the release version 6.0.2, so this is not only an issue of the 7 Branch. As I cannot remember any occurrence of this bug before 6.0.2, I assume that the regression happened between 6.0.1 and 6.0.2.

Comment 3

[LegNeato]

I can pretty much guarantee it wasn't caused by 6.0.2.

Comment 4

[Jan Rieke]

Ok, that was just an assumption. Could also be earlier.

Seems to be a problem with minimized windows.
Original bug could be bug 520178 or, even earlier, bug 502713.

Comment 5

[Christopher Blizzard (:blizzard)]

Oh, yeah, I tend to leave windows maximized or minimized and shut down when doing that often.

Comment 6

[Brian R. Bondy [:bbondy]]

I tried minimizing and closing several windows via taskbar and then restarting the browser but I can't reproduce.  Does anyone have steps to consistently reproduce?  I do notice that only 3 of my 5 windows get restored though.  Results are the same for me on Aurora and Nightly.  Haven't tried earlier versions.

Comment 7

[Brian R. Bondy [:bbondy]]

Also using Windows 7 x64

Comment 8

[George K.]

I can confirm Windows XP (x86) is affected, having seen this with Aurora 8 and 9.

Most recently I had a window's properties set to:

"width": 160,
"height": 34,
"screenX": -32000,
"screenY": -32000,

Given the screen placement, that 160 pixels is the width of a taskbar entry/group if there is no "crowding" (at least on this system, I measured), and 34 pixels is the minimum height an Aurora window can be, looks like minimization is involved. I'll see if I can get STR.

Comment 9

[George K.]

This may be triggered by hibernating Windows with a browser window maximized. Now that I have something to test I'll try to confirm exact STR, hopefully this weekend.

Comment 10

[Jan Rieke]

That sounds reasonable. I often hibernate my PC, but until now I did not think of the possibility that this could have something to do with the messed-up windows.

Comment 11

[J S]

I've found a way to reproduce this bug. It has nothing to do with hibernating Windows. Tested with Firefox 11 on new profiles on both XP x86 and Windows 7 x64 (with window grouping off, so each Firefox window has its own taskbar entry).

1. Open two windows and minimize the second window.
2. Restore window 2 by clicking on it in the taskbar and immediately click it again so it re-minimizes. (have to re-minimize really fast)
3. File/exit from window 1

sessionstore.js will now contain -32000/-32000 for position and 160x24 (or whatever your taskbar icon dimensions are) for window 2 and restoring previous session will result in the offscreen window.

If during step 2 you Restore window 2, then wait a second, then re-minimize it the sessionstore.js will contain the correct dimensions/position for window 2.

Comment 12

[Jan Rieke]

Nice work, J S. It's reproducible here here on Windows 7.

In step 2, to make sure you are fast enough, try clicking quickly 3 times on the minimized taskbar window entry.

Comment 13

[James]

Attachment: Preliminary patch

I think this bug belongs to the Widget: Win32 component and is related to bug 665249, bug 646946 and bug 739082

The patch fixes this bug and the others mentioned. I'm including my notes on the bug for reference.

My notes:
The root cause is the NS_SIZEMODE dispatch in WM_WINDOWPOSCHANGING

The flow is something like this:
WM_WINDOWPOSCHANGING -> NS_SIZEMODE -> nsWindow::SetSizeMode (sets mSizeMode to new mode) -> dispatching NS_ACTIVATE -> nsXULWindow::SavePersistentAttributes (size/position/sizemode)

WM_WINDOWPOSCHANGED -> NS_SIZEMODE -> nsWindow::SetSizeMode (aMode == mSizeMode, returns) -> nsWindow::OnResize -> NS_SIZE

When SavePersistentAttributes is called too early (during WM_WINDOWPOSCHANGING, before WM_WINDOWPOSCHANGED), it's getting the coordinates of the window pre-restore.

The reason delaying a re-minimize works is because WM_WINDOWPOSCHANGED -> nsWindow::OnResize is dispatching an NS_SIZE event, which calls nsXULWindow::SavePersistentAttributes again after a 500ms timer fires. If the window is re-minimized (or re-maximized) too fast before that 500ms timer fires, then the persistent attributes are stuck with the bad coordinates.

Blocking the dispatch of NS_ACTIVATE in nsWindow::SetSizeMode on a window restore during WM_WINDOWPOSCHANGING will prevent saving the bad attributes,
but the call must be deferred to the next SetSizeMode that originates from WM_WINDOWPOSCHANGED so the attributes are properly updated. 

This is especially important when going from a maximized to normal window because the sizemode attribute has to be updated before OnResize is called or javascript will not see the correct sizemode during the resize event and won't update css (titlebar tabs).

Comment 14

[Jim Mathies [:jimm]]

This is a touchy area of the code so I've pushed this to try for a test run - 

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=a90f4d69aa9e

Comment 15

[Mozilla RelEng Bot]

Try run for a90f4d69aa9e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a90f4d69aa9e
Results (out of 49 total builds):
    success: 43
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a90f4d69aa9e

Comment 16

[James]

Attachment: patch v2

I've cleaned the patch up a bit (removed ugly bool flag/redundant code). Can I safely assume the two oranges in the windows debug build from try were unrelated to the patch?

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 620908 [details] [diff] [review]
patch v2

Yeah it looks ok. Ran with this for a bit, didn't see any issues.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0a63ddcc8f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef0a63ddcc8f

Comment 20

[Jim Mathies [:jimm]]

Thanks for the contribution James!