Re: Release planning for 0.14.2

On Sat, Mar 07, 2015 at 10:36:42AM +0100, Uli Schlachter wrote:
> Hi,
> 
> @Bryce: I would suggest reverting this

Hi Uli, thanks for the suggestion, I've reverted the change for the
release.
 
> Am 05.03.2015 um 01:20 schrieb Henry (Yu) Song:
> [...]
> > Following patch fixes a memory leak in xlib surface.
> 
> this might be a stupid question, but: Which memory leak exactly? And why doesn't
> the commit message say so?
> 
> All that _XEReadEvents() does is flushing the output buffer and reading in
> events, appending them to the internal queue of pending events (_XReadEvents ->
> poll_for_response() -> poll_for_event(), then back to _XReadEvents() ->
> handle_response() -> _XEnq()). So no memory leak here.
> 
> Your code however calls _XDeq() on the first event on the queue. This function
> just removes the event from the list, but does not free it. So as I see it, this
> function is *introducing* a memory leak. And it even does so in plain sight,
> without having any other effect.
> 
> (And also I have no idea why it would be a good idea to drop the whole event
> queue. Pretty much none of the events in there belong to us. Let's imagine an
> application mapping a window and then calling into cairo. Cairo will happily
> drop the MapNotify event and also any Expose events which means that the
> application will possibly not draw its window.)
> 
> Could you please enlighten me?
> 
> > From cdd185b6143876a4ce320e4f3a3f59d3fc7a5813 Mon Sep 17 00:00:00 2001
> > From: Henry Song <[email protected]>
> > Date: Wed, 4 Mar 2015 10:06:36 -0800
> > Subject: [PATCH] xlib: Remove queued event from _XReadEvents
> 
> This sounds like it should only remove the events that _XReadEvents() just
> enqueued. This would still be a stupid idea, but the patch drops the whole
> queue, no matter if part of it existed before _XReadEvents() was called.
> 
> > ---
> >  src/cairo-xlib-surface-shm.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/cairo-xlib-surface-shm.c b/src/cairo-xlib-surface-shm.c
> > index fa7d3eb..9b13505 100644
> > --- a/src/cairo-xlib-surface-shm.c
> > +++ b/src/cairo-xlib-surface-shm.c
> > @@ -670,6 +670,7 @@ _cairo_xlib_shm_surface_flush (void *abstract_surface, unsigned flags)
> >      cairo_xlib_shm_surface_t *shm = abstract_surface;
> >      cairo_xlib_display_t *display;
> >      Display *dpy;
> > +    _XQEvent *qev;
> >      cairo_status_t status;
> >  
> >      if (shm->active == 0)
> > @@ -694,6 +695,10 @@ _cairo_xlib_shm_surface_flush (void *abstract_surface, unsigned flags)
> >      while (! seqno_passed (shm->active, LastKnownRequestProcessed (dpy))) {
> >  	LockDisplay(dpy);
> >  	_XReadEvents(dpy);
> > +	while (dpy->head) {
> > +	    qev = dpy->head;
> > +	    _XDeq (dpy, NULL, qev);
> > +	}
> >  	UnlockDisplay(dpy);
> >      }
> >  
> > 
> 
> -- 
> "Every once in a while, declare peace. It confuses the hell out of your enemies"
>  - 79th Rule of Acquisition
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo