1777664 - Opening popup menu freezes whole browser

Status: RESOLVED FIXED

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

Description

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

Opening popup menu freezes whole browser.

Comment 1

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

Attachment: Screencast

Seems to be specific to my environment/extensions installed.

Comment 2

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

This is caused by frozen refresh driver - still investigating why we see it. Looks like a race condition while we close the popup / pause compositor but we still render to the window.

Comment 3

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

The frozen thick driver causes that we're not proceed events for the popup so we don't set input region and don't configure the new popup.

Comment 4

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

This is a problem with CompositorBridgeChild::SendPause() in widget code - we call that in case we hide a window to stop rendering to it (perhaps?). But that causes issue when one nsRefreshDriver is used by more WebRenderLayerManager. The main UI window uses the same nsRefreshDriver as a popup (both have different WebRenderLayerManager but they use the same nsRefreshDriver), we end up with complete UI freeze and log is flooded with 'Over max pending transaction limit when trying to paint, skipping' when the popup is closed and CompositorBridgeChild::SendPause() is called.

So there's a question if it's a good idea to use CompositorBridgeChild::SendPause() here.

Comment 5

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

Explanation by mstange:

CompositorBridgeChild::SendPause() is intended to avoid compositing in the minimized window, for example from a loading spinner animation
these animations are driven by the compositor and can continue even if the main thread doesn't send any further paints
but bypassing the refresh driver for this seems like a bad design - one part of the system is saying "don't composite" and the other part of the system is saying "I'm expecting you to composite and I won't do anything until you do"

Comment 6

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

So we need to remove SendPause() here. We can use SendPause()/SendResumeAsync() only when we wait for some external event like window creation or so and we need to pair every SendPause() with SendResumeAsync() to make sure we don't block rendering of another window as nsRefreshDriver can be shared.

Comment 7

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

Attachment: Bug 1777664 [Linux] Replace CleanLayerManagerRecursive() with DestroyLayerManager() as they're the same r?emilio

Comment 8

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

Attachment: Bug 1777664 [Linux] Don't pause/resume compositor by nsWindow::SetCompositorWidgetDelegate() r?emilio

nsWindow::SetCompositorWidgetDelegate() should not control compositor state as nsWindow::SetCompositorWidgetDelegate() itself is called by nsBaseWidget::CreateCompositor()/nsBaseWidget::DestroyCompositor().

In this patch we remove compositor pause/resume from nsWindow::SetCompositorWidgetDelegate() and update only GdkWindow/XWindow stored in remote widget.

Depends on D152686

Comment 9

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

Attachment: Bug 1777664 [Linux] Create nsWindow with initially paused compositor as we don't have valid GdkWindow/XWindow yet r?emilio

Depends on D152687

Comment 10

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

Attachment: Bug 1777664 [Wayland] Don't pause compositor when nsWindow is hidden r?emilio

We should not pause compositor as compositing is driven by refresh driver and compositor pause leads to browser freeze.

Depends on D152688

Comment 11

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

Attachment: Bug 1777664 [Wayland] Remove EnableRenderingToWindow()/DisableRenderingToWindow/()/ResumeCompositorHiddenWindow() r?emilio

Enable composition only once when nsWindow is created and don't stop compositon until a window is destroyed.
Remove COMPOSITOR_PAUSED_MISSING_WINDOW as we don't manage compositing for hidden windows any more.

Depends on D152689

Comment 12

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

Attachment: Bug 1777664 [Wayland] Split ShowWaylandWindow() to ShowWaylandPopupWindow()/ShowWaylandToplevelWindow() and enable/disable VSync for visible/hidden toplevel windows r?emilio

Depends on D152690

Comment 13

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

Attachment: Bug 1777664 [Wayland] Add more logging to EGL window management r?emilio

Depends on D152691

Comment 14

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

Attachment: Bug 1777664 [Wayland] Handle mCompositorWidgetDelegate changes in nsWindow r?emilio

When mCompositorWidgetDelegate is changed we need to recreate compositor and Wayland vsync. In this patch we do:

• Make mCompositorWidgetDelegate hard requirement for Wayland vsync source to avoid hidden failure.
• Stop Wayland vsync source at nsWindow::SetCompositorWidgetDelegate() when we're going to release or change mCompositorWidgetDelegate.
• Start Wayland vsync source at nsWindow::SetCompositorWidgetDelegate() when there's a new mCompositorWidgetDelegate to make sure vsync uses
  recent one.
