GNU bug report logs - #75636
GTK memory leaks

Previous Next

To reply to this bug, email your comments to 75636 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org, Po Lu <luangruo <at> yahoo.com>
Subject: GTK memory leaks
Date: Fri, 17 Jan 2025 20:15:02 +0000

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.

2. free_frame_tool_bar is never called in my experiments, so the toolbar
widget isn't destroyed.

This isn't usually a problem because it's just a small memory leak, but
on the feature/igc branch, user data passed to glib must be protected in
a special GC root structure; such structures accumulate when running in
GTK, eventually reducing GC performance significantly.

Unfortunately, valgrind doesn't seem to catch these memory leaks.

Message #8 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Sat, 18 Jan 2025 17:10:00 +0800

Pip Cet <pipcet <at> protonmail.com> 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.

> 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.

Message #11 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Sat, 18 Jan 2025 11:49:58 +0000

"Po Lu" <luangruo <at> yahoo.com> writes:

> Pip Cet <pipcet <at> protonmail.com> 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.

>> 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

Message #14 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Sun, 19 Jan 2025 08:53:34 +0800

Pip Cet <pipcet <at> protonmail.com> writes:

> "Po Lu" <luangruo <at> yahoo.com> writes:
>
>> Pip Cet <pipcet <at> protonmail.com> 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.

Message #17 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Tue, 21 Jan 2025 11:24:21 +0000

"Po Lu" <luangruo <at> yahoo.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> "Po Lu" <luangruo <at> yahoo.com> writes:
>>
>>> Pip Cet <pipcet <at> protonmail.com> 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 <pipcet <at> protonmail.com>

    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

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;
       FRAME_TOOLBAR_TOP_HEIGHT (f) = FRAME_TOOLBAR_BOTTOM_HEIGHT (f) = 0;
       FRAME_TOOLBAR_LEFT_WIDTH (f) = FRAME_TOOLBAR_RIGHT_WIDTH (f) = 0;

If we can apply the first, minor patch, I think this bug can be closed;
if I run into further GTK issues, I'll open a new one.

Pip

Message #20 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>, Po Lu <luangruo <at> yahoo.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Tue, 21 Jan 2025 10:33:51 -0800

Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:

> 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;
>        FRAME_TOOLBAR_TOP_HEIGHT (f) = FRAME_TOOLBAR_BOTTOM_HEIGHT (f) = 0;
>        FRAME_TOOLBAR_LEFT_WIDTH (f) = FRAME_TOOLBAR_RIGHT_WIDTH (f) = 0;

LGTM.

Message #23 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Wed, 22 Jan 2025 08:42:18 +0800

Pip Cet <pipcet <at> protonmail.com> writes:

> "Po Lu" <luangruo <at> yahoo.com> writes:
>
>> Pip Cet <pipcet <at> protonmail.com> writes:
>>
>>> "Po Lu" <luangruo <at> yahoo.com> writes:
>>>
>>>> Pip Cet <pipcet <at> protonmail.com> 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 <pipcet <at> protonmail.com>
>
>     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.

Message #26 received at 75636 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 75636 <at> debbugs.gnu.org
Subject: Re: bug#75636: GTK memory leaks
Date: Wed, 22 Jan 2025 01:06:49 +0000

"Po Lu" <luangruo <at> yahoo.com> writes:

>> commit 5ed4aaa986dbf75f8565f4d8cdd3d1d1b38b6142 (HEAD)
>> Author: Pip Cet <pipcet <at> protonmail.com>
>>
>>     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.

Thanks, pushed.

Just to clarify why I suspect this is more important than it looks: on
the feature/igc branch, a leak like this means the frame and all objects
referenced by it will become unfreeable.  The problem is not the GTK
memory usage, it's that MPS has no way of indicating "if this object is
still alive, don't move it" without simultaneously keeping it alive.

(At some point we might allow the frame to be moved even though GTK owns
a pointer to a pointer to it.  After the merge...)

>> 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.

The issue with the old code (I've pushed this part by now) was the
duplicated assignment.  I think compilers will start to warn about that
much sooner than about the correct (if very, very slightly deprecated)
usage of plain 0 here.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.