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

Submitted by Olivier Fourdan on Sept. 14, 2018, 7:09 a.m.

Details

Message ID 20180914070911.23603-1-ofourdan@redhat.com
State New
Headers show
Series "xwayland: Avoid assert failure in flips_stop()" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Olivier Fourdan Sept. 14, 2018, 7:09 a.m.
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=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=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 <ofourdan@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(+)

Patch hide | download patch | download mbox

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;

Comments

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.
Hi Roman,

On Fri, Sep 21, 2018 at 11:53 AM Roman Gilg <subdiff@gmail.com> wrote:
>
> 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(+)