• Set new window config (XWindow, shape, widget) and resume compositor at nsWindow::SetCompositorWidgetDelegate() when new mCompositorWidgetDelegate is assigned to widget.
• As it's possible that mCompositorWidgetDelegate is missing in nsWindow::ConfigureGdkWindow() use moz_container_wayland_add_initial_draw_callback() to call mCompositorWidgetDelegate->EnableRendering() & WaylandStartVsync(). That ensures we won't silently fail to set up nsWindow for rendering.

Depends on D152692

Comment 15

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

Attachment: Bug 1777664 [Linux] Destory layer manager at nsWindow::ReleaseGdkWindow() r?emilio

Depends on D152693

Comment 16

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c201cd1a64628256974486fe9d0be3732c7f0fff

Comment 17

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

Attachment: Bug 1777664 [Wayland] Remove print of this from logging r?emilio

Depends on D152694

Comment 18

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

Attachment: Bug 1777664 [Wayland] Assert when ShowWaylandToplevelWindow() is called on popup r?emilio

Depends on D152730

Comment 19

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

Attachment: Bug 1777664 [Linux] Remove redundant logging from GtkCompositorWidget r?emilio

Depends on D152731

Comment 20

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

try: https://treeherder.mozilla.org/jobs?repo=try&revision=44ad6a5026a1942df6931d75d49035f55a8906ae

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fa3652858e9
[Linux] Replace CleanLayerManagerRecursive() with DestroyLayerManager() as they're the same r=emilio
https://hg.mozilla.org/integration/autoland/rev/396ad8e5d024
[Linux] Don't pause/resume compositor by nsWindow::SetCompositorWidgetDelegate() r=emilio
https://hg.mozilla.org/integration/autoland/rev/8a1503365add
[Linux] Create nsWindow with initially paused compositor as we don't have valid GdkWindow/XWindow yet r=emilio
https://hg.mozilla.org/integration/autoland/rev/0531e0fd8b47
[Wayland] Don't pause compositor when nsWindow is hidden r=emilio
https://hg.mozilla.org/integration/autoland/rev/66c3420f8f7f
[Wayland] Remove EnableRenderingToWindow()/DisableRenderingToWindow/()/ResumeCompositorHiddenWindow() r=emilio
https://hg.mozilla.org/integration/autoland/rev/5dcb07911ae4
[Wayland] Split ShowWaylandWindow() to ShowWaylandPopupWindow()/ShowWaylandToplevelWindow() and enable/disable VSync for visible/hidden toplevel windows r=emilio
https://hg.mozilla.org/integration/autoland/rev/d4beab20f7e5
[Wayland] Add more logging to EGL window management r=emilio
https://hg.mozilla.org/integration/autoland/rev/0a19e7857a10
[Wayland] Handle mCompositorWidgetDelegate changes in nsWindow r=emilio
https://hg.mozilla.org/integration/autoland/rev/af89aa9ca4fb
[Linux] Destory layer manager at nsWindow::ReleaseGdkWindow() r=emilio
https://hg.mozilla.org/integration/autoland/rev/07d98fdfda1e
[Wayland] Remove print of this from logging r=emilio
https://hg.mozilla.org/integration/autoland/rev/c79511446521
[Wayland] Assert when ShowWaylandToplevelWindow() is called on popup r=emilio
https://hg.mozilla.org/integration/autoland/rev/e081a91279cc
[Linux] Remove redundant logging from GtkCompositorWidget r=emilio

Comment 22

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

This one also depends on Bug 1752992 - we need to destroy compositor when popup window (usually tooltip) is closed to make sure the refresh driver is not blocked.

It's because we close the tooltip popups from widget code and it's not deleted by layout so compositing is still enabled/refres driver is ticking but nothing is painted and that causes whole UI freeze.

Bug 1619246 (disabled wayland vsync) may be also a factor here and can help if we enable/disable vsync for popups.

Comment 23

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0fa3652858e9
https://hg.mozilla.org/mozilla-central/rev/396ad8e5d024
https://hg.mozilla.org/mozilla-central/rev/8a1503365add
https://hg.mozilla.org/mozilla-central/rev/0531e0fd8b47
https://hg.mozilla.org/mozilla-central/rev/66c3420f8f7f
https://hg.mozilla.org/mozilla-central/rev/5dcb07911ae4
https://hg.mozilla.org/mozilla-central/rev/d4beab20f7e5
https://hg.mozilla.org/mozilla-central/rev/0a19e7857a10
https://hg.mozilla.org/mozilla-central/rev/af89aa9ca4fb
https://hg.mozilla.org/mozilla-central/rev/07d98fdfda1e
https://hg.mozilla.org/mozilla-central/rev/c79511446521
https://hg.mozilla.org/mozilla-central/rev/e081a91279cc