bug#76121: 31.0.50; crash in uniscribe_close

[Pip Cet]

"Richard Copley" <rcopley@gmail.com> writes:

> Emacs crashes when a font is closed in GC on Windows.
> This happens spontaneously. I have observed it while
> typing into an Emacs window, and on a timer event when no
> frame was visible. I am not sure that deleting a frame is
> necessary, but I can reliably reproduce the crash with
> the following recipe.
>
> Recipe:
>   ## Start Emacs server in gdb. In a command prompt:
>     gdb --args ./emacs -Q --daemon
>     run
>   ## Create a frame. In second command prompt:
>     emacsclient -nc
>   ## In Emacs
>     C-x 5 0 ;; delete-frame
>   ## Create another frame:
>     emacsclient -nc
>   ## Emacs server crashes.
>
> Backtrace:
>
> Thread 1 received signal SIGSEGV, Segmentation fault.
> 0x00007ff7bb45a0bf in release_com (i=0xbfb570) at w32dwrite.c:494
> 494           ((IUnknown *) (*i))->lpVtbl->Release (*i);
> (gdb) print *((IUnknown *) (*i))
> $1 = {
>   lpVtbl = 0x0
> }
> (gdb) bt full
> #0  0x00007ff7bb45a0bf in release_com (i=0xbfb570) at w32dwrite.c:494
> No locals.
> #1  0x00007ff7bb45a284 in w32_dwrite_free_cached_face
> (cache=0x58e2680) at w32dwrite.c:574
> No locals.
> #2  0x00007ff7bb455fbe in uniscribe_close (font=0x6bb2da0) at 
> w32uniscribe.c:206
>         uniscribe_font = 0x6bb2da0

My understanding is that font closing functions such as uniscribe_close
must be written to be idempotent, ensuring they don't close the same
font twice. This is because they can be called explicitly and from GC.

For example, here's w32font_close:

void
w32font_close (struct font *font)
{
  struct w32font_info *w32_font = (struct w32font_info *) font;

  if (w32_font->hfont)
    {
      /* Delete the GDI font object.  */
      DeleteObject (w32_font->hfont);
      w32_font->hfont = NULL;
      ....
    }
}

This will call DeleteObject at most once.

uniscribe_close:

static void
uniscribe_close (struct font *font)
{
  struct uniscribe_font_info *uniscribe_font
    = (struct uniscribe_font_info *) font;

#ifdef HAVE_HARFBUZZ
  w32_dwrite_free_cached_face (uniscribe_font->dwrite_cache);
  if (uniscribe_font->w32_font.font.driver == &harfbuzz_font_driver
      && uniscribe_font->cache)
    hb_font_destroy ((hb_font_t *) uniscribe_font->cache);
  else
#endif
  if (uniscribe_font->cache)
    (*pfnScriptFreeCache) ((SCRIPT_CACHE) &(uniscribe_font->cache));

  uniscribe_font->cache = NULL;

  w32font_close (font);
}

may call w32_dwrite_free_cached_face twice for the same object.

My guess is if we made uniscribe_close return immediately if
uniscribe_font->cache is NULL, this crash might be avoided.

Unless there's something obviously inappropriate with this theory, I'll
provide a patch (but can't really test it very well as I'm not using
Microsoft's Windows implementation).

Pip