1454589 - Sizes of newly opened unmaximized windows aren't remembered and are always very small after landing patches from bug #1446264

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open Firefox
2. Open new Firefox window
3. Unmaximize it
and see that this new Firefox window has very tiny dimensions and doesn't remember its unmaximized size set from before

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Caused by:
bug #1446264

Regression range pushlog:
https://hg.mozilla.org/integration/autoland/json-pushes?changeset=6dbe66440bd2b872776e22d1a2cbd32755c81a2c&full=1

mozregression-gui log:
> 2018-04-17T12:12:40: INFO : Narrowed inbound regression window from [a75367f4, 422ce7e8] (3 builds) to [a75367f4, 6dbe6644] (2 builds) (~1 steps left)
> 2018-04-17T12:12:40: DEBUG : Starting merge handling...
> 2018-04-17T12:12:40: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=6dbe66440bd2b872776e22d1a2cbd32755c81a2c&full=1
> 2018-04-17T12:12:41: DEBUG : Found commit message:
> Bug 1446264 part 5 - Add test for this bug. r=bz
> 
> MozReview-Commit-ID: DS2ucDd0Met
> 
> 2018-04-17T12:12:41: INFO : The bisection is done.
> 2018-04-17T12:12:41: INFO : Stopped

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

What's your operating system? What's your screen resolution?

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

FWIW, I cannot reproduce the issue from the STR in comment 0 on Windows 10.

There is a known issue similar to this on Linux which is bug 1449166. That's unrelated to the new window, though. If you unmaximize the original window, it would also be very small.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> What's your operating system?
My system is Windows 7 (64-bit) with latest security patches, as stated in "Platform: x86_64 Windows 7"

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
What's your screen resolution?
My screen resolution is 1680x1050.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> FWIW, I cannot reproduce the issue from the STR in comment 0 on Windows 10.
I'm attaching screenshot, so you can see how this issue looks on my end. 

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> There is a known issue similar to this on Linux which is bug 1449166. That's
> unrelated to the new window, though. If you unmaximize the original window,
> it would also be very small.

Looks similar, but probably it's not about this specific issue.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

In your screencast, what if you restore the original window rather than the new window?

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I mean, what happens if you restore the original window?

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast2.mp4

Depends, please see new screencast. I probably tested all needed steps.
Looks like only size of first initial window is remembered, but sizes of all new created windows are not remembered.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Ok, I managed to reproduce this on Windows 7 (but not Windows 10).

I suspect this is a widget issue that if a window is opened in maximized, its original size is not forgotten by the system. The reason that the original window is not affected is probably due to browser.startup.blankWindow. When I switched off that pref, if the original window is opened maximized, it would be restored to a small size as well.

