+++ This bug was initially created as a clone of Bug #746042 +++

Ray Strode [halfline] [reporter] [developer] 2015-03-20 14:11:32 EDT

Created attachment 299976 [details] [review] [review]
kms-winsys: fix leaks in _cogl_winsys_egl_display_setup

The code fails to free up the display when setup fails.

[reply] [−] Comment 13 Ray Strode [halfline] [reporter] [developer] 2015-03-20 14:11:39 EDT

Created attachment 299977 [details] [review] [review]
kms-winsys: fix another leak in _cogl_winsys_egl_display_setup

The code fails to free up the drmModeRes structure it queries for.

 Rui Matos [developer] 2015-03-23 09:03:23 EDT

Review of attachment 299976 [details] [review] [review]:

What I'd really like to see is removing the ad-hoc output initialization (mirror, etc) that's not really needed.

::: cogl/winsys/cogl-winsys-egl-kms.c
@@ +655,2 @@
   if (!output0)
+    goto out;

This one was clearly wrong since we would crash later in output_free() when called from _display_destroy()

@@ +744,3 @@
+        output_free (kms_renderer->fd, output1);
+
+      _cogl_winsys_egl_display_destroy (display);

This call seems wrong though. If this function returns FALSE, the calling code in cogl_context_new() does cogl_object_unref (display) which, IIUC, ends up doing winsys->display_destroy(), i.e. the calling code is responsible to destroy this object, not us.

[reply] [−] Comment 20 Rui Matos [developer] 2015-03-23 09:04:25 EDT

Review of attachment 299977 [details] [review] [review]:

right

 Ray Strode [halfline] [reporter] [developer] 2015-03-23 09:57:29 EDT

(In reply to Rui Matos from comment #19)
> Review of attachment 299976 [details] [review] [review] [review]:
> 
> What I'd really like to see is removing the ad-hoc output initialization
> (mirror, etc) that's not really needed.
Alright let's move discussion to a new bug, the only reason these fixes snuck into this bug was because I was going to use the leaked resources object for vblank handling. now we're not doing the vblank handling, so the leak fixes are pretty independent of this bug.  will clone.

> This call seems wrong though. If this function returns FALSE, the calling
> code in cogl_context_new() does cogl_object_unref (display) which, IIUC,
> ends up doing winsys->display_destroy(), i.e. the calling code is
> responsible to destroy this object, not us.
Indeed, reading fail there. before deciding to put the destroy call in I read the top half of the function, and decided it was idempotent.  of course the bottom half of the function isn't...