On Wed, May 28, 2014 at 12:07:40PM +0100, Frank Binns wrote: > > On 28/05/14 08:14, Chris Wilson wrote: > >If we close the Window prior to completing all the flips, we free the > >vblank pointer but leave screen_priv->flip_pending dangling. This leads > >to a host of invalid reads/writes as it is accessed from many locations, > >e.g.: > > > >==20302== Invalid write of size 4 > >==20302== at 0x81A8FD5: present_flip_destroy (present.c:879) > >==20302== by 0x81A6A2A: present_close_screen (present_screen.c:60) > >==20302== by 0x8147A5A: CursorCloseScreen (cursor.c:187) > >==20302== by 0x81A3DDA: AnimCurCloseScreen (animcur.c:106) > >==20302== by 0x8142F7D: compCloseScreen (compinit.c:85) > >==20302== by 0x80E6D2A: VidModeClose (xf86VidMode.c:111) > >==20302== by 0x496CF23: glxCloseScreen (glxscreens.c:187) > >==20302== by 0x808186F: dix_main (main.c:349) > >==20302== by 0x80D2FA1: main (stubmain.c:34) > >==20302== Address 0x67d0780 is 104 bytes inside a block of size 108 free'd > >==20302== at 0x4007BCD: free (in > >/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) > >==20302== by 0x81A90AC: present_vblank_destroy (present.c:911) > >==20302== by 0x81A6ADF: present_free_window_vblank (present_screen.c:79) > >==20302== by 0x81A6C2C: present_destroy_window (present_screen.c:116) > >==20302== by 0x8145796: compDestroyWindow (compwindow.c:608) > >==20302== by 0x8176B6B: DbeDestroyWindow (dbe.c:1318) > >==20302== by 0x80B5A98: FreeWindowResources (window.c:910) > >==20302== by 0x80B5D7B: DeleteWindow (window.c:978) > >==20302== by 0x80A9C85: doFreeResource (resource.c:873) > >==20302== by 0x80AA5F8: FreeClientResources (resource.c:1139) > >==20302== by 0x807BD5E: CloseDownClient (dispatch.c:3384) > >==20302== by 0x80740B5: Dispatch (dispatch.c:406) > > > >Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> > >--- > > present/present_screen.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > >diff --git a/present/present_screen.c b/present/present_screen.c > >index b475d03..0a525a8 100644 > >--- a/present/present_screen.c > >+++ b/present/present_screen.c > >@@ -72,11 +72,14 @@ static void > > present_free_window_vblank(WindowPtr window) > > { > > present_window_priv_ptr window_priv = present_window_priv(window); > >+ present_screen_priv_ptr screen_priv = > >present_screen_priv(window->drawable.pScreen); > >+ present_vblank_ptr flip_pending = screen_priv->flip_pending; > > present_vblank_ptr vblank, tmp; > > xorg_list_for_each_entry_safe(vblank, tmp, &window_priv->vblank, > > window_list) { > > present_abort_vblank(window->drawable.pScreen, vblank->crtc, > > vblank->event_id, vblank->target_msc); > >- present_vblank_destroy(vblank); > >+ if (vblank != flip_pending) > >+ present_vblank_destroy(vblank); > > } > > } > > Hi Chris, > Doesn't this result in the vblank structure never being cleaned up > because the vblank event, which would normally result in it being > destroyed, gets ignored due to present_abort_vblank() being called > a couple of lines above?
True, I was thinking it would still get cleaned up through present_flip_notify, which now never occurs. > I actually submitted a similar patch to this a month ago but never > got around to addressing Keith's review comments. See patch 2 > from the following series: > http://lists.x.org/archives/xorg-devel/2014-April/042129.html > > I've now gone back to looking at this so that I can address the > comments and resubmit the patches. Ok, I'll see if these make everything quiet here as well. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel