1449166 - Initial size of unmaximized window for new profile is too small

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[atlanto]

STR:
1. Create a new profile
2. Launch profile in new browser
3. Unmaximize the new browser window

ER:
Ordinally size.

AR:
Too small.
OS        ScreenSize WindowSize
LinuxMint 1280x1024  300x200
LinuxMint 1024x768   300x200
Manjaro   1024x768   300x99
(No problem on Windows10)
Windows10 1280x1024  1160x916

Former(or 59.0.1) size on LinuxMint(1280x1024) is 1152x899.

mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a75367f49378821ebe82c84e2dc714beb37bdfc2&tochange=6dbe66440bd2b872776e22d1a2cbd32755c81a2c

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I can repro this, it needs a really small screen resolution.

Thanks for reporting this!

Comment 2

[Alice0775 White]

I think the bug 1439875 hid the existing bug 1446629, and bug 1446264 re-exposed it.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack of the bogus resize.

That's the resize that changes the window size to small, from a normal size.

Unsure where does that come from...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch to reproduce the issue independently of screen resolution.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

gtk_window_resize is asynchronous, it just leaves a resize_width / resize_height
flag and carries on. The fatality here happens as follows:

  (1) We start of with a 1x1 window.
  (2) We size it to a reasonable size (let's call it S).
  (3) We call SetSizeMode(Maximized) on that.

When the user comes back from maximized mode, it expects S as a size (and
rightfully so I'd say), however, it finds whatever the size constraints set via
SetSizeConstraints end up being.

That happens because gtk_window_resize just queues two flags and schedules a
resize[1], and that event does not happen before (3).

Thus, when the next resize happens, we just notice that the window is
maximized[2], so we don't use those bits, and eventually those resize flags get
completely lost[3].

This didn't happen before bug 1446264 because we failed to maximize the window,
that is, to do step (3) entirely. That is indeed what bug 1446264 fixed.

The alternative here would be just calling gtk_window_move_resize right after
calling Resize(), I'd say.

[1]: https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L5373
[2]: https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L8346
[3]: https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L9038

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Hi Karl, Martin. Could you take a look at this patch and see if it makes sense?

I spent a fair amount of time debugging it so the description in the commit message should be accurate, but I'm not sure if this is the preferred way to fix it, or I should just call gtk_window_move_resize(GTK_WINDOW(mShell)) after the gtk_window_* calls instead, or what not.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Now, about why did this work before bug 1439875, and that bug regressed this:

Before that patch, the order of calls is:

  OnChromeLoaded()
    Resize(..) -> enqueues a resize.
    SetSizeMode(Maximized) -> enqueues an event that triggers the window state change..
    Show(..) -> runs _before_ any of those events have run, making the window visible.

Show does a check_resize, and hits https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L7781, effectively calling gtk_window_move_resize, since now it's visible, which picks up the first Resize(..) call.

But after that patch, the order of calls is:

  BeforeStartLayout()
    Resize(..) -> enqueues a resize.
    SetSizeMode(Maximized) -> enqueues an event that triggers the window state change.
    // nothing...

So the check_resize that happens after the Resize call bails out, since it's not visible, so there's no gtk_window_move_resize. The SizeMode change event doesn't bail out, effectively dropping the resize event on the floor.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Thank you for the accurate and helpful analysis.  There have been a number of
bugs in this GTK code, and so it is difficult to predict exactly what effects
even small changes may have.

gdk_window_resize() behaves a little differently from gtk_window_resize().
The difference that I am most afraid of is that the gdk_ version does not add
the necessary extra when CSD is in use.
https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L8355

(There is also a difference when called while the window is already maximized.
gtk_window_resize() has no effect, but gdk_window_resize() may have an effect,
perhaps depending on window manager.  I haven't checked the GTK_WINDOW_POPUP
situation.)

For the sake of CSD at least, I would recommend a different workaround.

The minimal change I can imagine would be to call
gtk_container_check_resize(GTK_CONTAINER(mShell)) before
gtk_window_maximize/fullscreen calls.
That would apply resize_width/height before maximized can be set.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Karl is the best reviewer here.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

It may be also related to Bug 1433603.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> Thank you for the accurate and helpful analysis.  There have been a number of
> bugs in this GTK code, and so it is difficult to predict exactly what effects
> even small changes may have.
> 
> gdk_window_resize() behaves a little differently from gtk_window_resize().
> The difference that I am most afraid of is that the gdk_ version does not add
> the necessary extra when CSD is in use.
> https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/
> gtk/gtkwindow.c#L8355
> 
> (There is also a difference when called while the window is already
> maximized.
> gtk_window_resize() has no effect, but gdk_window_resize() may have an
> effect,
> perhaps depending on window manager.  I haven't checked the GTK_WINDOW_POPUP
> situation.)
> 
> For the sake of CSD at least, I would recommend a different workaround.

I see, that makes sense.

> The minimal change I can imagine would be to call
> gtk_container_check_resize(GTK_CONTAINER(mShell)) before
> gtk_window_maximize/fullscreen calls.
> That would apply resize_width/height before maximized can be set.

Unfortunately that won't work, because by the time this happens, the window is not yet visible, and thus check_resize bails out without doing any work.

I also thought of something like:

    if (mIsShown) {
        gtk_window_resize(GTK_WINDOW(mShell), size.width, size.height);
    } else {
        // Size will be picked up when shown.
        gtk_window_set_default_size(GTK_WINDOW(mShell), size.width, size.height);
    }

By the time Show is called, the window is already maximized, and thus we don't even check the default size anymore... So I'm kinda out of ideas.

I'll try to dig a bit more, maybe workaround it by not calling maximize, etc. before Show(..), but if you know a good way to tell GTK+ "Please update the size even though you're not a visible widget" that'd be neat.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Oh, also, this may explain one of the other weird-ish glitches I'm seeing with GTK, which is the window not being correctly invalidated on first paint.

Even before my patch, the size in gtk_window_map happens to be 1x1 until the resize is checked, and thus the invalidate call invalidates basically nothing.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> I'll try to dig a bit more, maybe workaround it by not calling maximize,
> etc. before Show(..),

Yes, maximize could be delayed until after gtk_widget_show(), I assume

This approach deals with the loss of resize_width when maximizing before show,
which I guess is enough to make the reported case work.
It doesn't address the async resize before maximize after show, but I guess
that is not so common.

> but if you know a good way to tell GTK+ "Please update
> the size even though you're not a visible widget" that'd be neat.

I don't see a way to do that.

An alternative, which is fairly simple but crafty, might be to remove
GDK_WINDOW_STATE_MAXIMIZED from aEvent->changed_mask in
OnWindowStateEvent() if the window is not shown.  That would prevent
gtk_window_state_event() from setting priv->maximized.  The show will trigger
a subsequent window-state-event because GDK_WINDOW_STATE_WITHDRAWN will
change.

This alternative would also address an async resize before maximize after
show.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

FWIW, if you are making the window maximized after showing, you would likely cause the same regression as bug 1448199 which happens on Windows due to delayed maximizing.

Comment 15

[Noemi Erli[:noemi_erli]]

Any updates here? since this blocks bug 1447859 and in the past 7 days there are 39 failures on the following platforms: Linux debug, Linux x64 asan and linux64-ccov opt.

Recent log failure: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=174442928&lineNumber=17586

Relevant part of the log: 
[task 2018-04-19T00:48:04.962Z] 00:48:04     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_maximized_persist.xul | width should match after restore window - got 200, expected 300
[task 2018-04-19T00:48:04.962Z] 00:48:04     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2018-04-19T00:48:04.963Z] 00:48:04     INFO - checkWindow@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:38:7
[task 2018-04-19T00:48:04.963Z] 00:48:04     INFO - runTest@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:88:5
[task 2018-04-19T00:48:04.965Z] 00:48:04     INFO - async*onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:1:1
[task 2018-04-19T00:48:04.965Z] 00:48:04     INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:17
[task 2018-04-19T00:48:04.966Z] 00:48:04     INFO - EventHandlerNonNull*this.addLoadEvent@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:171:13
[task 2018-04-19T00:48:04.967Z] 00:48:04     INFO - @chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1444:5
[task 2018-04-19T00:48:04.968Z] 00:48:04     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-19T00:48:04.969Z] 00:48:04     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_maximized_persist.xul | height should match after restore window - got 200, expected 300

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I haven't had the time to look into this recently, but it's on my radar.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231714/diff/1-2/

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review244790

