[PATCH] On the nasty "ghost key" problem on NS

[PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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

Hi all,

Attached is a one-liner patch to the “ghost key” problem on NS. (The name is made up by me.) I didn’t have luck finding a bug report (but it did happen to many to-be-liberated Mac users [1-3]), so I decided to directly send it here.

On certain occasions (*), the key event buffer is not cleared to be empty even if the events have been processed. This results in a nasty problem: when you press a key, say “y”, a key that is pressed earlier will be prepended to the current key. There’s no way to reset the event buffer.

This problem is highly unpredictable, making it very hard to reproduce. A probably quicker way: hack Emacs’ C codebase with corfu-auto and citre-mode.

(*) The root cause is still not clear to me. It’s observed that sometimes, the [interpretKeyEvents] call seems not to return. (Blocked inside Cocoa?) And the supposedly matched “remove” is never called. Now “nsEvArray” is left with an earlier keyDown event, and since “nsEvArray” is static, you have the problem as described above. I guess it may be some Cocoa internal changes that caused this.

This patch is just a workaround, but it improves the current user experience a bit.

Regards
Kai

[1] https://emacs-china.org/t/emacs-rime-e/20286
[2] https://emacs-china.org/t/emacs/15430
[3] https://emacs-china.org/t/topic/13207


[-- Attachment #2: trace.org --]
[-- Type: application/octet-stream, Size: 4897 bytes --]

* trace1
#+begin_src fundamental

     ,*1 #nsEvArray = 0
  keyDown: code =6d	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:27:47.272 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:47.273 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:47.273 emacs[46756:2752417] insertText 'm'	len = 1
     event removed...
     ,*3 #nsEvArray = 0
     ,*1 #nsEvArray = 0
  keyDown: code =61	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:27:47.297 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:47.297 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:47.298 emacs[46756:2752417] insertText 'a'	len = 1
     event removed...
     ,*3 #nsEvArray = 0
     ,*1 #nsEvArray = 0
  keyDown: code =63	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:27:47.727 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:47.727 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:47.729 emacs[46756:2752417] insertText 'c'	len = 1
     event removed...
     ,*3 #nsEvArray = 0
     ,*1 #nsEvArray = 0
  keyDown: code =73	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:27:47.732 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:47.732 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:48.424 emacs[46756:2752417] selectedRange request
     ,*1 #nsEvArray = 1
     event added...
     ,*2 #nsEvArray = 2
  2022-11-04 00:27:50.028 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:50.028 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:50.029 emacs[46756:2752417] insertText 's'	len = 1
  2022-11-04 00:27:50.030 emacs[46756:2752417] selectedRange request
  2022-11-04 00:27:50.030 emacs[46756:2752417] firstRectForCharRange request
  2022-11-04 00:27:50.030 emacs[46756:2752417] insertText 's'	len = 1
     event removed...
     ,*3 #nsEvArray = 1
#+end_src
* trace2
#+begin_src fundamental
     ,*1 #nsEvArray = 0
  keyDown: code =73	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:08:38.627 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:38.627 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:38.628 emacs[43626:2728428] insertText 's'	len = 1
     event removed...
     ,*3 #nsEvArray = 0
     ,*1 #nsEvArray = 0
  keyDown: code =65	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:08:38.973 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:38.973 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:38.973 emacs[43626:2728428] insertText 'e'	len = 1
     event removed...
     ,*3 #nsEvArray = 0
     ,*1 #nsEvArray = 0
  keyDown: code =6c	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 1
  2022-11-04 00:08:38.976 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:38.976 emacs[43626:2728428] firstRectForCharRange request
     ,*1 #nsEvArray = 1
     event added...
     ,*2 #nsEvArray = 2
  2022-11-04 00:08:39.639 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:39.639 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:39.640 emacs[43626:2728428] insertText 'l'	len = 1
  2022-11-04 00:08:39.641 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:39.641 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:39.641 emacs[43626:2728428] insertText 'f'	len = 1
     event removed...
     ,*3 #nsEvArray = 1
     ,*1 #nsEvArray = 1
  keyDown: code =20	fnKey =0	flags = 100	mods = 0
  keyDown: Begin compose sequence.
     event added...
     ,*2 #nsEvArray = 2
  2022-11-04 00:08:40.776 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:40.776 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:40.776 emacs[43626:2728428] insertText 'l'	len = 1
  2022-11-04 00:08:40.777 emacs[43626:2728428] selectedRange request
  2022-11-04 00:08:40.777 emacs[43626:2728428] firstRectForCharRange request
  2022-11-04 00:08:40.778 emacs[43626:2728428] insertText ' '	len = 1
     event removed...
     ,*3 #nsEvArray = 1
#+end_src

[-- Attachment #3: fix-ghost-key.patch --]
[-- Type: application/octet-stream, Size: 412 bytes --]

diff --git a/src/nsterm.m b/src/nsterm.m
index 17f40dc7e3..0a90e85e15 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6904,7 +6904,7 @@ In that case we use UCKeyTranslate (ns_get_shifted_character)
   /* FIXME: Use [NSArray arrayWithObject:theEvent]?  */
   [nsEvArray addObject: theEvent];
   [self interpretKeyEvents: nsEvArray];
-  [nsEvArray removeObject: theEvent];
+  [nsEvArray removeAllObjects];
 }
 
 

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

> Hi all,
>
> Attached is a one-liner patch to the “ghost key” problem on NS. (The name is made up by me.) I didn’t have luck finding a bug report (but it did happen to many to-be-liberated Mac users [1-3]), so I decided to directly send it here.
>
> On certain occasions (*), the key event buffer is not cleared to be
> empty even if the events have been processed. This results in a nasty
> problem: when you press a key, say “y”, a key that is pressed earlier
> will be prepended to the current key. There’s no way to reset the
> event buffer.
>
> This problem is highly unpredictable, making it very hard to reproduce. A probably quicker way: hack Emacs’ C codebase with corfu-auto and citre-mode.
>
> (*) The root cause is still not clear to me. It’s observed that
> sometimes, the [interpretKeyEvents] call seems not to return. (Blocked
> inside Cocoa?) And the supposedly matched “remove” is never
> called. Now “nsEvArray” is left with an earlier keyDown event, and
> since “nsEvArray” is static, you have the problem as described
> above. I guess it may be some Cocoa internal changes that caused this.
>
> This patch is just a workaround, but it improves the current user experience a bit.

I am sorry, but this patch breaks compose processing.  So we're
certainly not installing anything like this in Emacs 29.

I will try to figure out a way to reproduce the bug and why it actually
happens.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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


> On Nov 4, 2022, at 08:32, Po Lu <luangruo@yahoo.com> wrote:
> 
> Kai Ma <justksqsf@gmail.com <mailto:justksqsf@gmail.com>> writes:
> 
> I am sorry, but this patch breaks compose processing.  So we're
> certainly not installing anything like this in Emacs 29.
> 
> I will try to figure out a way to reproduce the bug and why it actually
> happens.

Thanks!

I’ve narrowed the problem further to [firstRectForCharRange]. Some calls never return. Activating an input method makes Emacs issue excessive calls to this method. Specifically, the following code gets stuck somewhere.

  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
    win = XWINDOW (echo_area_window);
  else
    win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));


[-- Attachment #2: Type: text/html, Size: 5758 bytes --]

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

> I’ve narrowed the problem further to [firstRectForCharRange].

I'm either blind, or there is no method by that name...

> Some calls never return. Activating an input method makes Emacs issue
> excessive calls to this method. Specifically, the following code gets
> stuck somewhere.
>
>   if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
>     win = XWINDOW (echo_area_window);
>   else
>     win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));

So Emacs completely locks up in that function (ns-in-echo-area)?

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

> On Nov 4, 2022, at 15:16, Po Lu <luangruo@yahoo.com> wrote:
> 
> Kai Ma <justksqsf@gmail.com> writes:
> 
>> I’ve narrowed the problem further to [firstRectForCharRange].
> 
> I'm either blind, or there is no method by that name...

firstRectForCharacterRange.

> 
>> Some calls never return. Activating an input method makes Emacs issue
>> excessive calls to this method. Specifically, the following code gets
>> stuck somewhere.
>> 
>>  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
>>    win = XWINDOW (echo_area_window);
>>  else
>>    win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
> 
> So Emacs completely locks up in that function (ns-in-echo-area)?


Yes and no. Emacs still responds to new key events, but a previous call is stuck and does not return. Presumably in another thread. To be more clear: changing that code to be

  static int enter_cnt, leave_cnt;
  enter_cnt++;
  [referenced piece of code]
  leave_cnt++;
  /* (Or just count the return/leave counts of (ns-in-echo-area).) */

leave_cnt can be less than enter_cnt. I’ve confirmed re-entrance to [firstRectForCharacterRange] leads to the problem.

