[Mesa-dev] [PATCH] egl/wayland: do not leak wl_buffer when it is locked

[Juan A. Suarez Romero]

On Wed, 2018-08-08 at 17:49 +0100, Daniel Stone wrote:
> Hi Juan,
> 
> On Wed, 8 Aug 2018 at 17:40, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > On Wed, 2018-08-08 at 17:21 +0100, Daniel Stone wrote:
> > > On Thu, 2 Aug 2018 at 10:02, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > > > If color buffer is locked, do not set its wayland buffer to NULL;
> > > > otherwise it can not be freed later.
> > > 
> > > It can: see the 'if (i == ARRAY_SIZE(...))' branch inside wl_buffer_release.
> > 
> > I think I didn't explain wrongly :)
> > 
> > If color buffer is locked, we set color_buffer.wl_buffer to NULL, and thus we
> > can't free wl_buffer later.
> 
> If a surface is resized, we will orphan all its wl_buffers by clearing
> their pointers to NULL, hence losing the ability to directly free
> them. But when a new buffer has been attached and displayed, a
> 'release' event will be delivered for the old buffer (handled by
> wl_buffer_release as an event listener), which will detect that the
> wl_buffer is not in the list and should be immediately destroyed.
> 
> In fact, I have to rescind my R-b since I'm pretty sure this breaks resizing:
>   - buffer 1 is allocated with original size, committed to server
> (locked == true)
>   - user resizes surface, release_buffers() is called but leaves
> wl_buffer intact in color_buffers list
>   - buffers 2..4 are allocated with new size and committed to server
> (locked == true)
>   - release event for buffer 1 is delivered, locked = false
>   - get_back_bo() finds buffer 1 has a wl_buffer and is not locked,
> but dri_image is NULL so a new image is created
>   - swap_buffers_with_damage() finds buffer 1 (with new DRIimage)
> still has wl_buffer (old) and attaches that buffer
> 
> So we need to either find a way to still destroy the wl_buffer inside
> wl_buffer_release(), or at least do it later, e.g. in get_back_bo().
> 


Investigating a bit more, I found the following:


1) dri2_wl_release_buffers() is invoked; one of the color_buffers is locked, so
the proper wl_buffer is not destroyed, but set to NULL. This will be destroyed
later through the release event.

2) Test is finished, and dri2_wl_destroy_surface() is invoked. This function
destroys all wl_buffers, no matter if they are locked or not. But can't destroy
the above one, because it is NULL. 
More important, dri2_wl_destroy_surface() frees the surface itself, including
the event queue [wl_event_queue_destroy(dri2_surf->wl_queue);]. So when the
release event for the wl_buffer, there's no wl_queue.


A quick check of what would happen if the queue is not destroyrf reveald that
the tests pass without any problem. Of course, I don't think this is a proper
solution, as we would be leaking the queue itself.

	J.A.




> Cheers,
> Daniel
>