Quick heads-up, Xwayland present code causing memory corruption and crashes

[Roman Gilg]

On Wed, Apr 18, 2018 at 11:25 AM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> This is a quick heads up, I was experiencing random crashes in Xwayland
> (very annoying in gnome-shell and it takes gnome-shell and the entire
> session with it). while running skype with current Xwayladn from 1.20 rc4.

Sorry for the annoyances. But I'm of course very glad you found them
before release. Thanks!

> This is due to commit 82df2ce3:
>
> Author: Roman Gilg <subdiff at gmail.com>
> Date:   Tue Aug 22 15:38:26 2017 +0200
>
>     xwayland: Avoid repeatedly looping through window ancestor chain
>
>     Calling xwl_window_from_window means looping through the window ancestor
>     chain whenever it is called on a child window or on an automatically
>     redirected window.
>
>     Since these properties and the potential ancestor's xwl_window are
> constant
>     between window realization and unrealization, we can omit the looping by
>     always putting the respective xwl_window in the Window's private field
> on
>     its realization. If the Window doesn't feature an xwl_window on its own,
>     it's the xwl_window of its first ancestor with one.
>
>     Signed-off-by: Roman Gilg <subdiff at gmail.com>
>     Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> I think the assumption there is invalid in the case of a UnrealizeTree().

In UnrealizeTree() it seems indeed the tree is unrealized top to
bottom. What's the reason for doing it this way? Naively I would think
that unrealizing a window tree should run in opposed direction to
realizing one.

> Reverting that patch (and adjusting xwayland-present.c accordingly) solves
> the “invalid read” problem (as we don;t point to freed memoery anymore), but
> unfortunately that breaks xwayland-present logic that now leaves pending
> buffers and events after the unrealize()...

Reverting the patch and always calling xwl_window_from_window in the
present code (of course this means that we loop quite often in the
xwayland-present code) should work, since xwl_present_cleanup is
called in xwl_unrealize_window also already for the top parent window.

To fix the logic in xwl_present_cleanup we would need comparison
between event->present_window and xwl_window->present_window. And
destroy the xwl_window->present_frame_callback also if cleanup is
called on the top parent window.

So should we do this or try to always unrealize bottom-to-top in
UnrealizeTree()?