Bugzilla

Assignee

Comment 1

•

6 years ago

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

Thanks for reporting this!

Assignee: nobody → emilio

Status: UNCONFIRMED → NEW

Ever confirmed: true

Alice0775 White

Comment 2

•

6 years ago

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

Assignee

Comment 3

•

6 years ago

Attached file Stack of the bogus resize. — Details

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

Unsure where does that come from...

Assignee

Comment 4

•

6 years ago

Attached patch Patch to reproduce the issue independently of screen resolution. — Details — Splinter Review

Comment hidden (mozreview-request)

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/

Assignee

Comment 6

•

6 years ago

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.

Attachment #8962913 - Flags: feedback?(stransky)

Attachment #8962913 - Flags: feedback?(karlt)

Assignee

Comment 7

•

6 years ago

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

•

6 years ago

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.

Attachment #8962913 - Flags: feedback?(karlt)

Comment 9

•

6 years ago

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.

Attachment #8962913 - Flags: feedback?(stransky)

Comment 10

•

6 years ago

It may be also related to Bug 1433603.

Updated

•

6 years ago

Comment 11

•

6 years ago

(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.

Flags: needinfo?(karlt)

Assignee

Comment 12

•

6 years ago

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.

Updated

•

6 years ago

Blocks: 1447859

Comment 13

•

6 years ago

(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.

Flags: needinfo?(karlt)

Comment 14

•

6 years ago

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.

Noemi Erli[:noemi_erli]

Comment 15

•

6 years ago

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

Flags: needinfo?(enndeakin)

Whiteboard: [stockwell needswork]

Assignee

Comment 16

•

6 years ago

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

Flags: needinfo?(enndeakin)

Assignee

Updated

•

6 years ago

Flags: needinfo?(emilio)

Assignee

Updated

•

6 years ago

Component: XUL → Widget: Gtk

Comment hidden (mozreview-request)

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/

Assignee

Updated

•

6 years ago

Flags: needinfo?(emilio)

Updated

•

6 years ago

Blocks: 1453554

Comment 18

•

6 years ago

mozreview-review

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);
    }

Attachment #8962913 - Flags: review?(karlt)

Comment 19

•

6 years ago

mozreview-review-reply

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 hidden (mozreview-request)

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

•

6 years ago

mozreview-review

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.

Attachment #8962913 - Flags: review?(karlt) → review+

Dão Gottwald [:dao]

Updated

•

6 years ago

Keywords: regression

Priority: -- → P1

Andrei Ciure[:aciure]

Comment 22

•

6 years ago

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

Flags: needinfo?(emilio)

Stefan Hindli [:stefan_hindli]

Assignee

Comment 23

•

6 years ago

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

Flags: needinfo?(emilio)

Pulsebot

Comment 24

•

6 years ago

Pushed by ecoal95@gmail.com:
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

•

6 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox61: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla61

Joel Maher ( :jmaher ) (UTC -8)

Updated

•

6 years ago

Whiteboard: [stockwell needswork] → [stockwell fixed:product]

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

status-firefox59: --- → unaffected

status-firefox60: --- → unaffected

status-firefox-esr52: --- → unaffected

status-firefox-esr60: --- → unaffected

Updated

•

6 years ago

Depends on: 1458860

Assignee

Comment 27

•

6 years ago

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