[RFC PATCH xserver] xwayland: Avoid assert failure in flips_stop()

[Olivier Fourdan]

On `ClipNotify()`, `present_clip_notify()` will possibly end up issuing
a `flips_stop()` if `check_flip()` returns `FALSE`.

`present_wnmd_check_flip()` however can return `FALSE` in a variety of
cases, before eventually checking with the driver's `check_flip2()`
which in the case of `xwl_present_check_flip2()` makes sure that
`xwl_window->present_window` is not `NULL`.

Hence, if one of the preliminary conditions is not satisfied in
`present_wnmd_check_flip()`, we may end up calling Xwayland's
`xwl_present_flips_stop()` even though `xwl_window->present_window` is
'NULL', which will trigger an assertion failure and consequently a crash
of Xwayland.

A backtrace of such a case looks like:

#0 __GI_raise (sig=***@entry=6)
#1 __GI_abort () at abort.c:79
#2 OsAbort () at utils.c:1350
#3 AbortServer () at log.c:877
#4 FatalError () at log.c:1015
#5 OsSigHandler () at osinit.c:156
#6 <signal handler called>
#7 __GI_raise (sig=***@entry=6)
#8 __GI_abort () at abort.c:79
#9 __assert_fail_base () at assert.c:92
#10 __GI___assert_fail () at assert.c:101
#11 xwl_present_flips_stop () at xwayland-present.c:521
#12 present_wnmd_flips_stop () at present_wnmd.c:159
#13 present_wnmd_check_flip_window () at present_wnmd.c:332
#14 present_clip_notify () at present_screen.c:203
#15 compClipNotify () at compwindow.c:317
#16 miComputeClips () at mivaltree.c:478
#17 miValidateTree () at mivaltree.c:681
#18 MapWindow () at window.c:2699
#19 ReparentWindow () at window.c:2600
#20 ProcReparentWindow () at dispatch.c:829
#21 Dispatch () at dispatch.c:478
#22 dix_main () at main.c:276
#23 __libc_start_main () at ../csu/libc-start.c:308
#24 _start ()

In this case, a forensic examination of the core file showed that
`present_wnmd_check_flip()` returned `FALSE` because
`window->redirectDraw` was `RedirectDrawManual` and not the expected
`RedirectDrawNone`.

Signed-off-by: Olivier Fourdan <***@redhat.com>
---
See: https://lists.x.org/archives/xorg-devel/2018-September/057566.html

hw/xwayland/xwayland-present.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 316e04443..f77dc4d15 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -518,6 +518,9 @@ xwl_present_flips_stop(WindowPtr window)
if (!xwl_window)
return;

+ if (xwl_window->present_window == NULL)
+ return;
+
assert(xwl_window->present_window == window);

xwl_window->present_window = NULL;

[Roman Gilg]

Great detailed analysis in the backtrace! :)

What confused me at first was that the present_wnmd_flips_stop function is
called at all in this state because it should only be called when at least
one flip has been done and in this case xwl_window->present_window must
have been set to the presenting (child) window.

I believe now the root problem is that the window did in fact some flips in
the past, but on the reparent operation a new parent window with a new
xwl_window struct is set, which then has a different present_window value.
That means when reparenting of a child window with flips the
xwl_window->present_window values must be updated on the old parent (to
NULL) and on the new parent (to the new child window). Or more generic this
must be done on any unmap/map operation.

One extra case must be considered: if there is already a different child
window doing flips on the new parent window the reparented child window
must be instructed to stop its flips.

[Olivier Fourdan]

Hi Roman,

Post by Roman Gilg
Great detailed analysis in the backtrace! :)
What confused me at first was that the present_wnmd_flips_stop function
is called at all in this state because it should only be called when at
least one flip has been done and in this case xwl_window->present_window
must have been set to the presenting (child) window.
I believe now the root problem is that the window did in fact some flips
in the past, but on the reparent operation a new parent window with a
new xwl_window struct is set, which then has a different present_window
value. That means when reparenting of a child window with flips the
xwl_window->present_window values must be updated on the old parent
(to NULL) and on the new parent (to the new child window). Or more
generic this must be done on any unmap/map operation.
One extra case must be considered: if there is already a different
child window doing flips on the new parent window the reparented child
window must be instructed to stop its flips.

So the following two patches are my attempt at implementing your suggestion,
but I am not familiar with Present so this is a bit of a shot in the dark
for me, also because that issue is quite difficult to reproduce.

Those patches have been barely tested, expect dragons!

Anyway, please let me know if that's what you had in mind...

Cheers,
Olivier

