Open Bug 1254448 Opened 5 years ago Updated 2 years ago

Ensure window size is correct on Linux when fullscreenchange event dispatches for Fullscreen API calls

Categories

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

All
Linux
defect

Tracking

()

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 3 open bugs)

Details

(Whiteboard: tpi:+)

Attachments

(2 files)

It seems on Linux the window size is not changed when fullscreenchange event is dispatched, which should be fixed.
Blocks: 1240643
From bug 1240643 comment 15 it seems this problem only occurs on e10s.
tracking-e10s: --- → ?
Summary: Ensure window size is correct on Linux when fullscreenchange event dispatches → Ensure window size is correct on Linux when fullscreenchange event dispatches in e10s
No, this actually occurs on non-e10s as well. I have had patches for this, but need to investigate some failures tomorrow.
Assignee: nobody → quanxunzhen
tracking-e10s: ? → ---
Summary: Ensure window size is correct on Linux when fullscreenchange event dispatches in e10s → Ensure window size is correct on Linux when fullscreenchange event dispatches
fullscreenchange events triggered by exiting fullscreen unexpectedly may need further work to ensure this, and that may have relatively lower priority than this one.
Summary: Ensure window size is correct on Linux when fullscreenchange event dispatches → Ensure window size is correct on Linux when fullscreenchange event dispatches for Fullscreen API calls
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03adcd21166&filter-classifiedState=unclassified
(Hidden jobs are just empty jobs due to using tag to restrict tests running)

Mostly fine. Even better, it seems the two intermittents bug 1239932 and bug 931445 also go away here (but this only runs a small set of the tests, so it may not reflect the real case if previous tests could affect the state of those tests).

There is one "Assertion count 2 is greater than expected range 0-0 assertions" failure on Linux debug M(c2), which I'll continue investigating a bit. It doesn't seem to be a big issue, though.
Blocks: 1238838
It also seems bug 888164 goes away as well per https://treeherder.mozilla.org/#/jobs?repo=try&revision=2923bf1cc720
Blocks: 888164
No longer blocks: 1238838
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #7)
> It also seems bug 888164 goes away as well per
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2923bf1cc720

The assertions disappear in this try push... hmmmm... Anyway, let's do a full mochitest try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86708d8b23ef
Attachment #8729399 - Flags: review?(bugs) → review+
Comment on attachment 8729399 [details]
MozReview Request: Bug 1254448 part 2 - Add new test for checking window dimension.

https://reviewboard.mozilla.org/r/39421/#review36135
Attachment #8729398 - Flags: review?(karlt)
https://reviewboard.mozilla.org/r/39419/#review36267

::: widget/gtk/nsWindow.h:432
(Diff revision 1)
> +    // Fullscreen change has four steps:
> +    // 1. the change is started via MakeFullscreen;
> +    // 2. the window state gets changed in OnWindowStateEvent;
> +    // 3. the position of the window gets changed in OnConfigureEvent;
> +    // 4. the size of the window gets changed in OnSizeAllocate.
> +    // We want to continue the fullscreen change only after all of them
> +    // finish. However, the order of step 2 and 3 is not consistent
> +    // across machines. Some trigger step 3 first, others do step 2
> +    // first instead. It seems OnSizeAllocate is always the last signal
> +    // in these steps, and thus we invoke the callback there. However,
> +    // actually this is not documented anywhere, so this assumption
> +    // could be broken in some cases as well.

Errrr... it seems step 2, 3, 4 can happen in completely arbitrary order... I have a new idea then...
I'm uncomfortable with trying to enforce this.
If all three events are received, then I'd expect OnSizeAllocate() to be called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at any time wrt those.
However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent() will be called.

Is this causing real problems, or is it just that some tests are not waiting for the size change?

Size changes need to be handled anyway regardless of window state.