It is not clear to me why it appears on Windows 7 but not Windows 10. (I don't have dev environment on Windows 7 so it could become quite tricky to track down this issue...)

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Also, the real regression is probably not bug 1446264 but bug 1439875, because the latter introduced a regression that window would not be opened maximized, which was fixed by the former.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

The width and height property are correct on the new window, so it isn't an issue related to persisting (and thus not an issue related to DOM or XUL).

I would probably need to do some local debug build and install them in the win7 vm to test :/

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

If you need some logs from debug build, I can help and probably do this. Just some information will be needed, what to do and how to do that.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

So I setup a debug environment on a Win7 VM, and managed to reproduce this bug.

As far as I can see, we do invoke the system API `SetWindowPos` for the restore size before we maximizing the window, and the system acknowledge this change with a `WM_WINDOWPOSCHANGED` message, and at that point, we can get the correct restore size from the system API `GetWindowRect` and `GetClientRect`. So it seems that the system somehow loses the track of restore size.

But there is one interesting observation that, after the message triggered by resizing, there is a following message with `SWP_FRAMECHANGED` set, triggered by `SetNonClientMargins` (from syncing "chromemargin" attribute). When handling that message, `GetWindowPlacement` returns `SW_SHOWMAXIMIZED` in `pl.showCmd`, but the system shouldn't have known that the window will be maximized. The size returned from `GetClientRect` later in the handler is still the restored size, though.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

The following is a log with WINSTATE_DEBUG_OUTPUT. In this log, the second OnWindowPosChanged is invoked from the Resize method, and everything seems to be setup correctly there (and I also confirmed that the result of GetWindowPlacement has the right size).

The third OnWindowPosChanged is invoked by SetNonClientMargins, and in that, given the information returned from GetWindowPlacement, system has known that the window is maximized, and the normal rect has already been wrong.

The fourth one is triggered by Show method, which opens the window in maximized state. I think that works as expected.

So I guess the question is, between the second and the third message, what tells the system that the window is maximized. That can probably provide some clue on why the normal rect is messed up.


[3812:Main Thread]: I/Widget *** OnWindowPosChanged: [  top] 
[3812:Main Thread]: I/Widget WINDOWPOS flags:
[3812:Main Thread]: I/Widget SWP_FRAMECHANGED 
[3812:Main Thread]: I/Widget SWP_NOSIZE 
[3812:Main Thread]: I/Widget SWP_NOZORDER 
[3812:Main Thread]: I/Widget SWP_NOACTIVATE 
[3812:Main Thread]: I/Widget 
[3812:Main Thread]: I/Widget *** mSizeMode: nsSizeMode_Normal

[3812:Main Thread]: I/Widget *** OnWindowPosChanged: [  top] 
[3812:Main Thread]: I/Widget WINDOWPOS flags:
[3812:Main Thread]: I/Widget SWP_NOZORDER 
[3812:Main Thread]: I/Widget SWP_NOACTIVATE 
[3812:Main Thread]: I/Widget 
[3812:Main Thread]: I/Widget *** Resize window: 0 x 0 x 1452 x 990
[3812:Main Thread]: I/Widget glass margins: left:2 top:2 right:2 bottom:2

[3812:Main Thread]: I/Widget *** OnWindowPosChanged: [  top] 
[3812:Main Thread]: I/Widget WINDOWPOS flags:
[3812:Main Thread]: I/Widget SWP_FRAMECHANGED 
[3812:Main Thread]: I/Widget SWP_NOSIZE 
[3812:Main Thread]: I/Widget SWP_NOZORDER 
[3812:Main Thread]: I/Widget SWP_NOACTIVATE 
[3812:Main Thread]: I/Widget 
[3812:Main Thread]: I/Widget *** mSizeMode: nsSizeMode_Maximized
[3812:Main Thread]: I/Widget *** SetFocus: [  top] raise=0

[3812:Main Thread]: I/Widget *** OnWindowPosChanged: [  top] 
[3812:Main Thread]: I/Widget WINDOWPOS flags:
[3812:Main Thread]: I/Widget SWP_FRAMECHANGED 
[3812:Main Thread]: I/Widget SWP_SHOWWINDOW 
[3812:Main Thread]: I/Widget SWP_NOZORDER 
[3812:Main Thread]: I/Widget 
[3812:Main Thread]: I/Widget *** mSizeMode: nsSizeMode_Maximized
[3812:Main Thread]: I/Widget *** Resize window: -8 x -8 x 1936 x 1079

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

The "glass margins" log gave me some hint to try, and it seems that if I disable aero theme (by switching to Windows Classic or Windows 7 Basic theme), this issue disappears.

So it must be code related to glass border causing this issue...

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

I have a patch which seems to work...

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1454589 - Don't change window style according to sizemode if the window hasn't be shown.

Review commit: https://reviewboard.mozilla.org/r/237898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237898/

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

And I'm pretty sure the test I added in bug 1446264 has covered this case: https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/toolkit/content/tests/chrome/test_maximized_persist.xul#80-89

It doesn't fail because the Windows 7 instance we run for testing doesn't enable Aero, and thus the problematic code path here is not triggered.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8969167 [details]
Bug 1454589 - Don't change window style according to sizemode if the window hasn't be shown.

https://reviewboard.mozilla.org/r/237898/#review243804

Looks ok to me. Please keep an eye out for regression bugs.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8969167 [details]
Bug 1454589 - Don't change window style according to sizemode if the window hasn't be shown.

https://reviewboard.mozilla.org/r/237898/#review243810

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80ada5ae02fe
Don't change window style according to sizemode if the window hasn't be shown. r=jimm

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/80ada5ae02fe

Comment 22

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 61.0a1 (2018-04-20), so I'm marking this bug as VERIFIED.

Thank you very much! \o/