::: commit-message-0d428:1
(Diff revision 2)
> +Bug 1449166: Prevent maximization of unshown window. r?karlt

This is not actually preventing maximization, but it
does prevent GtkWindow's maximized flag from being set.

Perhaps the checkin comment could be something like
"trick GTK into using values from gtk_window_resize when showing maximized windows

so as to provide a sensible size for the window when the user exits maximized
state."

::: widget/gtk/nsWindow.cpp:3329
(Diff revision 2)
> +    // If we're not shown yet prevent maximization, since it will eat pending
> +    // resizes. See bug 1449166.

I'd like more details in the explanation comment inline here, as the
mechanism is not easy to understand.

Something like:

"When a window is resized before it is shown, gtk_window_resize() delays
 resizes until the window is shown.  If gtk_window_state_event() sees a
 GDK_WINDOW_STATE_MAXIMIZED change [1] before the window is shown, then
 gtk_window_compute_configure_request_size() ignores the values from the
 resize [2].
 [1] https://gitlab.gnome.org/GNOME/gtk/blob/3.22.30/gtk/gtkwindow.c#L7967
 [2] https://gitlab.gnome.org/GNOME/gtk/blob/3.22.30/gtk/gtkwindow.c#L9377

 In order to provide a sensible size for the window when the user exits
 maximized state, we hide the GDK_WINDOW_STATE_MAXIMIZED change from
 gtk_window_state_event() so as to trick GTK into using the values from
 gtk_window_resize() in its configure request.

 We instead notify gtk_window_state_event() of the maximized state change once
 the window is shown."

