GNU bug report logs - #56462
29.0.50; [PATCH] Memory leak in ns_draw_relief

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56462 in the body.
You can then email your comments to 56462 AT debbugs.gnu.org in the normal way.

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

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Sat, 09 Jul 2022 16:13:39 +0200

[Message part 1 (text/plain, inline)]

I ran the Leaks tool with Emacs 29, and I've found a memory leak in the
NS version of Emacs.

Since commit c7b48b61d08f0b6a08584080badc60fe62ba1db1, in function
ns_draw_relief, static local variables baseCol and lightCol are assigned
to nil separately to their declaration.  That has the subtle consequence
that the further down calls to [baseCol release] and [lightCol release]
become a no-op, so each time ns_draw_relief is called it leaks some
instances of NSColor.

The fix is to revert to the previous way those static variables were
declared.

I've attached a patch with the correction.  With this fix, the leaks
tool doesn't report the NSColor leaks anymore when I play around with
Emacs 29.

Thanks.

[0001-Fix-memory-leak-in-ns_draw_relief.patch (text/x-patch, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 56462 <at> debbugs.gnu.org
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Sun, 10 Jul 2022 10:25:18 +0800

Daniel Martín <mardani29 <at> yahoo.es> writes:

> I ran the Leaks tool with Emacs 29, and I've found a memory leak in the
> NS version of Emacs.
>
> Since commit c7b48b61d08f0b6a08584080badc60fe62ba1db1, in function
> ns_draw_relief, static local variables baseCol and lightCol are assigned
> to nil separately to their declaration.  That has the subtle consequence
> that the further down calls to [baseCol release] and [lightCol release]
> become a no-op, so each time ns_draw_relief is called it leaks some
> instances of NSColor.
>
> The fix is to revert to the previous way those static variables were
> declared.
>
> I've attached a patch with the correction.  With this fix, the leaks
> tool doesn't report the NSColor leaks anymore when I play around with
> Emacs 29.

Thanks.  But I think the use of static variables there is rather ugly,
and it would be much nicer if we replicated the `x_setup_relief_color'
logic there.

Do you want to work on that, or should I?

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56462 <at> debbugs.gnu.org
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Sun, 10 Jul 2022 12:37:48 +0200

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

> Daniel Martín <mardani29 <at> yahoo.es> writes:
>
>> I ran the Leaks tool with Emacs 29, and I've found a memory leak in the
>> NS version of Emacs.
>>
>> Since commit c7b48b61d08f0b6a08584080badc60fe62ba1db1, in function
>> ns_draw_relief, static local variables baseCol and lightCol are assigned
>> to nil separately to their declaration.  That has the subtle consequence
>> that the further down calls to [baseCol release] and [lightCol release]
>> become a no-op, so each time ns_draw_relief is called it leaks some
>> instances of NSColor.
>>
>> The fix is to revert to the previous way those static variables were
>> declared.
>>
>> I've attached a patch with the correction.  With this fix, the leaks
>> tool doesn't report the NSColor leaks anymore when I play around with
>> Emacs 29.
>
> Thanks.  But I think the use of static variables there is rather ugly,
> and it would be much nicer if we replicated the `x_setup_relief_color'
> logic there.
>
> Do you want to work on that, or should I?

Thanks, I can give it a try.

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Daniel Martín via "Bug reports for GNU Emacs, the Swiss
 army knife of text editors" <bug-gnu-emacs <at> gnu.org>
Cc: Po Lu <luangruo <at> yahoo.com>, 56462 <at> debbugs.gnu.org
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Sun, 10 Jul 2022 22:52:08 +0200

[Message part 1 (text/plain, inline)]

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

>>
>> Thanks.  But I think the use of static variables there is rather ugly,
>> and it would be much nicer if we replicated the `x_setup_relief_color'
>> logic there.
>>
>> Do you want to work on that, or should I?
>
> Thanks, I can give it a try.

Here's a patch where the static variables are now proper fields in the
ns_output structure.  Is that what you had in mind?

[0001-Fix-memory-leak-in-ns_draw_relief.patch (text/x-patch, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: Alan Third <alan <at> idiocy.org>, 56462 <at> debbugs.gnu.org
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Mon, 11 Jul 2022 09:50:11 +0800

Daniel Martín <mardani29 <at> yahoo.es> writes:

> Daniel Martín via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>>>
>>> Thanks.  But I think the use of static variables there is rather ugly,
>>> and it would be much nicer if we replicated the `x_setup_relief_color'
>>> logic there.
>>>
>>> Do you want to work on that, or should I?
>>
>> Thanks, I can give it a try.
>
> Here's a patch where the static variables are now proper fields in the
> ns_output structure.  Is that what you had in mind?

Yes, this would be great.  Thanks.

Alan might have some comments; if he doesn't, I'll install this now.

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

From: Alan Third <alan <at> idiocy.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56462 <at> debbugs.gnu.org,
 Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Mon, 11 Jul 2022 09:01:07 +0100

On Mon, Jul 11, 2022 at 09:50:11AM +0800, Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote:
> Daniel Martín <mardani29 <at> yahoo.es> writes:
> 
> > Daniel Martín via "Bug reports for GNU Emacs, the Swiss army knife of
> > text editors" <bug-gnu-emacs <at> gnu.org> writes:
> >
> >>>
> >>> Thanks.  But I think the use of static variables there is rather ugly,
> >>> and it would be much nicer if we replicated the `x_setup_relief_color'
> >>> logic there.
> >>>
> >>> Do you want to work on that, or should I?
> >>
> >> Thanks, I can give it a try.
> >
> > Here's a patch where the static variables are now proper fields in the
> > ns_output structure.  Is that what you had in mind?
> 
> Yes, this would be great.  Thanks.
> 
> Alan might have some comments; if he doesn't, I'll install this now.

No, looks fine to me.
-- 
Alan Third

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

From: Po Lu <luangruo <at> yahoo.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 56462 <at> debbugs.gnu.org
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Mon, 11 Jul 2022 18:25:35 +0800

Daniel Martín <mardani29 <at> yahoo.es> writes:

> Daniel Martín via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>>>
>>> Thanks.  But I think the use of static variables there is rather ugly,
>>> and it would be much nicer if we replicated the `x_setup_relief_color'
>>> logic there.
>>>
>>> Do you want to work on that, or should I?
>>
>> Thanks, I can give it a try.
>
> Here's a patch where the static variables are now proper fields in the
> ns_output structure.  Is that what you had in mind?

Now installed, so I'm closing this bug.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56462 <at> debbugs.gnu.org, Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#56462: 29.0.50; [PATCH] Memory leak in ns_draw_relief
Date: Mon, 05 Sep 2022 21:23:18 +0200

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

> Now installed, so I'm closing this bug.

The bug report was still open, so I'm closing it now.

Previous Next

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