[PATCH wayland] tests: fix memory leak

[Pekka Paalanen]

On Fri, 21 Nov 2014 11:18:33 +0100
Marek Chalupa <mchqwerty at gmail.com> wrote:

> We didn't free the struct client that we got from client_connect()
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  tests/display-test.c    | 23 +++++++++++++----------
>  tests/test-compositor.c |  1 +
>  2 files changed, 14 insertions(+), 10 deletions(-)

Hi,

how did you find these leaks?

More importantly, why doesn't our leak checker find these?
That is, the code in test-runner.c, run_test().

For one, I think those are not thread-safe. Calling exit() in a test
would bypass it too, but I'm not sure that happens.

The Valgrind output nowadays is quite messy. Even when I run a single
test from one test binary which should not fork at all, I think I see
two forks in Valgrind output: one is likely the debugger check, maybe
the other is actually a thread for the test compositor or something?

I think Valgrind is saying that there are many more leaks to plug.


In any case, this patch pushed.

Thanks,
pq

> 
> diff --git a/tests/display-test.c b/tests/display-test.c
> index aecf341..776a9c9 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -155,6 +155,14 @@ bind_seat(struct wl_client *client, void *data,
>  }
>  
>  static void
> +client_disconnect_nocheck(struct client *c)
> +{
> +	wl_proxy_destroy((struct wl_proxy *) c->tc);
> +	wl_display_disconnect(c->wl_display);
> +	free(c);
> +}
> +
> +static void
>  post_error_main(void)
>  {
>  	struct client *c = client_connect();
> @@ -170,8 +178,7 @@ post_error_main(void)
>  	/* don't call client_disconnect(c), because then the test would be
>  	 * aborted due to checks for error in this function */
>  	wl_proxy_destroy((struct wl_proxy *) seat);
> -	wl_proxy_destroy((struct wl_proxy *) c->tc);
> -	wl_display_disconnect(c->wl_display);
> +	client_disconnect_nocheck(c);
>  }
>  
>  TEST(post_error_to_one_client)
> @@ -224,8 +231,7 @@ post_error_main3(void)
>  	/* don't call client_disconnect(c), because then the test would be
>  	 * aborted due to checks for error in this function */
>  	wl_proxy_destroy((struct wl_proxy *) seat);
> -	wl_proxy_destroy((struct wl_proxy *) c->tc);
> -	wl_display_disconnect(c->wl_display);
> +	client_disconnect_nocheck(c);
>  }
>  
>  /* all the testcases could be in one TEST, but splitting it
> @@ -294,8 +300,7 @@ post_nomem_main(void)
>  	assert(wl_display_get_error(c->wl_display) == ENOMEM);
>  
>  	wl_proxy_destroy((struct wl_proxy *) seat);
> -	wl_proxy_destroy((struct wl_proxy *) c->tc);
> -	wl_display_disconnect(c->wl_display);
> +	client_disconnect_nocheck(c);
>  }
>  
>  TEST(post_nomem_tst)
> @@ -413,8 +418,7 @@ threading_post_err(void)
>  	test_set_timeout(3);
>  	pthread_join(thread, NULL);
>  
> -	wl_proxy_destroy((struct wl_proxy *) c->tc);
> -	wl_display_disconnect(c->wl_display);
> +	client_disconnect_nocheck(c);
>  }
>  
>  TEST(threading_errors_tst)
> @@ -565,8 +569,7 @@ threading_read_after_error(void)
>  	test_set_timeout(3);
>  	pthread_join(thread, NULL);
>  
> -	wl_proxy_destroy((struct wl_proxy *) c->tc);
> -	wl_display_disconnect(c->wl_display);
> +	client_disconnect_nocheck(c);
>  }
>  
>  TEST(threading_read_after_error_tst)
> diff --git a/tests/test-compositor.c b/tests/test-compositor.c
> index 3248e2d..6f86a85 100644
> --- a/tests/test-compositor.c
> +++ b/tests/test-compositor.c
> @@ -452,6 +452,7 @@ client_disconnect(struct client *c)
>  
>  	wl_proxy_destroy((struct wl_proxy *) c->tc);
>  	wl_display_disconnect(c->wl_display);
> +	free(c);
>  }
>  
>  /* num is number of clients that requests to stop display.