bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky <npostavs@gmail.com>]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think I understand why the change fixes the bug, sorry.  Can
> you tell in more detail?

Okay.  First, see attached reduced test case which demonstrates the bug.


[-- Attachment #2: demo for bug --]
[-- Type: text/plain, Size: 1133 bytes --]

(with-current-buffer (get-buffer-create "*test buffer*")
  (let* ((oldproc (get-buffer-process (current-buffer))))
    (when oldproc
      (kill-process oldproc)
      (accept-process-output oldproc)))
  (erase-buffer)
  (display-buffer (current-buffer))
  (let* ((print-level nil)
         (print-length nil)
         (proc (start-process
                "test proc" (current-buffer)
                (concat invocation-directory invocation-name)
                "-Q" "--batch" "--eval"
                (prin1-to-string
                 '(let (s)
                    (while (setq s (read-from-minibuffer "$ "))
                      (princ s)
                      (princ "\n")))))))
    (send-string proc "one\n")
    (sit-for 0.5)                       ; Read "one".
    (set-process-filter proc t)         ; Stop reading from proc.
    (send-string proc "two\n")
    (sit-for 1)                         ; Can't read "two" yet.
    (set-process-filter
     proc                               ; Resume reading from proc.
     #'internal-default-process-filter)
    (sit-for 0.5)                       ; Read "two" from proc.
    ))

[-- Attachment #3: Type: text/plain, Size: 5144 bytes --]


In Emacs 25 it shows a buffer named "*test buffer*" with contents

    $ one
    $ two
    $ 

In Emacs 26, the "two" never gets read, so the result is

    $ one
    $ 

Calling (set-process-filter PROC t) is supposed to turn off reading for
PROC.  This works fine.  But (set-process-filter PROC FILTER) doesn't
turn on reading again in Emacs 26.  Neither of the cases in
set_process_filter_masks are taken in the case when FILTER is not t.

    DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
      ...
      (Lisp_Object process, Lisp_Object filter)
    {
      ...
      pset_filter (p, filter); // <-- sets p->filter = filter;

      if (p->infd >= 0)
        set_process_filter_masks (p);
      ...
    }

    static void
    set_process_filter_masks (struct Lisp_Process *p)
    {
      if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
        delete_read_fd (p->infd);
      else if (EQ (p->filter, Qt)
           /* Network or serial process not stopped:  */
           && !EQ (p->command, Qt))
        add_process_read_fd (p->infd);
    }

In Emacs 25 it looks like the 'if' cases are the same, but there is a
subtle difference: the first 'if' checks 'filter', while the second
checks 'p->filter'.  And furthermore note that pset_filter is called
after this check against 'p->filter', so it is checking the "original"
'p->filter' value (which means that Emacs 25 has a bug that the fd is
incorrectly enabled if setting the filter to t twice, i.e., (progn
(set-process-filter PROC t) (set-process-filter PROC t))).

    DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
      ...
      (register Lisp_Object process, Lisp_Object filter)
    {
      ...
      if (p->infd >= 0)
        {
          if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
            {
              FD_CLR (p->infd, &input_wait_mask);
              FD_CLR (p->infd, &non_keyboard_wait_mask);
            }
          else if (EQ (p->filter, Qt)
                   /* Network or serial process not stopped:  */
                   && !EQ (p->command, Qt))
            {
              FD_SET (p->infd, &input_wait_mask);
              FD_SET (p->infd, &non_keyboard_wait_mask);
            }
        }

      pset_filter (p, filter);

The patch found by Adam's bisect put the pset_filter call before this
check, so that Emacs 26 checks the 'p->filter' after it's been set
(i.e., the value of the 'filter' parameter).  So the second case is no
longer entered when calling (set-filter-process PROC FILTER).

> Also, the same function is called in another
> place, so what will this change do to that other caller?

Hmm, it's difficult to say, there are a lot of optional code paths.  I
suspect socket subprocesses might suffer from the same bug, but there
are also some (redundant?) calls add_process_read_fd that might cover it
up (notice that all calls are guarded by !EQ (p->filter, Qt), i.e., the
corrected version of the check in set_process_filter_masks).

    static void
    connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
                            Lisp_Object use_external_socket_p)
    {
      ...
      if (p->is_non_blocking_client)
        {...}
      else
        /* A server may have a client filter setting of Qt, but it must
           still listen for incoming connects unless it is stopped.  */
        if ((!EQ (p->filter, Qt) && !EQ (p->command, Qt))
            || (EQ (p->status, Qlisten) && NILP (p->command)))
          add_process_read_fd (inch);
      ...
    }
    ...
    static void
    server_accept_connection (Lisp_Object server, int channel)
    {
      ...
      /* Client processes for accepted connections are not stopped initially.  */
      if (!EQ (p->filter, Qt))
        add_process_read_fd (s);
      ...
    }
    ...
    int
    wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                                 bool do_display,
                                 Lisp_Object wait_for_cell,
                                 struct Lisp_Process *wait_proc, int just_wait_proc)
    {
      ...
                      if (0 <= p->infd && !EQ (p->filter, Qt)
                          && !EQ (p->command, Qt))
                        add_process_read_fd (p->infd);
      ...
    }
    ...
    DEFUN ("continue-process", Fcontinue_process, Scontinue_process, 0, 2, 0,
           ...)
      (Lisp_Object process, Lisp_Object current_group)
    {
      ...
          if (EQ (p->command, Qt)
              && p->infd >= 0
              && (!EQ (p->filter, Qt) || EQ (p->status, Qlisten)))
            {
              add_process_read_fd (p->infd);


> And how come we lived with this bug for 3 years without having noticed
> it?

I think there is very little use of t as a process filter value.  Maybe
term.el is the only user of it.  As to why nobody noticed the problem in
term.el earlier, I have the impression that most users just assume
term.el will be buggy and don't even bother reporting problems with it
(typical suggestions are "run screen or tmux to handle escape codes
properly", or "use the libvterm Emacs extension instead").