Olivier Fourdan (2):
present: cancel flip on reparenting
xwayland: update Xwayland present window on reparent

hw/xwayland/xwayland-present.c | 16 ++++++++++++++++
hw/xwayland/xwayland.c | 23 +++++++++++++++++++++++
hw/xwayland/xwayland.h | 2 ++
present/present_priv.h | 3 +++
present/present_scmd.c | 1 +
present/present_screen.c | 16 ++++++++++++++++
present/present_wnmd.c | 17 +++++++++++++++++
7 files changed, 78 insertions(+)

[Olivier Fourdan]

If a window is reparented and the new parent already has a child
flipping, we need to cancel the flipping on the reparented window.

Install a new ReparentWindow handler in present screen with hooks in
wnmd implementation to check if the new parent has flip pending or
active in which case we cancel flip on the reparented window.

Signed-off-by: Olivier Fourdan <***@redhat.com>
---
present/present_priv.h | 3 +++
present/present_scmd.c | 1 +
present/present_screen.c | 16 ++++++++++++++++
present/present_wnmd.c | 17 +++++++++++++++++
4 files changed, 37 insertions(+)

diff --git a/present/present_priv.h b/present/present_priv.h
index f62456755..668322416 100644
--- a/present/present_priv.h
+++ b/present/present_priv.h
@@ -106,6 +106,7 @@ typedef Bool (*present_priv_check_flip_ptr)(RRCrtcPtr crtc,
PresentFlipReason *reason);
typedef void (*present_priv_check_flip_window_ptr)(WindowPtr window);
typedef Bool (*present_priv_can_window_flip_ptr)(WindowPtr window);
+typedef void (*present_priv_reparent_window_ptr)(WindowPtr pWin);

typedef int (*present_priv_pixmap_ptr)(WindowPtr window,
PixmapPtr pixmap,
@@ -147,6 +148,7 @@ struct present_screen_priv {
ConfigNotifyProcPtr ConfigNotify;
DestroyWindowProcPtr DestroyWindow;
ClipNotifyProcPtr ClipNotify;
+ ReparentWindowProcPtr ReparentWindow;

present_vblank_ptr flip_pending;
uint64_t unflip_event_id;
@@ -171,6 +173,7 @@ struct present_screen_priv {
present_priv_check_flip_ptr check_flip;
present_priv_check_flip_window_ptr check_flip_window;
present_priv_can_window_flip_ptr can_window_flip;
+ present_priv_reparent_window_ptr reparent_window;

present_priv_pixmap_ptr present_pixmap;
present_priv_create_event_id_ptr create_event_id;
diff --git a/present/present_scmd.c b/present/present_scmd.c
index 0803a0c0b..a3ca16333 100644
--- a/present/present_scmd.c
+++ b/present/present_scmd.c
@@ -828,6 +828,7 @@ present_scmd_init_mode_hooks(present_screen_priv_ptr screen_priv)

screen_priv->abort_vblank = &present_scmd_abort_vblank;
screen_priv->flip_destroy = &present_scmd_flip_destroy;
+ screen_priv->reparent_window = NULL;
}

Bool
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..f3f2ddef9 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
}

+static void
+present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ ScreenPtr screen = pWin->drawable.pScreen;
+ present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+
+ if (screen_priv->reparent_window)
+ screen_priv->reparent_window(pWin);
+
+ unwrap(screen_priv, screen, ReparentWindow)
+ if (screen->ReparentWindow)
+ screen->ReparentWindow(pWin, pPriorParent);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
+}
+
static Bool
present_screen_register_priv_keys(void)
{
@@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
wrap(screen_priv, screen, ConfigNotify, present_config_notify);
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);

dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);

diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 8f3836440..0c49a3507 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
(*screen_priv->wnmd_info->flush) (window);
}

+static void
+present_wnmd_reparent_window(WindowPtr pWin)
+{
+ present_window_priv_ptr parent_window_priv;
+
+ parent_window_priv = present_window_priv(pWin->parent);
+ if (!parent_window_priv)
+ return;
+
+ /* If the new parent window already has a child flipping, cancel the
+ * flip on the reparented window
+ */
+ if (parent_window_priv->flip_pending || parent_window_priv->flip_active)
+ present_wnmd_cancel_flip(pWin);
+}
+
void
present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
{
@@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)

screen_priv->abort_vblank = &present_wnmd_abort_vblank;
screen_priv->flip_destroy = &present_wnmd_flip_destroy;
+ screen_priv->reparent_window = &present_wnmd_reparent_window;
}

[Roman Gilg]