Pardon my ignorance, is there any reason to special-case the echo area? Simply let win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe)) and the problem is gone.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

> Yes and no. Emacs still responds to new key events, but a previous
> call is stuck and does not return. Presumably in another thread. To be
> more clear: changing that code to be
>
>   static int enter_cnt, leave_cnt;
>   enter_cnt++;
>   [referenced piece of code]
>   leave_cnt++;
>   /* (Or just count the return/leave counts of (ns-in-echo-area).) */
>
> leave_cnt can be less than enter_cnt. I’ve confirmed re-entrance to
> [firstRectForCharacterRange] leads to the problem.

I'm going to guess what actually happened is that ns-in-echo-area
signalled.

What happens if you replace ns-in-echo-area with:

  safe_call (0, Qns_in_echo_area)

?

> Pardon my ignorance, is there any reason to special-case the echo
> area? Simply let win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe))
> and the problem is gone.

I don't know.  I'd rather leave as much code untouched as possible.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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



> On Nov 4, 2022, at 17:29, Po Lu <luangruo@yahoo.com> wrote:
> 
> Kai Ma <justksqsf@gmail.com <mailto:justksqsf@gmail.com>> writes:
> 
>> Yes and no. Emacs still responds to new key events, but a previous
>> call is stuck and does not return. Presumably in another thread. To be
>> more clear: changing that code to be
>> 
>>  static int enter_cnt, leave_cnt;
>>  enter_cnt++;
>>  [referenced piece of code]
>>  leave_cnt++;
>>  /* (Or just count the return/leave counts of (ns-in-echo-area).) */
>> 
>> leave_cnt can be less than enter_cnt. I’ve confirmed re-entrance to
>> [firstRectForCharacterRange] leads to the problem.
> 
> I'm going to guess what actually happened is that ns-in-echo-area
> signalled.
> 
> What happens if you replace ns-in-echo-area with:
> 
>  safe_call (0, Qns_in_echo_area)
> 
> ?

Yes, this indeed is related to signals, but it’s not (ns-in-echo-area) that signals. It seems that there are problems in nsterm.m regarding waiting_for_input, which caused aborts for me ([eval.c:signal_or_quit] asserts !waiting_for_input). It’s also observed that inhibit-quit must be set, or the bug persists.

I believe the attached patch fixes this bug, but I don’t know why Vthrow_on_input is not correctly handled by safe_call. 



