Subject: bug#75636: GTK memory leaks

From: Po Lu <[email-masked]>
Date: Wed, 22 Jan 2025 08:42:18 +0800

Pip Cet <[email-masked]> writes:

> "Po Lu" <[email-masked]> writes:
>
>> Pip Cet <[email-masked]> writes:
>>
>>> "Po Lu" <[email-masked]> writes:
>>>
>>>> Pip Cet <[email-masked]> writes:
>>>>
>>>>> Creating and destroying GTK frames (using gtkutil.c, not PGTK) currently
>>>>> leaks some memory, particularly when using tool bars.
>>>>>
>>>>> 1. The input "multicontext" created by:
>>>>>
>>>>>   imc = gtk_im_multicontext_new ();
>>>>>   g_object_ref (imc);
>>>>>
>>>>> is only unref'd once when destroying the frame.  This leaves the
>>>>> refcount at 1 and leaks the object.
>>>>
>>>> Strange, I was under the impression that IM contexts descended from
>>>> GInitiallyUnowned.  Evidently they don't, and as such, removing the
>>>> g_object_ref will suffice.
>>>
>>> Thanks!  I'll just do that, then?  I think it's okay only to do it on
>>> master, as it's a minor bug in the !HAVE_MPS case.
>>
>> Sounds good to me.
>>
>>>>> 2. free_frame_tool_bar is never called in my experiments, so the toolbar
>>>>> widget isn't destroyed.
>>>>
>>>> It is only supposed to be called when a frame's tool bar is disabled.
>>>> while it is x_free_frame_resources that should release all resources
>>>> when a frame is to be destroyed.
>>>
>>> Thanks!  I'll come up with a patch and discuss it further for this one,
>>> because it's not quite trivial.
>>>
>>> Pip
>>
>> Thanks.
>
> This showed up during testing, but might ultimately have been because I
> deleted the new frame too soon.  I can only reproduce it on the
> feature/igc branch, but that might be because the timing is different on
> that branch.
>
> When we create the initial tool bar, f->tool_bar_items is still Qnil.
> In that case, we don't pack the toolbar widget, but create it and
> connect signals to it.
>
> xg_free_frame_widgets does not explicitly delete the toolbar, which
> works only if it's packed and destroyed implicitly.
>
> I think this is right, and the bug isn't reproducible on the master
> branch only because of timing differences:
>
> commit 5ed4aaa986dbf75f8565f4d8cdd3d1d1b38b6142 (HEAD)
> Author: Pip Cet <[email-masked]>
>
>     Destroy GTK tool bar widget if it was never attached (bug#75636)
>     
>     * src/gtkutil.c (xg_free_frame_widgets): Call gtk_widget_destroy on an
>     unpacked toolbar widget.
>
> diff --git a/src/gtkutil.c b/src/gtkutil.c
> index 0e9dd4dfe11..97582a524da 100644
> --- a/src/gtkutil.c
> +++ b/src/gtkutil.c
> @@ -1885,6 +1885,12 @@ xg_free_frame_widgets (struct frame *f)
>        if (tbinfo)
>          xfree (tbinfo);
>  
> +      if (x->toolbar_widget && !x->toolbar_is_packed)
> +	{
> +	  gtk_widget_destroy (x->toolbar_widget);
> +	  x->toolbar_widget = NULL;
> +	}
> +
>        /* x_free_frame_resources should have taken care of it */
>  #ifndef HAVE_PGTK
>  #ifdef HAVE_XDBE

LGTM.

> I think this second patch might also avoid future compiler warnings:
>
> diff --git a/src/gtkutil.c b/src/gtkutil.c
> index 97582a524da..73e9f778c75 100644
> --- a/src/gtkutil.c
> +++ b/src/gtkutil.c
> @@ -6123,8 +6123,7 @@ free_frame_tool_bar (struct frame *f)
>        else
>          gtk_widget_destroy (x->toolbar_widget);
>  
> -      x->toolbar_widget = 0;
> -      x->toolbar_widget = 0;
> +      x->toolbar_widget = NULL;
>        x->toolbar_is_packed = false;

0 as the rhs of an assignment to a pointer variable is taken for a NULL
pointer, so there's no issue with the existing code.  Indeed on many
systems NULL is simply defined to 0.