I'm a bit unsure on that one. I thought there is no cleanup code
necessary in Present on a reparent because in theory the current
Present code alone allows clients to flip arbitrary many child windows
to a certain parent window as long as they have the same dimension as
the parent. Of course a client trying to do flips on multiple child
windows at the same time all with the same dimension as the parent can
be considered somewhat weird and should not exist in practice. So if
there is a "flipping" window being reparented to one with a child
window doing flips it could be done from Present's side without any
cleanup.

But in the Xwayland case the driver should disallow the reparented
child window doing flips, which then does a cleanup on the Present
side for this particular child window.

Post by Olivier Fourdan
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..f3f2ddef9 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
}
+static void
+present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ ScreenPtr screen = pWin->drawable.pScreen;
+ present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+
+ if (screen_priv->reparent_window)
+ screen_priv->reparent_window(pWin);
+
+ unwrap(screen_priv, screen, ReparentWindow)
+ if (screen->ReparentWindow)
+ screen->ReparentWindow(pWin, pPriorParent);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
+}
+
static Bool
present_screen_register_priv_keys(void)
{
@@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
wrap(screen_priv, screen, ConfigNotify, present_config_notify);
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);
diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 8f3836440..0c49a3507 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
(*screen_priv->wnmd_info->flush) (window);
}
+static void
+present_wnmd_reparent_window(WindowPtr pWin)
+{
+ present_window_priv_ptr parent_window_priv;
+
+ parent_window_priv = present_window_priv(pWin->parent);

The present priv struct of a parent does not carry the information of
a potentially presenting child. These are saved to the child's priv
struct directly. So currently one would need to iterate all children
and check if one of them is presenting.

Post by Olivier Fourdan
+ if (!parent_window_priv)
+ return;
+
+ /* If the new parent window already has a child flipping, cancel the
+ * flip on the reparented window
+ */
+ if (parent_window_priv->flip_pending || parent_window_priv->flip_active)

present_wnmd_cancel_flip checks these conditions already, so we do not
need to do this here as well.

Post by Olivier Fourdan
+ present_wnmd_cancel_flip(pWin);
+}
+
void
present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
{
@@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
screen_priv->abort_vblank = &present_wnmd_abort_vblank;
screen_priv->flip_destroy = &present_wnmd_flip_destroy;
+ screen_priv->reparent_window = &present_wnmd_reparent_window;
}
--
2.19.0

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info:

[Olivier Fourdan]

Hi Roman,

Post by Roman Gilg
I'm a bit unsure on that one. I thought there is no cleanup code
necessary in Present on a reparent because in theory the current
Present code alone allows clients to flip arbitrary many child windows
to a certain parent window as long as they have the same dimension as
the parent. Of course a client trying to do flips on multiple child
windows at the same time all with the same dimension as the parent can
be considered somewhat weird and should not exist in practice. So if
there is a "flipping" window being reparented to one with a child
window doing flips it could be done from Present's side without any
cleanup.
But in the Xwayland case the driver should disallow the reparented
child window doing flips, which then does a cleanup on the Present
side for this particular child window.

I'm a bit lost, those two patches were my attempt to translate into
code what you wrote im your reply previously:

https://lists.x.org/archives/xorg-devel/2018-September/057581.html

Post by Roman Gilg
I believe now the root problem is that the window did in fact some flips in the past, but on the reparent operation a new parent
window with a new xwl_window struct is set, which then has a different present_window value. That means when reparenting
of a child window with flips the xwl_window->present_window values must be updated on the old parent (to NULL) and on the
new parent (to the new child window). Or more generic this must be done on any unmap/map operation.

That would have been patch 2/2 "xwayland: update Xwayland present
window on reparent"

Post by Roman Gilg
One extra case must be considered: if there is already a different child window doing flips on the new parent window the
reparented child window must be instructed to stop its flips.

Whereas this would have been this patch 1/2 "present: cancel flip on
reparenting"

So do you mean we don't need this part and should drop this patch instead?

Post by Roman Gilg