::: widget/gtk/nsWindow.cpp:3332
(Diff revision 2)
> +        aEvent->changed_mask =
> +            static_cast<GdkWindowState>(aEvent->changed_mask & ~GDK_WINDOW_STATE_MAXIMIZED);

Please wrap so as to keep within 80 columns.

I would wrap after '>' so that the statement requires only two lines, but wrapping after '='
and '&' is fine too.

::: widget/gtk/nsWindow.cpp:3334
(Diff revision 2)
> +    // If we're not shown yet prevent maximization, since it will eat pending
> +    // resizes. See bug 1449166.
> +    if (!mIsShown) {
> +        aEvent->changed_mask =
> +            static_cast<GdkWindowState>(aEvent->changed_mask & ~GDK_WINDOW_STATE_MAXIMIZED);
> +    }

We need to tell gtk_window_state_event() about the maximized state eventually so that it can appropriately draw and handle input on the window buttons it adds when in client-side-decorated (CSD) mode.

CSD is only used on some systems.  You probably did not notice any problems because the window manager added these buttons on your system.

I expect this should work:

    } else if (aEvent->changed_mask & GDK_WINDOW_STATE_WITHDRAWN &&
               aEvent->new_window_state & GDK_WINDOW_STATE_MAXIMIZED) {
        aEvent->changed_mask = static_cast<GdkWindowState>
            (aEvent->changed_mask & GDK_WINDOW_STATE_MAXIMIZED);
    }

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review244790

> We need to tell gtk_window_state_event() about the maximized state eventually so that it can appropriately draw and handle input on the window buttons it adds when in client-side-decorated (CSD) mode.
> 
> CSD is only used on some systems.  You probably did not notice any problems because the window manager added these buttons on your system.
> 
> I expect this should work:
> 
>     } else if (aEvent->changed_mask & GDK_WINDOW_STATE_WITHDRAWN &&
>                aEvent->new_window_state & GDK_WINDOW_STATE_MAXIMIZED) {
>         aEvent->changed_mask = static_cast<GdkWindowState>
>             (aEvent->changed_mask & GDK_WINDOW_STATE_MAXIMIZED);
>     }

aEvent->changed_mask | GDK_WINDOW_STATE_MAXIMIZED

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231714/diff/2-3/

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review245548

::: widget/gtk/nsWindow.cpp:3330
(Diff revision 3)
>          }
>          return;
>      }
>      // else the widget is a shell widget.
>  
> +    // The block below is a bit evil.

Aye, it is.

Comment 22

[Andrei Ciure[:aciure]]

emilio: can you give us an eta for landing this patch?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

I triggered autoland, but it's closed... So whenever it reopens.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f10e92f864
Trick GTK into using values from gtk_window_resize when showing maximized windows. r=karlt

Comment 25

[Stefan Hindli [:stefan_hindli]]

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

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Filed https://gitlab.gnome.org/GNOME/gtk/issues/1044 on the underlying GTK issue / limitation.