Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Pip Cet]

On Sun, May 31, 2020 at 2:26 PM Eli Zaretskii <eliz@gnu.org> wrote:
> diff --git a/src/xdisp.c b/src/xdisp.c
> index bff6218..60848f4 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -11539,7 +11539,9 @@ display_echo_area_1 (ptrdiff_t a1, Lisp_Object a2)
>    /* Display.  */
>    clear_glyph_matrix (w->desired_matrix);
>    XSETWINDOW (window, w);
> +  void *itdata = bidi_shelve_cache ();

But what if we're out of memory? bidi_shelve_cache will call xmalloc...

(Playing around with this, the composition code also tries to allocate
memory when I call memory_full.)

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 31 May 2020 14:44:54 +0000
> Cc: emacs-diffs@gnu.org
> 
> > +  void *itdata = bidi_shelve_cache ();
> 
> But what if we're out of memory? bidi_shelve_cache will call xmalloc...

xmalloc calls memory_full if that happens.

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Pip Cet]

On Sun, May 31, 2020 at 3:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 31 May 2020 14:44:54 +0000
> > Cc: emacs-diffs@gnu.org
> >
> > > +  void *itdata = bidi_shelve_cache ();
> >
> > But what if we're out of memory? bidi_shelve_cache will call xmalloc...
>
> xmalloc calls memory_full if that happens.

Sorry, should have been more explicit: if we run out of memory,
memory_full will signal an error, after freeing some "spare" memory.
The error handler will call echo_display_area_1. If the bidi iterator
is in use, the cache size is much larger than the spare memory we
freed, so the allocation will also fail, and I'm not sure we ever
recover from that.

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 31 May 2020 15:36:22 +0000
> Cc: emacs-devel@gnu.org, emacs-diffs@gnu.org
> 
> > xmalloc calls memory_full if that happens.
> 
> Sorry, should have been more explicit: if we run out of memory,
> memory_full will signal an error, after freeing some "spare" memory.
> The error handler will call echo_display_area_1. If the bidi iterator
> is in use, the cache size is much larger than the spare memory we
> freed, so the allocation will also fail, and I'm not sure we ever
> recover from that.

Yes, our code is full of such places.  It would be good to improve the
recovery in these cases.

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Eli Zaretskii]

Btw, please don't CC emacs-diffs, that list should be free of any
messages that aren't repository diffs.

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Pip Cet]

On Sun, May 31, 2020 at 3:46 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 31 May 2020 15:36:22 +0000

> > Sorry, should have been more explicit: if we run out of memory,
> > memory_full will signal an error, after freeing some "spare" memory.
> > The error handler will call echo_display_area_1. If the bidi iterator
> > is in use, the cache size is much larger than the spare memory we
> > freed, so the allocation will also fail, and I'm not sure we ever
> > recover from that.
> Yes, our code is full of such places.  It would be good to improve the
> recovery in these cases.

I've played around a little, and when resize-mini-windows is nil, it's
quite easy to get into a loop where the OOM message isn't displayed.
Without the patch, recovery sometimes works. Maybe it's just time to
update SPARE_SIZE to reflect the amount of memory the bidi iterator
currently needs.

Re: emacs-27 41232e6: Avoid crashes due to bidi cache being reset during redisplay

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 31 May 2020 16:24:23 +0000
> Cc: emacs-devel@gnu.org
> 
> I've played around a little, and when resize-mini-windows is nil, it's
> quite easy to get into a loop where the OOM message isn't displayed.
> Without the patch, recovery sometimes works. Maybe it's just time to
> update SPARE_SIZE to reflect the amount of memory the bidi iterator
> currently needs.

Feel free to suggest a patch for the master branch along those lines.