[-- Attachment #2.1: Type: text/html, Size: 8557 bytes --]

[-- Attachment #2.2: fix-ghost-key-v2.patch --]
[-- Type: application/octet-stream, Size: 2318 bytes --]

diff --git a/src/lisp.h b/src/lisp.h
index eafa241adf..2ba18ec684 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4603,6 +4603,7 @@ xsignal (Lisp_Object error_symbol, Lisp_Object data)
 extern Lisp_Object call_debugger (Lisp_Object arg);
 extern void init_eval_once (void);
 extern Lisp_Object safe_call (ptrdiff_t, Lisp_Object, ...);
+extern Lisp_Object safe_call_inhibit_quit (ptrdiff_t, Lisp_Object, ...);
 extern Lisp_Object safe_call1 (Lisp_Object, Lisp_Object);
 extern Lisp_Object safe_call2 (Lisp_Object, Lisp_Object, Lisp_Object);
 extern void init_eval (void);
diff --git a/src/nsterm.m b/src/nsterm.m
index 17f40dc7e3..e4b998410d 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7063,16 +7063,20 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
   NSRect rect;
   NSPoint pt;
   struct window *win;
+  bool owfi;
 
   NSTRACE ("[EmacsView firstRectForCharacterRange:]");
 
   if (NS_KEYLOG)
     NSLog (@"firstRectForCharRange request");
 
-  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
+  owfi = waiting_for_input;
+  waiting_for_input = false;
+  if (WINDOWP (echo_area_window) && ! NILP (safe_call_inhibit_quit (0, Qns_in_echo_area)))
     win = XWINDOW (echo_area_window);
   else
     win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+  waiting_for_input = owfi;
 
   rect.size.width = theRange.length * FRAME_COLUMN_WIDTH (emacsframe);
   rect.size.height = FRAME_LINE_HEIGHT (emacsframe);
@@ -11012,6 +11016,7 @@ Nil means use fullscreen the old (< 10.7) way.  The old way works better with
   DEFSYM (Qcondensed, "condensed");
   DEFSYM (Qreverse_italic, "reverse-italic");
   DEFSYM (Qexpanded, "expanded");
+  DEFSYM (Qns_in_echo_area, "ns-in-echo-area")
 
 #ifdef NS_IMPL_COCOA
   Fprovide (Qcocoa, Qnil);
diff --git a/src/xdisp.c b/src/xdisp.c
index dd243eca98..af697d0050 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3041,6 +3041,18 @@ safe_call (ptrdiff_t nargs, Lisp_Object func, ...)
   return retval;
 }
 
+Lisp_Object
+safe_call_inhibit_quit (ptrdiff_t nargs, Lisp_Object func, ...)
+{
+  Lisp_Object retval;
+  va_list ap;
+
+  va_start (ap, func);
+  retval = safe__call (true, nargs, func, ap);
+  va_end (ap);
+  return retval;
+}
+
 /* Call function FN with one argument ARG.
    Return the result, or nil if something went wrong.  */
 

[-- Attachment #2.3: Type: text/html, Size: 212 bytes --]

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

> +  owfi = waiting_for_input;
> +  waiting_for_input = false;
> +  if (WINDOWP (echo_area_window) && ! NILP (safe_call_inhibit_quit (0, Qns_in_echo_area)))
>      win = XWINDOW (echo_area_window);
>    else
>      win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
> +  waiting_for_input = owfi;

Please rename `owfi' to `was_waiting_for_input' or some other more
descriptive name.  What happens if you just block input around safe_call
(0, Qns_in_echo_area), instead of making an extra function that passes a
magic flag to safe__call?

> +Lisp_Object
> +safe_call_inhibit_quit (ptrdiff_t nargs, Lisp_Object func, ...)
> +{
> +  Lisp_Object retval;
> +  va_list ap;
> +
> +  va_start (ap, func);
> +  retval = safe__call (true, nargs, func, ap);
> +  va_end (ap);
> +  return retval;
> +}
> +

This seems extraneous to me.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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



> On Nov 4, 2022, at 19:27, Po Lu <luangruo@yahoo.com> wrote:
> 
> Kai Ma <justksqsf@gmail.com> writes:
> 
>> +  owfi = waiting_for_input;
>> +  waiting_for_input = false;
>> +  if (WINDOWP (echo_area_window) && ! NILP (safe_call_inhibit_quit (0, Qns_in_echo_area)))
>>     win = XWINDOW (echo_area_window);
>>   else
>>     win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
>> +  waiting_for_input = owfi;
> 
> Please rename `owfi' to `was_waiting_for_input' or some other more
> descriptive name.  What happens if you just block input around safe_call
> (0, Qns_in_echo_area), instead of making an extra function that passes a
> magic flag to safe__call?

Either safe_call alone or safe_call+block_input does not fix the bug.

>> +Lisp_Object
>> +safe_call_inhibit_quit (ptrdiff_t nargs, Lisp_Object func, ...)
>> +{
>> +  Lisp_Object retval;
>> +  va_list ap;
>> +
>> +  va_start (ap, func);
>> +  retval = safe__call (true, nargs, func, ap);
>> +  va_end (ap);
>> +  return retval;
>> +}
>> +
> 
> This seems extraneous to me.


Changed to specbind(Qinhibit_quit, Qt).


[-- Attachment #2: fix-ghost-key-v3.patch --]
[-- Type: application/octet-stream, Size: 1352 bytes --]

diff --git a/src/nsterm.m b/src/nsterm.m
index 17f40dc7e3..0788442b98 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7063,16 +7063,23 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
   NSRect rect;
   NSPoint pt;
   struct window *win;
+  bool was_waiting_for_input;
+  specpdl_ref count = SPECPDL_INDEX ();
 
   NSTRACE ("[EmacsView firstRectForCharacterRange:]");
 
   if (NS_KEYLOG)
     NSLog (@"firstRectForCharRange request");
 
-  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
+  was_waiting_for_input = waiting_for_input;
+  waiting_for_input = false;
+  specbind (Qinhibit_quit, Qt);
+  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))
     win = XWINDOW (echo_area_window);
   else
     win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+  unbind_to (count, Qnil);
+  waiting_for_input = was_waiting_for_input;
 
   rect.size.width = theRange.length * FRAME_COLUMN_WIDTH (emacsframe);
   rect.size.height = FRAME_LINE_HEIGHT (emacsframe);
@@ -11012,6 +11019,7 @@ Nil means use fullscreen the old (< 10.7) way.  The old way works better with
   DEFSYM (Qcondensed, "condensed");
   DEFSYM (Qreverse_italic, "reverse-italic");
   DEFSYM (Qexpanded, "expanded");
+  DEFSYM (Qns_in_echo_area, "ns-in-echo-area");
 
 #ifdef NS_IMPL_COCOA
   Fprovide (Qcocoa, Qnil);

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

> Either safe_call alone or safe_call+block_input does not fix the bug.

Okay, I see.

Would people on NS run this patch for a while and ack?  I don't see any
adverse affects on GNUstep.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Stefan Monnier]

