Re: emacs-29 0e39ad6fa5: Fix crash after X error

Re: emacs-29 0e39ad6fa5: Fix crash after X error

[Eli Zaretskii]

> diff --git a/src/xdisp.c b/src/xdisp.c
> index 06c8b7730c..4e5250486f 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -16838,6 +16838,13 @@ redisplay_internal (void)
>  		/* Only GC scrollbars when we redisplay the whole frame.  */
>  		= f->redisplay || !REDISPLAY_SOME_P ();
>  	      bool f_redisplay_flag = f->redisplay;
> +
> +	      /* The X error handler may have deleted that frame
> +		 before we went back to retry_frame.  This must come
> +		 before any accesses to f->terminal.  */
> +	      if (!FRAME_LIVE_P (f))
> +		continue;
> +
>  	      /* Mark all the scroll bars to be removed; we'll redeem
>  		 the ones we want when we redisplay their windows.  */
>  	      if (gcscrollbars && FRAME_TERMINAL (f)->condemn_scroll_bars_hook)
> @@ -16845,7 +16852,6 @@ redisplay_internal (void)
>  
>  	      if (FRAME_VISIBLE_P (f) && !FRAME_OBSCURED_P (f))
>  		{
> -
>  		  /* Don't allow freeing images and faces for this
>  		     frame as long as the frame's update wasn't
>  		     completed.  This prevents crashes when some Lisp
> @@ -16861,10 +16867,6 @@ redisplay_internal (void)
>  	      else if (!REDISPLAY_SOME_P ())
>  		f->redisplay = true;
>  
> -	      /* The X error handler may have deleted that frame.  */
> -	      if (!FRAME_LIVE_P (f))
> -		continue;
> -
>  	      /* Any scroll bars which redisplay_windows should have
>  		 nuked should now go away.  */
>  	      if (gcscrollbars && FRAME_TERMINAL (f)->judge_scroll_bars_hook)

Any reason not to have this test in both places?  I don't want to make
unsafe changes on the release branch.

Re: emacs-29 0e39ad6fa5: Fix crash after X error

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> diff --git a/src/xdisp.c b/src/xdisp.c
>> index 06c8b7730c..4e5250486f 100644
>> --- a/src/xdisp.c
>> +++ b/src/xdisp.c
>> @@ -16838,6 +16838,13 @@ redisplay_internal (void)
>>  		/* Only GC scrollbars when we redisplay the whole frame.  */
>>  		= f->redisplay || !REDISPLAY_SOME_P ();
>>  	      bool f_redisplay_flag = f->redisplay;
>> +
>> +	      /* The X error handler may have deleted that frame
>> +		 before we went back to retry_frame.  This must come
>> +		 before any accesses to f->terminal.  */
>> +	      if (!FRAME_LIVE_P (f))
>> +		continue;
>> +
>>  	      /* Mark all the scroll bars to be removed; we'll redeem
>>  		 the ones we want when we redisplay their windows.  */
>>  	      if (gcscrollbars && FRAME_TERMINAL (f)->condemn_scroll_bars_hook)
>> @@ -16845,7 +16852,6 @@ redisplay_internal (void)
>>  
>>  	      if (FRAME_VISIBLE_P (f) && !FRAME_OBSCURED_P (f))
>>  		{
>> -
>>  		  /* Don't allow freeing images and faces for this
>>  		     frame as long as the frame's update wasn't
>>  		     completed.  This prevents crashes when some Lisp
>> @@ -16861,10 +16867,6 @@ redisplay_internal (void)
>>  	      else if (!REDISPLAY_SOME_P ())
>>  		f->redisplay = true;
>>  
>> -	      /* The X error handler may have deleted that frame.  */
>> -	      if (!FRAME_LIVE_P (f))
>> -		continue;
>> -
>>  	      /* Any scroll bars which redisplay_windows should have
>>  		 nuked should now go away.  */
>>  	      if (gcscrollbars && FRAME_TERMINAL (f)->judge_scroll_bars_hook)
>
> Any reason not to have this test in both places?  I don't want to make
> unsafe changes on the release branch.

Sure, please feel free to make that change.

Thanks.