<div dir="ltr">Hi Roman,<div><br></div><div>Thanks for your reply!</div><div class="gmail_extra"><br><div class="gmail_quote">On 18 April 2018 at 13:02, Roman Gilg <span dir="ltr"><<a href="mailto:subdiff@gmail.com" target="_blank">subdiff@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Apr 18, 2018 at 11:25 AM, Olivier Fourdan <<a href="mailto:ofourdan@redhat.com">ofourdan@redhat.com</a>> wrote:<br>
> This is a quick heads up, I was experiencing random crashes in Xwayland<br>
> (very annoying in gnome-shell and it takes gnome-shell and the entire<br>
> session with it). while running skype with current Xwayladn from 1.20 rc4.<br>
<br>
</span>Sorry for the annoyances. But I'm of course very glad you found them<br>
before release. Thanks!<br>
<span class=""><br>
> This is due to commit 82df2ce3:<br>
><br>
> Author: Roman Gilg <<a href="mailto:subdiff@gmail.com">subdiff@gmail.com</a>><br>
> Date:   Tue Aug 22 15:38:26 2017 +0200<br>
><br>
>     xwayland: Avoid repeatedly looping through window ancestor chain<br>
><br>
>     Calling xwl_window_from_window means looping through the window ancestor<br>
>     chain whenever it is called on a child window or on an automatically<br>
>     redirected window.<br>
><br>
>     Since these properties and the potential ancestor's xwl_window are<br>
> constant<br>
>     between window realization and unrealization, we can omit the looping by<br>
>     always putting the respective xwl_window in the Window's private field<br>
> on<br>
>     its realization. If the Window doesn't feature an xwl_window on its own,<br>
>     it's the xwl_window of its first ancestor with one.<br>
><br>
>     Signed-off-by: Roman Gilg <<a href="mailto:subdiff@gmail.com">subdiff@gmail.com</a>><br>
>     Reviewed-by: Pekka Paalanen <<a href="mailto:pekka.paalanen@collabora.co.uk">pekka.paalanen@collabora.co.<wbr>uk</a>><br>
><br>
> I think the assumption there is invalid in the case of a UnrealizeTree().<br>
<br>
</span>In UnrealizeTree() it seems indeed the tree is unrealized top to<br>
bottom. What's the reason for doing it this way? Naively I would think<br>
that unrealizing a window tree should run in opposed direction to<br>
realizing one.<br></blockquote><div><br></div><div>I don't know the reason for this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Reverting that patch (and adjusting xwayland-present.c accordingly) solves<br>
> the “invalid read” problem (as we don;t point to freed memoery anymore), but<br>
> unfortunately that breaks xwayland-present logic that now leaves pending<br>
> buffers and events after the unrealize()...<br>
<br>
</span>Reverting the patch and always calling xwl_window_from_window in the<br>
present code (of course this means that we loop quite often in the<br>
xwayland-present code) should work, since xwl_present_cleanup is<br>
called in xwl_unrealize_window also already for the top parent window.<br></blockquote><div><br></div><div>Yeah, I have a patch ready for that, with some minor tweaks as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
To fix the logic in xwl_present_cleanup we would need comparison<br>
between event->present_window and xwl_window->present_window. And<br>
destroy the xwl_window->present_frame_<wbr>callback also if cleanup is<br>
called on the top parent window.<br>
<br>
So should we do this or try to always unrealize bottom-to-top in<br>
UnrealizeTree()?</blockquote><div><br></div><div>I suspect changing the logic in UnrealizeTree() might unvocer even more problems, sp close to a release.</div><div><br></div><div>I'd rather fix that in Xwayland code instead. I'll post my patch so you can elaborate on that if you will.</div><div><br></div><div>Cheers,</div><div>Olivier</div></div></div></div>