Po Lu [2022-11-04 20:17:29] wrote:
> Kai Ma <justksqsf@gmail.com> writes:
>> Either safe_call alone or safe_call+block_input does not fix the bug.
> Okay, I see.
> Would people on NS run this patch for a while and ack?  I don't see any
> adverse affects on GNUstep.

Just a comment from someone who's rather unfamiliar with the way input
methods work in Emacs (well, I use `TeX`, but that's about it), and even
more so for the NS code in general:

    diff --git a/src/nsterm.m b/src/nsterm.m
    index 17f40dc7e3..0788442b98 100644
    --- a/src/nsterm.m
    +++ b/src/nsterm.m
    @@ -7063,16 +7063,23 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
       NSRect rect;
       NSPoint pt;
       struct window *win;
    +  bool was_waiting_for_input;
    +  specpdl_ref count = SPECPDL_INDEX ();
     
       NSTRACE ("[EmacsView firstRectForCharacterRange:]");
     
       if (NS_KEYLOG)
         NSLog (@"firstRectForCharRange request");
     
    -  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
    +  was_waiting_for_input = waiting_for_input;
    +  waiting_for_input = false;
    +  specbind (Qinhibit_quit, Qt);
    +  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))

I'm glad we found a way to make the code work, apparently, but
Here we need a comment explaining why we do this gymnastic of
`safe_call` + `inhibit_quit` + `waiting_for_input`.

It's very unusual to have to do that.

         win = XWINDOW (echo_area_window);
       else
         win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
    +  unbind_to (count, Qnil);
    +  waiting_for_input = was_waiting_for_input;

The `unbind_to` suggests that non-local exits are still possible, but if
so, we will sometimes not restore `waiting_for_input`.  So a comment
here would be welcome explaining either that non-local exits aren't
possible, or why it's OK not to restore `waiting_for_input`.
Or maybe change the code to use an unwind_protect to reliably restore
`waiting_for_input` so we don't have to worry about it?

    +  DEFSYM (Qns_in_echo_area, "ns-in-echo-area");

Thanks :-)