GDK delays requesting a draw until there are no events remaining in the queue, so drawing in mid-transition would usually be rare.  OTMC likely defeats that logic, but doesn't the fullscreen animation usually cover the window during transition?
(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> I'm uncomfortable with trying to enforce this.
> If all three events are received, then I'd expect OnSizeAllocate() to be
> called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at
> any time wrt those.
> However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent()
> will be called.
> 
> Is this causing real problems, or is it just that some tests are not waiting
> for the size change?

It is a real issue, since we want to ensure our size being stable after we dispatch fullscreenchange event, so that the page can rely on the size at that moment and we do a big reflow rather than doing two reflows for resize and fullscreen separately.

> Size changes need to be handled anyway regardless of window state.

Yes, but authors may want to use the size of elements and viewport in fullscreenchange event handler to do something. Getting any size there would make an additional reflow.

> GDK delays requesting a draw until there are no events remaining in the
> queue, so drawing in mid-transition would usually be rare.  OTMC likely
> defeats that logic, but doesn't the fullscreen animation usually cover the
> window during transition?

Yes, but resize reflow + fullscreen reflow could be slow. It could take several hundreds of ms for each full reflow, and we cannot cover things for that long.
(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> I'm uncomfortable with trying to enforce this.
> If all three events are received, then I'd expect OnSizeAllocate() to be
> called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at
> any time wrt those.
> However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent()
> will be called.

In which case those two will not be called? Probably the window can occupy the whole screen without being in fullscreen state? I guess comparing bounds against screen rect and original bounds is even more unreliable... I have no idea how can I fix this then, if those signals are not reliable... Any suggestion?
Flags: needinfo?(karlt)
If GDK requests a draw only until there is no event in the queue, can we probably handle that at the very beginning of the first OnExposeEvent after the window state changes?
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #13)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> In which case those two will not be called? Probably the window can occupy
> the whole screen without being in fullscreen state?

Yes, a maximized window may be fullscreen under some window managers.
Conversely, some window managers may not make the window cover the whole screen when switching to fullscreen mode.

... I have no
> idea how can I fix this then, if those signals are not reliable... Any
> suggestion?

I don't have any good ideas, which is why it seems easier to accept possible
double reflow.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #14)
> If GDK requests a draw only until there is no event in the queue, can we
> probably handle that at the very beginning of the first OnExposeEvent after
> the window state changes?

That's essentially how things used to work.  OnExposeEvent would force the
needed reflow before paint.  That had some problems, but we usually survived.
JS would run in response to the expose signal, which could run other events.
If there were too many input events, the expose event would never arrive
because we stayed busy.

With OMTC, I guess we now have the refresh driver forcing reflow before
OnExposeEvent requests a redraw?  If there are pending resizes when that
reflow is forced, then we have the same problem with performing an unnecessary
reflow.  The issue doesn't seem specific to fullscreen changes.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #12)
> It is a real issue, since we want to ensure our size being stable after we
> dispatch fullscreenchange event, so that the page can rely on the size at
> that moment and we do a big reflow rather than doing two reflows for resize
> and fullscreen separately.

What forces a reflow?

Is there a reason why handling the size change before the fullscreen change is
better than the reverse?

The page may reorganize in response to a size change before a fullscreen change.
Flags: needinfo?(karlt)
The order of resize and fullscreenchange events is not particularly important, but any intermittent state of the page not being observable inside those event handlers is important. If we expose the intermittent state, we would likely waste a reflow, because functions like Element.getClientBoundingRect(), or even Window.innerWidth need a flush.

We currently have code to freeze the refresh driver and block any resize request during fullscreen change. This flag is set before calling MakeFullscreen, and unset inside FullscreenChanged callback, which means resize happens before fullscreen change finishes would not trigger any resize reflow itself, so that the intermittent state is not observable.

I want this to be consistent on GTK+ like on other platforms.

Other platforms ensure this actually because they call Resize() to enter fullscreen, which change mBounds directly. So they have the correct geometry synchronously. But I guess it is not quite doable with GTK+, and we would need to play with the signals anyway...
No... even OnExposeEvent doesn't work... I have no idea what can I do then...
I want to disable all fullscreen tests which have intermittent on Linux then... I totally have no idea how can I fix it...
From this try run result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d22718c9fd8d&filter-classifiedState=unclassified&selectedJob=18017107

It seems:
1. expose-event could happen before size-allocate after window-state-event [Linux x64 asan M-e10s(2)]
2. expose-event may not happen at all after window-state-event [M(bc7) M-e10s(bc7)]

I have no idea what can I do then... karlt, suggestions?
Flags: needinfo?(karlt)
It seems expose-event not happening after window-state-even happens on other tests as well, like M(2) on Linux opt/debug and Linux64 ASAN.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #19)
> From this try run result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d22718c9fd8d&filter-
> classifiedState=unclassified&selectedJob=18017107
> 
> It seems:
> 1. expose-event could happen before size-allocate after window-state-event
> [Linux x64 asan M-e10s(2)]
> 2. expose-event may not happen at all after window-state-event [M(bc7)
> M-e10s(bc7)]
> 
> I have no idea what can I do then... karlt, suggestions?

My best idea is to use gdk_frame_clock_request_phase() [1] on receipt of window state change and suppress refresh driver ticks until that phase is received.

That way, we know that a signal will be received (as long as the window is not destroyed) and I'm assuming we can pick a phase that will happen after the size change, if there is one pending.  If the window manager is performing an animation, then it may take an unknown length of time for the size change to be received, and so the size change could still be after the frame clock phase signal.

The idea needs some investigation to see whether it is sane, and it is only available from gtk 3.8.  Tests run against 3.4, which does not have this function.

I'm sceptical re whether it is worth the effort.

https://developer.gnome.org/gdk3/stable/GdkFrameClock.html#gdk-frame-clock-request-phase
Flags: needinfo?(karlt)
It's worth because we want to ensure the consistency across platforms, and we want to fix all linux-specific intermittents related to fullscreen. Disabling them means we lose coverage of all pointerlock tests and a large part of fullscreen tests for Linux.
Filed a bug for GTK+ to see whether their developers have any suggestions.
Priority: -- → P3
Whiteboard: tpi:+
You need to log in before you can comment on or make changes to this bug.