bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Hi. This is similar to, but different from
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21509

On my machine if I open a daemon with emacs -Q, and start/stop clients
as described in the previous report, I see one font leaking with every
frame. With one exception (below), everything that is cleaned out, is
cleaned with x_delete_terminal() where all fonts in emacs's font cache
are cleared.

The bug appears to be that something is resetting emacs's font cache at
some point during frame creation, and any fonts that were cached before
that point end up leaking. With a breakpoint at font_load_for_lface() I
print the font cache. font_load_for_lface() is called 4 times during
frame creation, and the caches look like this:


(":0.0" (x 1) (xft 1))
(":0.0" (x 1)
  (xft 1
    (#<font-spec xft nil Monospace nil iso8859-1 nil nil nil nil nil nil nil ((:name . "monospace-10"))> .
      [#<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 bold oblique normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-BoldOblique.ttf" . 0) (:name . "monospace-10"))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 bold normal normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-Bold.ttf" . 0) (:name . "monospace-10"))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 normal normal normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono.ttf" . 0) (:name . "monospace-10"))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 normal oblique normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-Oblique.ttf" . 0) (:name . "monospace-10"))>])))
(":0.0" (x 1) (xft 1))
(":0.0" (x 1)
  (xft 1
    (#<font-spec xft unknown DejaVu\ Sans\ Mono nil iso8859-1 nil nil nil nil nil nil nil nil> .
      [#<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 bold oblique normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-BoldOblique.ttf" . 0))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 bold normal normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-Bold.ttf" . 0))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 normal normal normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono.ttf" . 0))>
       #<font-entity xft unknown DejaVu\ Sans\ Mono nil iso10646-1 normal oblique normal 0 nil 100 0 ((:font-entity "/usr/share/fonts/truetype/dejavu/DejaVuSansMono-Oblique.ttf" . 0))>])))


Note that this cache is reset between the 2nd and 3rd calls without the
corresponding font being freed. I cannot tell where this is being
cleared; all the suspect breakpoints aren't hitting. Main question of
this bug report so far:




  Is there a way to put a watchpoint on a lisp object? How?




I'm observing that most (but not all) of the time, the menu font is
being created around the same time as the cache reset. Backtrace:

	    7fcf06d1e99f XftFontOpenName (/usr/lib/x86_64-linux-gnu/libXft.so.2.3.2)
	          6b5861 openXftFont (/tmp/emacs-tst)
	          6b5999 XlwMenuInitialize (/tmp/emacs-tst)
	    7fcf07ac9e2c [unknown] (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
	    7fcf07aca7c8 [unknown] (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
	    7fcf07acac18 _XtCreateWidget (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
	    7fcf07acaefd XtCreateWidget (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
	          6b01f3 xlw_create_menubar (/tmp/emacs-tst)
	          6af1d3 instantiate_widget_instance (/tmp/emacs-tst)
	          6ae462 allocate_widget_instance (/tmp/emacs-tst)
	          6af306 lw_make_widget (/tmp/emacs-tst)
	          6af392 lw_create_widget (/tmp/emacs-tst)
	          4974b9 set_frame_menubar (/tmp/emacs-tst)
	          497673 initialize_frame_menubar (/tmp/emacs-tst)
	          5358f5 Fx_create_frame (/tmp/emacs-tst)

This font allocation doesn't read or write to the frame cache, and has
its own font-cleanup call that appears to work. I cannot find any
explicit evidence that this clears out the font cache, but turning off
the menu with M-x menu-bar-mode makes this leak mostly go away.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Date: Thu, 24 Sep 2015 17:05:12 -0700
> 
> Main question of this bug report so far:
> 
>   Is there a way to put a watchpoint on a lisp object?

Theoretically, yes.

> How?

Depends on the object.  Which object do you want to put the watchpoint
on?  Please show its source variable name and the source file lines
where it is used.  Also, if the object is complex, what kinds of
changes in it would you like to watch?

> I'm observing that most (but not all) of the time, the menu font is
> being created around the same time as the cache reset. Backtrace:
> 
> 	    7fcf06d1e99f XftFontOpenName (/usr/lib/x86_64-linux-gnu/libXft.so.2.3.2)
> 	          6b5861 openXftFont (/tmp/emacs-tst)
> 	          6b5999 XlwMenuInitialize (/tmp/emacs-tst)
> 	    7fcf07ac9e2c [unknown] (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
> 	    7fcf07aca7c8 [unknown] (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
> 	    7fcf07acac18 _XtCreateWidget (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
> 	    7fcf07acaefd XtCreateWidget (/usr/lib/x86_64-linux-gnu/libXt.so.6.0.0)
> 	          6b01f3 xlw_create_menubar (/tmp/emacs-tst)
> 	          6af1d3 instantiate_widget_instance (/tmp/emacs-tst)
> 	          6ae462 allocate_widget_instance (/tmp/emacs-tst)
> 	          6af306 lw_make_widget (/tmp/emacs-tst)
> 	          6af392 lw_create_widget (/tmp/emacs-tst)
> 	          4974b9 set_frame_menubar (/tmp/emacs-tst)
> 	          497673 initialize_frame_menubar (/tmp/emacs-tst)
> 	          5358f5 Fx_create_frame (/tmp/emacs-tst)
> 
> This font allocation doesn't read or write to the frame cache, and has
> its own font-cleanup call that appears to work. I cannot find any
> explicit evidence that this clears out the font cache, but turning off
> the menu with M-x menu-bar-mode makes this leak mostly go away.

This is in the lwlib library, whose font allocations are AFAIK not
tracked by Emacs's font managing machinery.  With other toolkits, we
don't even know which fonts are used by the menus, and don't handle
nor have access to those fonts.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is there a way to put a watchpoint on a lisp object? How?
>
> Depends on the object.  Which object do you want to put the watchpoint
> on?  Please show its source variable name and the source file lines
> where it is used.  Also, if the object is complex, what kinds of
> changes in it would you like to watch?

The object is the emacs font cache. Specifically in this case (Debian,
X11, xft fonts, lucid widgets) it is

  frame->output_data.x->display_info->name_list_element

where frame is a struct frame*, such as the first argument of

  font_load_for_lface()

In the original message for this bug report I showed 'pp' output for
this first argument of font_load_for_lface(). This function is called 4
times during the creation of one frame. I want to know why the font
cache is cleared before the 3rd call. I tried to examine the code, and
to place breakpoints in places that could be performing this cache
clear, but none of those places look like the right ones. If a
watchpoint is possible, it would tell me where this is cleared, instead
of me having to guess.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: 21556@debbugs.gnu.org
> Date: Thu, 24 Sep 2015 23:57:00 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is there a way to put a watchpoint on a lisp object? How?
> >
> > Depends on the object.  Which object do you want to put the watchpoint
> > on?  Please show its source variable name and the source file lines
> > where it is used.  Also, if the object is complex, what kinds of
> > changes in it would you like to watch?
> 
> The object is the emacs font cache. Specifically in this case (Debian,
> X11, xft fonts, lucid widgets) it is
> 
>   frame->output_data.x->display_info->name_list_element
> 
> where frame is a struct frame*, such as the first argument of
> 
>   font_load_for_lface()
> 
> In the original message for this bug report I showed 'pp' output for
> this first argument of font_load_for_lface(). This function is called 4
> times during the creation of one frame. I want to know why the font
> cache is cleared before the 3rd call. I tried to examine the code, and
> to place breakpoints in places that could be performing this cache
> clear, but none of those places look like the right ones. If a
> watchpoint is possible, it would tell me where this is cleared, instead
> of me having to guess.

The place where the font cache is "cleared" (actually, it's compacted)
is in compact_font_caches, and specifically in its subroutine
compact_font_cache_entry.  Did you place breakpoints there, and if so,
what did you see?

If the breakpoints in those functions don't help, then you'd need to
put 2 watchpoints: one at the cdr pointer of (xft 1 ...) list, the
other at the object pointed by that pointer.  You can get at the
values of these by using the GDB commands "xcar", "xcdr", and "xcons",
starting with

  (gdb) p TERMINAL_FONT_CACHE(FRAME_TERMINAL(f))

which will display (the EMACS_INT value of) the object you showed in
your previous message.  Use xcdr to get to the tail of the list which
you want to watch, then use xcar to show the element and xcons to
display its C internals.

Once you discover the addresses of the 2 values, set the watchpoints
on their numerical addresses, do not use the variable names.
Something like this:

  (gdb) watch *(struct Lisp_Cons *) 0x12345676887654320

(If you do want to use the variable names, use "watch -l" to place the
watchpoints on their locations.)

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dima Kogan <dima@secretsauce.net>
>> Date: Thu, 24 Sep 2015 17:05:12 -0700
>> 
>> Main question of this bug report so far:
>> 
>>   Is there a way to put a watchpoint on a lisp object?
>
> Theoretically, yes.

Hi. I just came back to this, figured out how to (semi-manually) put in
a watchpoint, and this watchpoint told me what's resetting the font
cache: the garbage collector. The backtrace in question:

    0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
    #0  0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
    #1  0x00000000005d221b in compact_font_caches () at alloc.c:5339
    #2  0x00000000005d2742 in garbage_collect_1 (end=0x7ffcdb166830) at alloc.c:5515
    #3  0x00000000005d2e1d in Fgarbage_collect () at alloc.c:5720
    #4  0x000000000054eb21 in maybe_gc () at lisp.h:4515
    #5  0x00000000005f638c in Ffuncall (nargs=3, args=0x7ffcdb166988) at eval.c:2584

    Lisp Backtrace:
    "Automatic GC" (0x0)
    "map-keymap" (0xdb166990)
    "keymap-canonicalize" (0xdb166f38)
    "x-create-frame" (0xdb1678a0)
    "x-create-frame-with-faces" (0xdb167dd8)
    0x12b9d80 PVEC_COMPILED
    "apply" (0xdb168450)
    "frame-creation-function" (0xdb1689f0)
    "make-frame" (0xdb168f40)
    "make-frame-on-display" (0xdb1694a8)
    "server-create-window-system-frame" (0xdb169a78)
    "server-process-filter" (0xdb169ff8)
    Hardware watchpoint 24: ((struct Lisp_Cons *) 0x1701190)->u.cdr


As a test I asked gdb to never drop fonts in the gc by acting as if
drop==0 in compact_font_cache_entry(), and that made the leak in this
bug go away. Clearly that's not a fix, but it's evidence that the
understanding of the problem is correct, so progress is being made.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: 21556@debbugs.gnu.org
> Date: Fri, 25 Sep 2015 01:13:15 -0700
> 
> Hi. I just came back to this, figured out how to (semi-manually) put in
> a watchpoint, and this watchpoint told me what's resetting the font
> cache: the garbage collector. The backtrace in question:
> 
>     0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
>     #0  0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
>     #1  0x00000000005d221b in compact_font_caches () at alloc.c:5339
>     #2  0x00000000005d2742 in garbage_collect_1 (end=0x7ffcdb166830) at alloc.c:5515
>     #3  0x00000000005d2e1d in Fgarbage_collect () at alloc.c:5720
>     #4  0x000000000054eb21 in maybe_gc () at lisp.h:4515
>     #5  0x00000000005f638c in Ffuncall (nargs=3, args=0x7ffcdb166988) at eval.c:2584

Yes, that's what I wrote to you you meanwhile.

This seems to mean that fonts whose entries in the cache are not
marked are still being used, or are unused but not freed.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> Date: Fri, 25 Sep 2015 11:49:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 21556@debbugs.gnu.org
> 
> > From: Dima Kogan <dima@secretsauce.net>
> > Cc: 21556@debbugs.gnu.org
> > Date: Fri, 25 Sep 2015 01:13:15 -0700
> > 
> > Hi. I just came back to this, figured out how to (semi-manually) put in
> > a watchpoint, and this watchpoint told me what's resetting the font
> > cache: the garbage collector. The backtrace in question:
> > 
> >     0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
> >     #0  0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
> >     #1  0x00000000005d221b in compact_font_caches () at alloc.c:5339
> >     #2  0x00000000005d2742 in garbage_collect_1 (end=0x7ffcdb166830) at alloc.c:5515
> >     #3  0x00000000005d2e1d in Fgarbage_collect () at alloc.c:5720
> >     #4  0x000000000054eb21 in maybe_gc () at lisp.h:4515
> >     #5  0x00000000005f638c in Ffuncall (nargs=3, args=0x7ffcdb166988) at eval.c:2584
> 
> Yes, that's what I wrote to you you meanwhile.
> 
> This seems to mean that fonts whose entries in the cache are not
> marked are still being used, or are unused but not freed.

Some additional background for these issues:

This is why the font cache compaction was introduced:

   http://lists.gnu.org/archive/html/emacs-devel/2013-10/msg00740.html

Here's one problem caused by the compaction code, and some followup
discussions with perhaps useful debug code:

   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15876#77

This is another related bug, which eventually caused the font cache
compaction be ifdef'ed away for MS-Windows:

   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16140

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> cache: the garbage collector. The backtrace in question:
>> 
>>     0x00000000005d2172 in compact_font_cache_entry (entry=24121763) at alloc.c:5313
>
> Yes, that's what I wrote to you you meanwhile.
>
> This seems to mean that fonts whose entries in the cache are not
> marked are still being used, or are unused but not freed.

You called it exactly.

> Some additional background for these issues:

Thanks. Very useful.

I guess I still don't know if the fonts are supposed to be marked or
not. They appear to never be marked. Do you know where that is supposed
to happen?

Furthermore, the compaction code is incomplete, at least for xft. Xft
refence-counts the fonts, so you must close all fonts you have opened.
Emacs stores the fonts that have been opened in the cache, so if it ever
drops any fonts from the cache, it must tell xft to close, or else
things leak, as we're seeing. I haven't tried to do this yet, but I
suspect that the fonts should be marked, otherwise we'd be closing the
font that we have just opened.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: 21556@debbugs.gnu.org
> Date: Fri, 25 Sep 2015 02:30:45 -0700
> 
> I guess I still don't know if the fonts are supposed to be marked or
> not. They appear to never be marked. Do you know where that is supposed
> to happen?
> 
> Furthermore, the compaction code is incomplete, at least for xft. Xft
> refence-counts the fonts, so you must close all fonts you have opened.
> Emacs stores the fonts that have been opened in the cache, so if it ever
> drops any fonts from the cache, it must tell xft to close, or else
> things leak, as we're seeing. I haven't tried to do this yet, but I
> suspect that the fonts should be marked, otherwise we'd be closing the
> font that we have just opened.

Originally, font caches were not compacted at all until that
discussion in Oct 2013; they are still not compacted now on w32.  To
figure out how to compact those caches correctly, I think we need to
start with the basics, and understand well the "life cycle" of a font
in Emacs, including its font-cache entries: when and how a font is
opened and registered in the cache, when and how (and whether) it is
closed and removed from the cache, etc.  This stuff is notoriously
under-documented in Emacs, and we no longer have active maintainers on
board who are familiar with it.

So I'm afraid we are on our own wrt these issues.  (I'm CC'ing
Handa-san, who wrote most of the font-related code, in the hope that
he could chime in at some point and help us.)  I can offer whatever
help I can extend, which is admittedly not too much.  If you have time
and motivation, I suggest to start from the end: i.e., figure out when
and how a font is closed and removed from Emacs.  If we are lucky,
perhaps the font cache compaction could be triggered by whatever
triggers a font's removal from Emacs.

Thanks.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: 21556@debbugs.gnu.org
> Date: Fri, 25 Sep 2015 02:30:45 -0700
> 
> I guess I still don't know if the fonts are supposed to be marked or
> not. They appear to never be marked. Do you know where that is supposed
> to happen?

See the commentary at the beginning of font.h: a font object is a
pseudo-vector.  So I guess fonts should be marked where pseudo-vectors
are.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[K. Handa]

In article <83y4ftfbjw.fsf@gnu.org>, Eli Zaretskii <eliz@gnu.org> writes:

> > > I guess I still don't know if the fonts are supposed to be marked or
> > > not. They appear to never be marked. Do you know where that is supposed
> > > to happen?
> > > 
> > > Furthermore, the compaction code is incomplete, at least for xft. Xft
> > > refence-counts the fonts, so you must close all fonts you have opened.
> > > Emacs stores the fonts that have been opened in the cache, so if it ever
> > > drops any fonts from the cache, it must tell xft to close, or else
> > > things leak, as we're seeing. I haven't tried to do this yet, but I
> > > suspect that the fonts should be marked, otherwise we'd be closing the
> > > font that we have just opened.

As the font compaction code (and some other work related to clearing
font cache) is done by Dmitry, I included him in CC:.

> > Originally, font caches were not compacted at all until that
> > discussion in Oct 2013; they are still not compacted now on w32.  To
> > figure out how to compact those caches correctly, I think we need to
> > start with the basics, and understand well the "life cycle" of a font
> > in Emacs, including its font-cache entries: when and how a font is
> > opened and registered in the cache, when and how (and whether) it is
> > closed and removed from the cache, etc.  This stuff is notoriously
> > under-documented in Emacs, and we no longer have active maintainers on
> > board who are familiar with it.
> > 
> > So I'm afraid we are on our own wrt these issues.  (I'm CC'ing
> > Handa-san, who wrote most of the font-related code, in the hope that
> > he could chime in at some point and help us.)

I feel very guilty for those under-documented codes.  When I wrote the
original code long ago, font listing and opening was very slow on
GNU/Linux system.  So I tried to cache the result of listing as far as
possible, and also tried to reuse the once opened font object as far as
possible.  The latter means that even if a font object once created
becomes unnecessary (perhaps because a frame is deleted), it is not
freed, because, once a user displayed a specific character, it is
displayed again with a very high possibility.  So, when a font is
closed?  It is closed by an explicit call of clear-font-fache.  As far
as I remember (though vaguely), that was my original intention.

---
K. Handa
handa@gnu.org

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: handa@gnu.org (K. Handa)
> Cc: 21556@debbugs.gnu.org, dmantipov@yandex.ru
> Date: Sun, 27 Sep 2015 16:56:47 +0900
> 
> When I wrote the original code long ago, font listing and opening
> was very slow on GNU/Linux system.  So I tried to cache the result
> of listing as far as possible, and also tried to reuse the once
> opened font object as far as possible.  The latter means that even
> if a font object once created becomes unnecessary (perhaps because a
> frame is deleted), it is not freed, because, once a user displayed a
> specific character, it is displayed again with a very high
> possibility.  So, when a font is closed?  It is closed by an
> explicit call of clear-font-fache.  As far as I remember (though
> vaguely), that was my original intention.

So maybe we should simply remove (or ifdef away) the code that
compacts the font caches.  If your assumption about reusing the font
is anywhere near the truth, compacting the font cache gives us more
trouble than it gains: we get slow redisplay in some cases and random
hard-to-debug bugs, while the gains are only visible in very rare use
cases such as the one described in the Oct 2013 discussion.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

> So maybe we should simply remove (or ifdef away) the code that
> compacts the font caches.  If your assumption about reusing the font
> is anywhere near the truth, compacting the font cache gives us more
> trouble than it gains: we get slow redisplay in some cases and random
> hard-to-debug bugs, while the gains are only visible in very rare use
> cases such as the one described in the Oct 2013 discussion.

Hi. I put in some more probes, and it looks like the fonts aren't being
marked as used properly. I also wrote a bit of code to make the gc
actually free fonts it drops. Unsurprisingly, this causes things to
crash, since it then frees fonts that are actually in use. Does it make
sense to track down why the fonts aren't marked as used? If the
compaction code goes away, then it doesn't really matter.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: "K. Handa" <handa@gnu.org>, 21556@debbugs.gnu.org, dmantipov@yandex.ru
> Date: Mon, 28 Sep 2015 02:22:50 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So maybe we should simply remove (or ifdef away) the code that
> > compacts the font caches.  If your assumption about reusing the font
> > is anywhere near the truth, compacting the font cache gives us more
> > trouble than it gains: we get slow redisplay in some cases and random
> > hard-to-debug bugs, while the gains are only visible in very rare use
> > cases such as the one described in the Oct 2013 discussion.
> 
> Hi. I put in some more probes, and it looks like the fonts aren't being
> marked as used properly. I also wrote a bit of code to make the gc
> actually free fonts it drops. Unsurprisingly, this causes things to
> crash, since it then frees fonts that are actually in use. Does it make
> sense to track down why the fonts aren't marked as used? If the
> compaction code goes away, then it doesn't really matter.

It depends on how much time and energy do you have.  I think it would
be nice to understand where and how do we mark used fonts, if only to
document that and make sure we don't miss anything.  However, my
personal preference is to remove the cache compaction, unless we hear
some good reasons from Dmitry, and some ideas for how to fix the
leaks.

So, if you prefer not to invest any time in investigating the font
marking code, let's wait for Dmitry to respond, and make the decision
when he does (or when we give up waiting).

Thanks.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> So, if you prefer not to invest any time in investigating the font
> marking code, let's wait for Dmitry to respond, and make the decision
> when he does (or when we give up waiting).

I guess I preferred to invest more time. I found and fixed the bug, and
the patch is attached. The issue was that the compaction code wasn't
checking all the right lisp objects for the marks. The font entities
were storing a list of fonts, and this list had to be traversed, looking
for the marks. See font_clear_cache() for a function that WAS traversing
the full list.

I'm certain there are more leaks here, but bug 21509 is now the main
leaker with my current test case.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-compact_font_cache_entry-now-properly-checks-for-mar.patch --]
[-- Type: text/x-diff, Size: 1876 bytes --]

From e9b0394826c4e706550259cd3862a89343c6cf2b Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 29 Sep 2015 02:19:35 -0700
Subject: [PATCH] compact_font_cache_entry() now properly checks for marked
 fonts

* src/alloc.c (compact_font_cache_entry): When checking for marked
fonts we were looking at a font entity object.  However the entity
could contain a list of font objects that need to be checked, which we
now do.  This resolves a memory leak
Fixes: bug#21556
---
 src/alloc.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 3ab2a6e..03df258 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5303,8 +5303,31 @@ compact_font_cache_entry (Lisp_Object entry)
 	     are not marked too.  But we must be sure that nothing is
 	     marked within OBJ before we really drop it.  */
 	  for (i = 0; i < size; i++)
-	    if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
-	      break;
+            {
+              Lisp_Object objlist;
+              bool any_fonts_marked;
+
+              if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
+                break;
+
+              objlist = AREF (AREF (XCDR (obj), i), FONT_OBJLIST_INDEX);
+              any_fonts_marked = false;
+
+              for (; CONSP (objlist); objlist = XCDR (objlist))
+                {
+                  Lisp_Object val = XCAR (objlist);
+                  struct font *font = XFONT_OBJECT (val);
+
+                  if (! NILP (AREF (val, FONT_TYPE_INDEX)) &&
+                      VECTOR_MARKED_P(font))
+                    {
+                      any_fonts_marked = true;
+                      break;
+                    }
+                }
+              if(any_fonts_marked)
+                break;
+            }
 
 	  if (i == size)
 	    drop = 1;
-- 
2.1.4

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: handa@gnu.org, 21556@debbugs.gnu.org, dmantipov@yandex.ru
> Date: Tue, 29 Sep 2015 02:28:56 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So, if you prefer not to invest any time in investigating the font
> > marking code, let's wait for Dmitry to respond, and make the decision
> > when he does (or when we give up waiting).
> 
> I guess I preferred to invest more time. I found and fixed the bug, and
> the patch is attached. The issue was that the compaction code wasn't
> checking all the right lisp objects for the marks. The font entities
> were storing a list of fonts, and this list had to be traversed, looking
> for the marks. See font_clear_cache() for a function that WAS traversing
> the full list.

Thanks!

Let's wait for a week for comments, and if nothing significant comes
up, let's install this.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dmitry Antipov]

On 09/29/2015 12:28 PM, Dima Kogan wrote:

> I guess I preferred to invest more time. I found and fixed the bug, and
> the patch is attached. The issue was that the compaction code wasn't
> checking all the right lisp objects for the marks. The font entities
> were storing a list of fonts, and this list had to be traversed, looking
> for the marks. See font_clear_cache() for a function that WAS traversing
> the full list.

IMO this code should be reworked to omit 'any_fonts_marked' variable :-),
something like:

diff --git a/src/alloc.c b/src/alloc.c
index 3ab2a6e..3109654 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5303,8 +5303,25 @@ compact_font_cache_entry (Lisp_Object entry)
              are not marked too.  But we must be sure that nothing is
              marked within OBJ before we really drop it.  */
           for (i = 0; i < size; i++)
-           if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
-             break;
+            {
+              Lisp_Object objlist;
+
+              if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
+                break;
+
+              objlist = AREF (AREF (XCDR (obj), i), FONT_OBJLIST_INDEX);
+              for (; CONSP (objlist); objlist = XCDR (objlist))
+                {
+                  Lisp_Object val = XCAR (objlist);
+                  struct font *font = XFONT_OBJECT (val);
+
+                  if (! NILP (AREF (val, FONT_TYPE_INDEX)) &&
+                      VECTOR_MARKED_P(font))
+                   break;
+                }
+              if (!NILP (objlist))
+                break;
+            }

           if (i == size)
             drop = 1;


In general, this patch hits the case where the font object is marked but the
corresponding font entity is not; but is that legal? IIRC Emacs asks the font
driver to find a font described by font spec, and returned object is a font entity,
which is a list of font objects plus some extra stuff. Thus, there should be
no "free-floating" font objects, i.e. for each font object, there should be
at least one font entity object which references that font. IOW, having
marked font object without marked font entity looks like GC mark bug for me.

Dmitry

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 09/29/2015 12:28 PM, Dima Kogan wrote:
>
>> I found and fixed the bug, and the patch is attached. The issue was
>> that the compaction code wasn't checking all the right lisp objects
>> for the marks. The font entities were storing a list of fonts, and
>> this list had to be traversed, looking for the marks.
>
> In general, this patch hits the case where the font object is marked but the
> corresponding font entity is not; but is that legal? IIRC Emacs asks the font
> driver to find a font described by font spec, and returned object is a font entity,
> which is a list of font objects plus some extra stuff. Thus, there should be
> no "free-floating" font objects, i.e. for each font object, there should be
> at least one font entity object which references that font. IOW, having
> marked font object without marked font entity looks like GC mark bug for me.

OK. The target of the patch is as you describe: fonts marked inside an
unmarked entity. I'm observing this situation every time from an emacs
-Q. The font is marked inside mark_face_cache(), which looks like this:


   NO_INLINE /* To reduce stack depth in mark_object.  */
   static void
   mark_face_cache (struct face_cache *c)
   {
     if (c)
       {
         int i, j;
         for (i = 0; i < c->used; ++i)
           {
             struct face *face = FACE_FROM_ID (c->f, i);

             if (face)
               {
                 if (face->font && !VECTOR_MARKED_P (face->font))
                   mark_vectorlike ((struct Lisp_Vector *) face->font);

                 for (j = 0; j < LFACE_VECTOR_SIZE; ++j)
                   mark_object (face->lface[j]);
               }
           }
       }
   }


Clearly in this function we mark the font. We don't obviously mark the
containing entity, unless it's one of the face->lface[] elements. If
even in this case we're supposed to be marking the entity, where would
this be?

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dmitry Antipov]

On 10/01/2015 12:42 PM, Dima Kogan wrote:

> Clearly in this function we mark the font. We don't obviously mark the
> containing entity, unless it's one of the face->lface[] elements. If
> even in this case we're supposed to be marking the entity, where would
> this be?

Hm...I suppose that all really used font stuff should be marked through
fontsets (read: char tables, see fontset.c) recorded in Vfontset_table.

Dmitry

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 10/01/2015 12:42 PM, Dima Kogan wrote:
>
>> Clearly in this function we mark the font. We don't obviously mark the
>> containing entity, unless it's one of the face->lface[] elements. If
>> even in this case we're supposed to be marking the entity, where would
>> this be?
>
> Hm...I suppose that all really used font stuff should be marked through
> fontsets (read: char tables, see fontset.c) recorded in Vfontset_table.

OK, so are you suggesting changing how mark_face_cache() works? How bad
is it to accept that fonts and font entities are not necessarily linked,
and to install the latest patch in this bug?

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dmitry Antipov]

On 10/01/2015 09:50 PM, Dima Kogan wrote:

> OK, so are you suggesting changing how mark_face_cache() works? How bad
> is it to accept that fonts and font entities are not necessarily linked,
> and to install the latest patch in this bug?

I'm suggesting to check whether there are unmarked font objects after marking
from Vfontset_table, and, if so, understand whether it's correct. Otherwise
your patch, even being correct by itself, may just hide subtle GC bug.

Dmitry

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> I'm suggesting to check whether there are unmarked font objects after marking
> from Vfontset_table, and, if so, understand whether it's correct. Otherwise
> your patch, even being correct by itself, may just hide subtle GC bug.

OK, I need even more clarification here. You're saying that you would
expect both the font entities and the fonts to be marked from
mark_char_table()? How does this relate to Vfontset_table? The code
doesn't make this obvious.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 10/01/2015 09:50 PM, Dima Kogan wrote:
>
>> OK, so are you suggesting changing how mark_face_cache() works? How bad
>> is it to accept that fonts and font entities are not necessarily linked,
>> and to install the latest patch in this bug?
>
> I'm suggesting to check whether there are unmarked font objects after marking
> from Vfontset_table, and, if so, understand whether it's correct. Otherwise
> your patch, even being correct by itself, may just hide subtle GC bug.

Hi. I looked at this again. Running the same test as before (emacs -Q,
repeatedly creating/destroying client frame) I see:


- entities are created with each new client frame but are /never/
  marked.

- entity-creation backtrace is always

  #0  0x000000000060e74e in font_make_entity () at font.c:173
  #1  0x00000000006793ae in ftfont_pattern_entity (p=0xf8c180, extra=20784563) at ftfont.c:215
  #2  0x000000000067b952 in ftfont_list (f=0x13fb8c0, spec=13463989) at ftfont.c:1057
  #3  0x0000000000680de6 in xftfont_list (f=0x13fb8c0, spec=13463989) at xftfont.c:138
  #4  0x0000000000615ebc in font_list_entities (f=0x13fb8c0, spec=20978277) at font.c:2780
  #5  0x0000000000617c27 in font_find_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933, c=-1) at font.c:3262
  #6  0x0000000000617fb0 in font_load_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933) at font.c:3335
  #7  0x00000000006183a2 in font_open_by_spec (f=0x13fb8c0, spec=20082933) at font.c:3429
  #8  0x0000000000618415 in font_open_by_name (f=0x13fb8c0, name=13702436) at font.c:3440
  #9  0x000000000052fec4 in x_default_font_parameter (f=0x13fb8c0, parms=20784979) at xfns.c:2904
  #10 0x0000000000530bc2 in Fx_create_frame (parms=20784979) at xfns.c:3139

- Vfontset_table has fontsets and font-specs in it, but NO
  font-entities. Marking from the Vfontset_table thus cannot mark any
  font entities.

Where are the entities supposed to be referenced? Does it make sense
they're never marked?

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, handa@gnu.org, 21556@debbugs.gnu.org
> Date: Thu, 29 Oct 2015 15:51:38 -0700
> 
> Dmitry Antipov <dmantipov@yandex.ru> writes:
> 
> > On 10/01/2015 09:50 PM, Dima Kogan wrote:
> >
> >> OK, so are you suggesting changing how mark_face_cache() works? How bad
> >> is it to accept that fonts and font entities are not necessarily linked,
> >> and to install the latest patch in this bug?
> >
> > I'm suggesting to check whether there are unmarked font objects after marking
> > from Vfontset_table, and, if so, understand whether it's correct. Otherwise
> > your patch, even being correct by itself, may just hide subtle GC bug.
> 
> Hi. I looked at this again. Running the same test as before (emacs -Q,
> repeatedly creating/destroying client frame) I see:
> 
> 
> - entities are created with each new client frame but are /never/
>   marked.
> 
> - entity-creation backtrace is always
> 
>   #0  0x000000000060e74e in font_make_entity () at font.c:173
>   #1  0x00000000006793ae in ftfont_pattern_entity (p=0xf8c180, extra=20784563) at ftfont.c:215
>   #2  0x000000000067b952 in ftfont_list (f=0x13fb8c0, spec=13463989) at ftfont.c:1057
>   #3  0x0000000000680de6 in xftfont_list (f=0x13fb8c0, spec=13463989) at xftfont.c:138
>   #4  0x0000000000615ebc in font_list_entities (f=0x13fb8c0, spec=20978277) at font.c:2780
>   #5  0x0000000000617c27 in font_find_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933, c=-1) at font.c:3262
>   #6  0x0000000000617fb0 in font_load_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933) at font.c:3335
>   #7  0x00000000006183a2 in font_open_by_spec (f=0x13fb8c0, spec=20082933) at font.c:3429
>   #8  0x0000000000618415 in font_open_by_name (f=0x13fb8c0, name=13702436) at font.c:3440
>   #9  0x000000000052fec4 in x_default_font_parameter (f=0x13fb8c0, parms=20784979) at xfns.c:2904
>   #10 0x0000000000530bc2 in Fx_create_frame (parms=20784979) at xfns.c:3139
> 
> - Vfontset_table has fontsets and font-specs in it, but NO
>   font-entities. Marking from the Vfontset_table thus cannot mark any
>   font entities.
> 
> Where are the entities supposed to be referenced? Does it make sense
> they're never marked?

It's a long time since we last spoke about this, so maybe I've lost
the focus.  We are discussing a problem with leaking memory, right?
If font-entities are related to that, and if not marking them is the
cause of the memory leak, then you are, in effect, saying that when we
GC a font-entity, we should free some additional memory referenced by
that font-entity, is that correct?

If so, the problem is not that font-entities are not marked; that can
only explain why we GC some font-entities which we shouldn't have,
something that cannot possibly lead to a memory leak.  Any Lisp object
that is not marked will be GC'ed during the next GC cycle, so not
marking intermediary short-lived objects should be fine.  However, it
could be that when we GC them, we forget to free some related memory
that is not part of the font-entity object itself -- that could
certainly lead to a leak.

Does this make sense?  Or did I miss something?

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dima Kogan <dima@secretsauce.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>, handa@gnu.org, 21556@debbugs.gnu.org
>> Date: Thu, 29 Oct 2015 15:51:38 -0700
>> 
>> Dmitry Antipov <dmantipov@yandex.ru> writes:
>> 
>> > On 10/01/2015 09:50 PM, Dima Kogan wrote:
>> >
>> >> OK, so are you suggesting changing how mark_face_cache() works? How bad
>> >> is it to accept that fonts and font entities are not necessarily linked,
>> >> and to install the latest patch in this bug?
>> >
>> > I'm suggesting to check whether there are unmarked font objects after marking
>> > from Vfontset_table, and, if so, understand whether it's correct. Otherwise
>> > your patch, even being correct by itself, may just hide subtle GC bug.
>> 
>> Hi. I looked at this again. Running the same test as before (emacs -Q,
>> repeatedly creating/destroying client frame) I see:
>> 
>> 
>> - entities are created with each new client frame but are /never/
>>   marked.
>> 
>> - entity-creation backtrace is always
>> 
>>   #0  0x000000000060e74e in font_make_entity () at font.c:173
>>   #1  0x00000000006793ae in ftfont_pattern_entity (p=0xf8c180, extra=20784563) at ftfont.c:215
>>   #2  0x000000000067b952 in ftfont_list (f=0x13fb8c0, spec=13463989) at ftfont.c:1057
>>   #3  0x0000000000680de6 in xftfont_list (f=0x13fb8c0, spec=13463989) at xftfont.c:138
>>   #4  0x0000000000615ebc in font_list_entities (f=0x13fb8c0, spec=20978277) at font.c:2780
>>   #5  0x0000000000617c27 in font_find_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933, c=-1) at font.c:3262
>>   #6  0x0000000000617fb0 in font_load_for_lface (f=0x13fb8c0, attrs=0x7fff3ee81f50, spec=20082933) at font.c:3335
>>   #7  0x00000000006183a2 in font_open_by_spec (f=0x13fb8c0, spec=20082933) at font.c:3429
>>   #8  0x0000000000618415 in font_open_by_name (f=0x13fb8c0, name=13702436) at font.c:3440
>>   #9  0x000000000052fec4 in x_default_font_parameter (f=0x13fb8c0, parms=20784979) at xfns.c:2904
>>   #10 0x0000000000530bc2 in Fx_create_frame (parms=20784979) at xfns.c:3139
>> 
>> - Vfontset_table has fontsets and font-specs in it, but NO
>>   font-entities. Marking from the Vfontset_table thus cannot mark any
>>   font entities.
>> 
>> Where are the entities supposed to be referenced? Does it make sense
>> they're never marked?
>
> It's a long time since we last spoke about this, so maybe I've lost
> the focus.

I'm forgetting what's happening here too, so trying to finish this bug
now.


> We are discussing a problem with leaking memory, right? If
> font-entities are related to that, and if not marking them is the
> cause of the memory leak, then you are, in effect, saying that when we
> GC a font-entity, we should free some additional memory referenced by
> that font-entity, is that correct?

This isn't quite what's happening here. We have:

- fonts (sometimes) live inside font-entities

- before the patch in this report, the cache compaction code drops font
  entities that aren't marked from the cache

- I'm observing that such unmarked entities sometimes contain marked
  fonts. These become un-cached when their entity is dropped

- Fonts are eventually deallocated by traversing the cache, but these
  un-cached fonts are no longer in the cache, so they leak. So the
  entity isn't the thing that leaks, but fonts that were removed from
  the cache while still used.

- Patch in report looks at the fonts in the entity, and drops the entity
  only if the contained fonts are unmarked also

- Dmitry thinks that unmarked-fonts-inside-a-marked-entity is a
  situation that can't happen, so he requested a deeper look at this. In
  particular, he thinks that after marking from Vfontset_table,
  everything (fonts and entities) should be marked. My latest email was
  in reference to these questions.

Hope this is useful

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: dmantipov@yandex.ru, handa@gnu.org, 21556@debbugs.gnu.org
> Date: Fri, 30 Oct 2015 12:17:36 -0700
> 
> - fonts (sometimes) live inside font-entities

Can you show an example of that, or maybe even tell what path(s)
through the code cause this to happen?  AFAICS, the backtrace you've
shown in your last mail is not such a path: the entity there is some
temporary object that is used to find and open a font, and can
thereafter be forgotten.  Or did I miss something?

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dima Kogan <dima@secretsauce.net>
>> Cc: dmantipov@yandex.ru, handa@gnu.org, 21556@debbugs.gnu.org
>> Date: Fri, 30 Oct 2015 12:17:36 -0700
>> 
>> - fonts (sometimes) live inside font-entities
>
> Can you show an example of that, or maybe even tell what path(s)
> through the code cause this to happen?  AFAICS, the backtrace you've
> shown in your last mail is not such a path: the entity there is some
> temporary object that is used to find and open a font, and can
> thereafter be forgotten.  Or did I miss something?

If you look at the font cache object as you run emacs, you should see
font-containing-entities in it.

I don't believe the entity in that backtrace is temporary. The code is:

    Lisp_Object
    font_list_entities (struct frame *f, Lisp_Object spec)
    {
      .....
                val = driver_list->driver->list (f, scratch_font_spec);
                if (!NILP (val))
                  {
                    Lisp_Object copy = copy_font_spec (scratch_font_spec);

                    val = Fvconcat (1, &val);
                    ASET (copy, FONT_TYPE_INDEX, driver_list->driver->type);
                    XSETCDR (cache, Fcons (Fcons (copy, val), XCDR (cache)));
                  }
      .....


The driver_list->driver->list() call is the one in the backtrace. As you
can see, the results of that call are stored in the font cache. The
questions for Dmitry (or you, Eli, if you know the answer) are

    1. does it make sense that Vfontset_table has no entities in it, and
    thus does not mark them?

    2. what, if anything, should be marking the entities? I'm not seeing
    anything marking them

If the observed behavior is correct (nothing marking the entities), then
the patch in this report is correct also, and we should install it.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Dima Kogan <dima@secretsauce.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> It's a long time since we last spoke about this, so maybe I've lost
>> the focus.
>
> I'm forgetting what's happening here too, so trying to finish this bug
> now.

Hi Eli. It looks like Dmitry is busy. What do you think about merging
the patch now before we both forget what this is all about? When Dmitry
gets time to comment, we can revisit. The concern wasn't that the patch
was "wrong", but rather that it maybe is a workaround for a deeper issue
instead of a fix for the issue itself.

I'm attaching the latest version of the patch. It takes Dmitry's
suggestion for avoiding an extra local variable, and adds some comments


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-compact_font_cache_entry-now-properly-checks-for-mar.patch --]
[-- Type: text/x-diff, Size: 1924 bytes --]

From 6a0c91e750cdab83a63f0ef03ea037b1e6d8d381 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 29 Sep 2015 02:19:35 -0700
Subject: [PATCH] compact_font_cache_entry() now properly checks for marked
 fonts

* src/alloc.c (compact_font_cache_entry): When checking for marked
fonts we were looking at a font entity object.  However the entity
could contain a list of font objects that need to be checked, which we
now do.  This resolves a memory leak
Fixes: bug#21556
---
 src/alloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 3ab2a6e..2cdc581 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5303,10 +5303,31 @@ compact_font_cache_entry (Lisp_Object entry)
 	     are not marked too.  But we must be sure that nothing is
 	     marked within OBJ before we really drop it.  */
 	  for (i = 0; i < size; i++)
-	    if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
-	      break;
+            {
+              Lisp_Object objlist;
+
+              if (VECTOR_MARKED_P (XFONT_ENTITY (AREF (XCDR (obj), i))))
+                break;
+
+              objlist = AREF (AREF (XCDR (obj), i), FONT_OBJLIST_INDEX);
+              for (; CONSP (objlist); objlist = XCDR (objlist))
+                {
+                  Lisp_Object val = XCAR (objlist);
+                  struct font *font = XFONT_OBJECT (val);
+
+                  if (! NILP (AREF (val, FONT_TYPE_INDEX)) &&
+                      VECTOR_MARKED_P(font))
+                    break;
+                }
+              if (CONSP (objlist))
+                // we left the for() early because we found marked
+                // font
+                break;
+            }
 
 	  if (i == size)
+            // we didn't leave the for() early because no marked fonts
+            // or entities were found.
 	    drop = 1;
 	}
       if (drop)
-- 
2.1.4

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: dmantipov@yandex.ru, handa@gnu.org, 21556@debbugs.gnu.org
> Date: Sun, 08 Nov 2015 18:55:06 -0800
> 
> Hi Eli. It looks like Dmitry is busy. What do you think about merging
> the patch now before we both forget what this is all about? When Dmitry
> gets time to comment, we can revisit.

Thanks, pushed with some minor reformatting, to adapt to our coding
conventions.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Lars Ingebrigtsen]

Dima Kogan <dima@secretsauce.net> writes:

> Hi Eli. It looks like Dmitry is busy. What do you think about merging
> the patch now before we both forget what this is all about? When Dmitry
> gets time to comment, we can revisit. The concern wasn't that the patch
> was "wrong", but rather that it maybe is a workaround for a deeper issue
> instead of a fix for the issue itself.

The patch was applied four years ago, but there was no followup on the
possible deeper issue.  Was there more to be done here, or can this be
closed?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  handa@gnu.org,  dmantipov@yandex.ru,
>   21556@debbugs.gnu.org
> Date: Sun, 17 Nov 2019 07:34:09 +0100
> 
> Dima Kogan <dima@secretsauce.net> writes:
> 
> > Hi Eli. It looks like Dmitry is busy. What do you think about merging
> > the patch now before we both forget what this is all about? When Dmitry
> > gets time to comment, we can revisit. The concern wasn't that the patch
> > was "wrong", but rather that it maybe is a workaround for a deeper issue
> > instead of a fix for the issue itself.
> 
> The patch was applied four years ago, but there was no followup on the
> possible deeper issue.  Was there more to be done here, or can this be
> closed?

If Dima has no further suggestions, please close.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Dima Kogan]

Hi. I haven't gone memory-leak hunting in a while, but this patch did plug a sizeable hole with not clear ill effects. I think it's safe to close this bug.

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[Lars Ingebrigtsen]

Dima Kogan <dima@secretsauce.net> writes:

> Hi. I haven't gone memory-leak hunting in a while, but this patch did
> plug a sizeable hole with not clear ill effects. I think it's safe to
> close this bug.

OK; closing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21556: 25.0.50; Memory leak in emacs -Q with lucid (font)

[K. Handa]

In article <83io6wffm9.fsf@gnu.org>, Eli Zaretskii <eliz@gnu.org> writes:

> So maybe we should simply remove (or ifdef away) the code that
> compacts the font caches.  If your assumption about reusing the font
> is anywhere near the truth, compacting the font cache gives us more
> trouble than it gains: we get slow redisplay in some cases and random
> hard-to-debug bugs, while the gains are only visible in very rare use
> cases such as the one described in the Oct 2013 discussion.

That may be an practical workaround at the moment.

But, my design was based on the situation of more than 10 years.
Nowadays, font listing and loading may not be that slow.  And, if the
cache-compacting code itself is doing the right thing, and it just
revealed a bug in core part of font handler, the effort of tracking down
the reason of current crashing may lead to fixing that bug.

---
K. Handa
handa@gnu.org