Post by Olivier Fourdan
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..f3f2ddef9 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
}
+static void
+present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ ScreenPtr screen = pWin->drawable.pScreen;
+ present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+
+ if (screen_priv->reparent_window)
+ screen_priv->reparent_window(pWin);
+
+ unwrap(screen_priv, screen, ReparentWindow)
+ if (screen->ReparentWindow)
+ screen->ReparentWindow(pWin, pPriorParent);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
+}
+
static Bool
present_screen_register_priv_keys(void)
{
@@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
wrap(screen_priv, screen, ConfigNotify, present_config_notify);
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
+ wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);
diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 8f3836440..0c49a3507 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
(*screen_priv->wnmd_info->flush) (window);
}
+static void
+present_wnmd_reparent_window(WindowPtr pWin)
+{
+ present_window_priv_ptr parent_window_priv;
+
+ parent_window_priv = present_window_priv(pWin->parent);

The present priv struct of a parent does not carry the information of
a potentially presenting child. These are saved to the child's priv
struct directly. So currently one would need to iterate all children
and check if one of them is presenting.

Post by Olivier Fourdan
+ if (!parent_window_priv)
+ return;
+
+ /* If the new parent window already has a child flipping, cancel the
+ * flip on the reparented window
+ */
+ if (parent_window_priv->flip_pending || parent_window_priv->flip_active)

present_wnmd_cancel_flip checks these conditions already, so we do not
need to do this here as well.

Post by Olivier Fourdan
+ present_wnmd_cancel_flip(pWin);
+}
+
void
present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
{
@@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
screen_priv->abort_vblank = &present_wnmd_abort_vblank;
screen_priv->flip_destroy = &present_wnmd_flip_destroy;
+ screen_priv->reparent_window = &present_wnmd_reparent_window;
}
--
2.19.0

Cheers,
Olivier
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman

[Olivier Fourdan]

When reparenting a window, the Xwayland present window either on the
prior parent or on the new parent may need to be updated:

1. If the window is flipping, clear the present window on its old
parent.
2. If the new parent has no flipping already, set its flipping window to
the reparented window.

Install new ReparentWindow hooks in Xwayland to handle those cases.

Signed-off-by: Olivier Fourdan <***@redhat.com>
---
hw/xwayland/xwayland-present.c | 16 ++++++++++++++++
hw/xwayland/xwayland.c | 23 +++++++++++++++++++++++
hw/xwayland/xwayland.h | 2 ++
3 files changed, 41 insertions(+)

diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 316e04443..f54fe2ac4 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -558,3 +558,19 @@ xwl_present_init(ScreenPtr screen)

return present_wnmd_screen_init(screen, &xwl_present_info);
}
+
+void
+xwl_present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ struct xwl_window *xwl_window;
+
+ /* If the window is flipping, clear the present window on its old parent */
+ xwl_window = xwl_window_from_window(pPriorParent);
+ if (xwl_present_is_flipping(pWin, xwl_window))
+ xwl_window->present_window = NULL;
+
+ /* If the new parent window has no child flipping, update its present_window */
+ xwl_window = xwl_window_from_window(pWin->parent);
+ if (xwl_window && xwl_window->present_window == NULL)
+ xwl_window->present_window = pWin;
+}
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 96b4db18c..345b62f59 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -660,6 +660,26 @@ xwl_destroy_window(WindowPtr window)
return ret;
}

+static void
+xwl_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ ScreenPtr screen = pWin->drawable.pScreen;
+ struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+
+#ifdef GLAMOR_HAS_GBM
+ if (xwl_screen->present)
+ xwl_present_reparent_window(pWin, pPriorParent);
+#endif
+
+ /* Chain up with previous ReparentWindow, if any */
+ if (xwl_screen->ReparentWindow) {
+ screen->ReparentWindow = xwl_screen->ReparentWindow;
+ (*screen->ReparentWindow) (pWin, pPriorParent);
+ xwl_screen->ReparentWindow = screen->ReparentWindow;
+ screen->ReparentWindow = xwl_reparent_window;
+ }
+}
+
static void
xwl_window_post_damage(struct xwl_window *xwl_window)
{
@@ -1109,6 +1129,9 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
xwl_screen->CloseScreen = pScreen->CloseScreen;
pScreen->CloseScreen = xwl_close_screen;

+ xwl_screen->ReparentWindow = pScreen->ReparentWindow;
+ pScreen->ReparentWindow = xwl_reparent_window;
+
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;

diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 67819e178..a7c7f2aee 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -132,6 +132,7 @@ struct xwl_screen {
RealizeWindowProcPtr RealizeWindow;
UnrealizeWindowProcPtr UnrealizeWindow;
DestroyWindowProcPtr DestroyWindow;
+ ReparentWindowProcPtr ReparentWindow;
XYToWindowProcPtr XYToWindow;

struct xorg_list output_list;
@@ -451,6 +452,7 @@ void xwl_glamor_egl_make_current(struct xwl_screen *xwl_screen);
#ifdef GLAMOR_HAS_GBM
Bool xwl_present_init(ScreenPtr screen);
void xwl_present_cleanup(WindowPtr window);
+void xwl_present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent);
#endif /* GLAMOR_HAS_GBM */

#ifdef XV