Now for the other part of my comment:

    (defun ns-in-echo-area ()
      "Whether, for purposes of inserting working composition text, the minibuffer
    is currently being used."
      (or isearch-mode
          (and cursor-in-echo-area (current-message))
          ;; Overlay strings are not shown in some cases.
          (get-char-property (point) 'invisible)
          (and (not (bobp))
    	   (or (and (get-char-property (point) 'display)
    		    (eq (get-char-property (1- (point)) 'display)
    			(get-char-property (point) 'display)))
    	       (and (get-char-property (point) 'composition)
    		    (eq (get-char-property (1- (point)) 'composition)
    			(get-char-property (point) 'composition)))))))

In which sense does (get-char-property (point) 'invisible), or the big
(and ...) below, indicate that "the minibuffer is currently being used"?

Could someone clarify this code?  Maybe all it takes is to change the
docstring, I don't know, but as it stands it looks quite odd.


        Stefan

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Just a comment from someone who's rather unfamiliar with the way input
> methods work in Emacs (well, I use `TeX`, but that's about it), and even
> more so for the NS code in general:
>
>     diff --git a/src/nsterm.m b/src/nsterm.m
>     index 17f40dc7e3..0788442b98 100644
>     --- a/src/nsterm.m
>     +++ b/src/nsterm.m
>     @@ -7063,16 +7063,23 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
>        NSRect rect;
>        NSPoint pt;
>        struct window *win;
>     +  bool was_waiting_for_input;
>     +  specpdl_ref count = SPECPDL_INDEX ();
>      
>        NSTRACE ("[EmacsView firstRectForCharacterRange:]");
>      
>        if (NS_KEYLOG)
>          NSLog (@"firstRectForCharRange request");
>      
>     -  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
>     +  was_waiting_for_input = waiting_for_input;
>     +  waiting_for_input = false;
>     +  specbind (Qinhibit_quit, Qt);
>     +  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))
>
> I'm glad we found a way to make the code work, apparently, but
> Here we need a comment explaining why we do this gymnastic of
> `safe_call` + `inhibit_quit` + `waiting_for_input`.

That is done all over the place in the NS code.  I don't really know
why, you will have to ask its original authors for that, but suffice it
to say calling Lisp from firstRectForCharacterRange (and also the menu
bar update callbacks) will otherwise crash upon Fsignal being called.

> It's very unusual to have to do that.
>
>          win = XWINDOW (echo_area_window);
>        else
>          win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
>     +  unbind_to (count, Qnil);
>     +  waiting_for_input = was_waiting_for_input;
>
> The `unbind_to` suggests that non-local exits are still possible, but if
> so, we will sometimes not restore `waiting_for_input`.

I think the unbind_to is just a general pattern and could easily be
replaced with "Vinhibit_quit = old_inhibit_quit" (and so on and so
forth.)

> Now for the other part of my comment:
>
>     (defun ns-in-echo-area ()
>       "Whether, for purposes of inserting working composition text, the minibuffer
>     is currently being used."
>       (or isearch-mode
>           (and cursor-in-echo-area (current-message))
>           ;; Overlay strings are not shown in some cases.
>           (get-char-property (point) 'invisible)
>           (and (not (bobp))
>     	   (or (and (get-char-property (point) 'display)
>     		    (eq (get-char-property (1- (point)) 'display)
>     			(get-char-property (point) 'display)))
>     	       (and (get-char-property (point) 'composition)
>     		    (eq (get-char-property (1- (point)) 'composition)
>     			(get-char-property (point) 'composition)))))))
>
> In which sense does (get-char-property (point) 'invisible), or the big
> (and ...) below, indicate that "the minibuffer is currently being used"?

I really have no idea...

> Could someone clarify this code?  Maybe all it takes is to change the
> docstring, I don't know, but as it stands it looks quite odd.

+1

Re: [PATCH] On the nasty "ghost key" problem on NS

[Stefan Monnier]

>>     -  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
>>     +  was_waiting_for_input = waiting_for_input;
>>     +  waiting_for_input = false;
>>     +  specbind (Qinhibit_quit, Qt);
>>     +  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))
>>
>> I'm glad we found a way to make the code work, apparently, but
>> Here we need a comment explaining why we do this gymnastic of
>> `safe_call` + `inhibit_quit` + `waiting_for_input`.
>
> That is done all over the place in the NS code.

Then why does it need to be hand-coded here?  If it's done all over the
place, it should have its own `super_extra_safe_call` function or
something, no?

> I don't really know why, you will have to ask its original authors for
> that, but suffice it to say calling Lisp from
> firstRectForCharacterRange (and also the menu bar update callbacks)
> will otherwise crash upon Fsignal being called.

Yet I don't see anything in `ns-in-echo-area` which would call `signal`.
I don't mean to say that we should not protect ourselves from the case
where `ns-in-echo-area` calls `signal`, but that the above explanation
doesn't seem to explain the problem we're currently facing.
[ And `safe_call` should be sufficient to protect ourselves from
  `signal`.  ]


        Stefan

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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


> On Nov 5, 2022, at 22:40, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>>    -  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
>>>    +  was_waiting_for_input = waiting_for_input;
>>>    +  waiting_for_input = false;
>>>    +  specbind (Qinhibit_quit, Qt);
>>>    +  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))
>>> 
>>> I'm glad we found a way to make the code work, apparently, but
>>> Here we need a comment explaining why we do this gymnastic of
>>> `safe_call` + `inhibit_quit` + `waiting_for_input`.
>> 
>> That is done all over the place in the NS code.
> 
> Then why does it need to be hand-coded here?  If it's done all over the
> place, it should have its own `super_extra_safe_call` function or
> something, no?
> 
>> I don't really know why, you will have to ask its original authors for
>> that, but suffice it to say calling Lisp from
>> firstRectForCharacterRange (and also the menu bar update callbacks)
>> will otherwise crash upon Fsignal being called.
> 
> Yet I don't see anything in `ns-in-echo-area` which would call `signal`.
> I don't mean to say that we should not protect ourselves from the case
> where `ns-in-echo-area` calls `signal`, but that the above explanation
> doesn't seem to explain the problem we're currently facing.
> [ And `safe_call` should be sufficient to protect ourselves from
>  `signal`.  ]

I’m not super familiar with the signal mechanism, but here are some findings.  (Assume that `waiting_for_input` is correctly maintained.)  On certain occasions (which still remain unclear to me), the `Vthrow_on_input` path in `process_quit_flag` is taken.  The curious thing is that `safe_call` does not seem to catch that, and thus the control flow directly moves to somewhere above the Lisp call in `firstRectForCharacterRange`.  Is it intentional that `safe_call` does not catch throw_on_input?

(Also a correction: I guessed it could be related to threading at first. No, it’s not. It’s always the main thread.)

[-- Attachment #2: Type: text/html, Size: 2725 bytes --]

Re: [PATCH] On the nasty "ghost key" problem on NS

[Stefan Monnier]

> I’m not super familiar with the signal mechanism, but here are some
> findings.  (Assume that `waiting_for_input` is correctly maintained.)  On
> certain occasions (which still remain unclear to me), the `Vthrow_on_input`
> path in `process_quit_flag` is taken.  The curious thing is that `safe_call`
> does not seem to catch that, and thus the control flow directly moves to
> somewhere above the Lisp call in `firstRectForCharacterRange`.  Is it
> intentional that `safe_call` does not catch throw_on_input?

The implementation of `safe_call` protects against uses of `signal` but
not `throw`.  Is this intentional?  Good question.  AFAIK we don't
currently have a mechanism to catch all throws like we have for signals,
so that might be the explanation.

FWIW, my initial implementation of `while-no-input` used `signal` rather
than `throw` and Richard insisted that it was wrong and should use
`throw` instead.  I couldn't see any reason to prefer one over the other
(except that it was easier to use `signal`, hence my original choice).

AFAICT this is the first time I encounter where it seems to make
a difference :-)
Maybe this is telling us that I was right all along and `while-no-input`
should use `signal` rather than `throw`?  :-)

Then again, `ns-in-echo-area` and other functions called by `safe_call`
could explicitly call `throw` for all kinds of reasons, and it's not
clear what we should do in those cases: should we disallow/catch them
all, or let them all through?  Something in-between?

I wonder why it matters here and not in other uses of `safe_call`?

> (Also a correction: I guessed it could be related to threading at first.
> No, it’s not.  It’s always the main thread.)

Thanks.  That's good to know.


        Stefan

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Then why does it need to be hand-coded here?  If it's done all over the
> place, it should have its own `super_extra_safe_call` function or
> something, no?

I don't know, that's just how it is.

> Yet I don't see anything in `ns-in-echo-area` which would call `signal`.

Something may do that, I'm not sure what.

> [ And `safe_call` should be sufficient to protect ourselves from
>   `signal`.  ]

signal_or_quit calls abort if waiting_for_input.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Kai Ma]

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


> On Nov 4, 2022, at 23:09, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
> I'm glad we found a way to make the code work, apparently, but
> Here we need a comment explaining why we do this gymnastic of
> `safe_call` + `inhibit_quit` + `waiting_for_input`.
> It's very unusual to have to do that.

Thanks for the pointer, this indeed turned out unnecessary (and dangerous).

A tester informed me of a problem in the v3 patch:

  safe_call (0, Qns_in_echo_area)

is incorrect. The 0 should be 1, or nargs will be -1 for funcall_general.  
This will cause an error to be signaled, which explains why 
`waiting_for_input` has to be masked.

[ I guess we could add an assertion that numargs >= 0 in funcall_general or
  somewhere else? ]

This patch should be correct even without the ugly `waiting_for_input` hack.
I’ve been running patched Emacs for some time without issues.

With the current understanding of the bug, I guess the comment line could be
  /* Protect against throw-on-input. */

WDYT?



[-- Attachment #2.1: Type: text/html, Size: 2702 bytes --]

[-- Attachment #2.2: fix-ghost-key-v4.patch --]
[-- Type: application/octet-stream, Size: 1193 bytes --]

diff --git a/src/nsterm.m b/src/nsterm.m
index 17f40dc7e3..0ed047849a 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7063,16 +7063,19 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
   NSRect rect;
   NSPoint pt;
   struct window *win;
+  specpdl_ref count = SPECPDL_INDEX ();
 
   NSTRACE ("[EmacsView firstRectForCharacterRange:]");
 
   if (NS_KEYLOG)
     NSLog (@"firstRectForCharRange request");
 
-  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
+  specbind (Qinhibit_quit, Qt);
+  if (WINDOWP (echo_area_window) && ! NILP (safe_call (1, Qns_in_echo_area)))
     win = XWINDOW (echo_area_window);
   else
     win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+  unbind_to (count, Qnil);
 
   rect.size.width = theRange.length * FRAME_COLUMN_WIDTH (emacsframe);
   rect.size.height = FRAME_LINE_HEIGHT (emacsframe);
@@ -11012,6 +11015,7 @@ Nil means use fullscreen the old (< 10.7) way.  The old way works better with
   DEFSYM (Qcondensed, "condensed");
   DEFSYM (Qreverse_italic, "reverse-italic");
   DEFSYM (Qexpanded, "expanded");
+  DEFSYM (Qns_in_echo_area, "ns-in-echo-area");
 
 #ifdef NS_IMPL_COCOA
   Fprovide (Qcocoa, Qnil);

[-- Attachment #2.3: Type: text/html, Size: 223 bytes --]

Re: [PATCH] On the nasty "ghost key" problem on NS

[Po Lu]

Kai Ma <justksqsf@gmail.com> writes:

>  On Nov 4, 2022, at 23:09, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
>  I'm glad we found a way to make the code work, apparently, but
>  Here we need a comment explaining why we do this gymnastic of
>  `safe_call` + `inhibit_quit` + `waiting_for_input`.
>
>  It's very unusual to have to do that.
>
> Thanks for the pointer, this indeed turned out unnecessary (and dangerous).
>
> A tester informed me of a problem in the v3 patch:
>
>   safe_call (0, Qns_in_echo_area)
>
> is incorrect. The 0 should be 1, or nargs will be -1 for funcall_general.  
> This will cause an error to be signaled, which explains why 
> `waiting_for_input` has to be masked.
>
> [ I guess we could add an assertion that numargs >= 0 in funcall_general or
>   somewhere else? ]
>
> This patch should be correct even without the ugly `waiting_for_input` hack.
> I’ve been running patched Emacs for some time without issues.
>
> With the current understanding of the bug, I guess the comment line could be
>   /* Protect against throw-on-input. */

I will install a change using internal_catch_all instead, if Stefan is
fine with that.  By doing so, you get all of the benefits of safe_call
and inhibit_quit, and it also protects against faulty code that changes
the value of inhibit-quit inside the Lisp function.

Re: [PATCH] On the nasty "ghost key" problem on NS

[Stefan Monnier]

Po Lu [2022-11-10 20:25:03] wrote:

> Kai Ma <justksqsf@gmail.com> writes:
>
>>  On Nov 4, 2022, at 23:09, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>
>>  I'm glad we found a way to make the code work, apparently, but
>>  Here we need a comment explaining why we do this gymnastic of
>>  `safe_call` + `inhibit_quit` + `waiting_for_input`.
>>
>>  It's very unusual to have to do that.
>>
>> Thanks for the pointer, this indeed turned out unnecessary (and dangerous).
>>
>> A tester informed me of a problem in the v3 patch:
>>
>>   safe_call (0, Qns_in_echo_area)
>>
>> is incorrect. The 0 should be 1, or nargs will be -1 for funcall_general.  
>> This will cause an error to be signaled, which explains why 
>> `waiting_for_input` has to be masked.
>>
>> [ I guess we could add an assertion that numargs >= 0 in funcall_general or
>>   somewhere else? ]
>>
>> This patch should be correct even without the ugly `waiting_for_input` hack.
>> I’ve been running patched Emacs for some time without issues.
>>
>> With the current understanding of the bug, I guess the comment line could be
>>   /* Protect against throw-on-input. */
>
> I will install a change using internal_catch_all instead, if Stefan is
> fine with that.

I don't have any objection to any of the patches that were proposed.
My only point was that it looked like we were getting to the point where
we "add hacks upon hacks for lack of understanding".
Once we start doing that it's very important to add as much context in
the form of comments in order to help the poor sod who needs to deal
with the next problem (in the hope that they will get a chance to
actually understand at some point what is the true underlying problem).


        Stefan

Re: [PATCH] On the nasty "ghost key" problem on NS

[Eli Zaretskii]

> From: Kai Ma <justksqsf@gmail.com>
> Date: Fri, 4 Nov 2022 16:53:34 +0800
> Cc: emacs-devel@gnu.org
> 
> Pardon my ignorance, is there any reason to special-case the echo area?

What window do you think FRAME_SELECTED_WINDOW will return when the
user is interacting with the minibuffer in the mini-window?