atomontage / xterm-color

ANSI & xterm-256 color text property translator for Emacs
BSD 2-Clause "Simplified" License
214 stars 20 forks source link

Bright colours cause freezes in process buffers filtered by xterm-color #23

Closed lionel- closed 5 years ago

lionel- commented 5 years ago

And keeps hanging for all subsequent output. Setting xterm-color--attributes to 0 manually fixes the issue.

Temporary solution that only made sense when I thought kill-subjob was the cause: Resetting the variable by advising kill-subjob doesn't work, maybe because the xterm-color filter is still running at that time. It needs to be reset in a self-removing hooked function: ```lisp (defun my-xterm-color-fixup (&rest args) (setq xterm-color--attributes 0) (remove-hook 'comint-output-filter-functions #'my-xterm-color-fixup 'local)) (advice-add #'comint-interrupt-subjob :after (lambda (&rest args) (add-hook 'comint-output-filter-functions #'my-xterm-color-fixup 'append 'local))) ```
lionel- commented 5 years ago

I think the kill-sujob was a false lead. The interrupt caused the REPL to output text in bright black. Whenever such output is filtered, xterm-color--attributes is set to 1 forever.

cat("\033[90m bright black \033[39m\n")
lionel- commented 5 years ago

Does this patch make any sense?

--- a/xterm-color.el
+++ b/xterm-color.el
@@ -299,6 +299,9 @@ Once that happens, we generate a single text property for the entire string.")
     ;; Reset to default FG color
     ((= 39 elem)
      (remhash 'foreground-color xterm-color--current)
+     (setq xterm-color--attributes
+           (logand xterm-color--attributes
+                   (lognot +xterm-color--bright+)))
      (setq elems (cdr elems)))
     ;; Reset to default BG color
     ((= 49 elem)
atomontage commented 5 years ago

Not really since it does the wrong thing according to the spec.

Let's start from the beginning. I could not reproduce your hang on both Emacs 25 and Emacs 26. Can you post some Emacs Lisp code here that hangs?

Additionally, the state machine as it stands before your changes does the right thing. Resetting to default FG color should only do that, and not change the bright-or-bold state machine status bit.

You can verify that xterm-color does the right thing by opening a terminal emulator outside of Emacs (e.g. Terminal.app on OSX) and running:

printf "default \033[34;1mhigh-intensity \033[39mreset-to-default-color \033[34mblue (should be hi intensity)\033[0m\n"

You should see the word blue in bold or high intensity, rendered exactly like the "high-intensity" word.

You can do the same in Emacs with M-x shell or M-x eshell and get similar results. The high intensity bit in xterm-color--attributes should stay present even after the state machine encounters SGR 39. The only SGR attributes that should change that bit in xterm-color--attributes are:

0 (= total state machine reset) 1 (= set bright) 2 (= set faint, since Emacs does not do faint this is the same as normal in xterm-color) 22 (= set normal)

There is however something that xterm-color does not do correctly, and that's how it deals with AIXTERM hi-intensity color in a visual manner, but should not cause Emacs to hang.

    ;; AIXTERM hi-intensity FG color
    ((and (>= elem 90)
          (<= elem 97))
     (setf (gethash 'foreground-color xterm-color--current)
           (- elem 90))
     (setq xterm-color--attributes
           (logior xterm-color--attributes
                   +xterm-color--bright+))

So the issue here is that I was too lazy to properly check a pull-request I accepted. End result being that this block of code does the wrong thing since it sets the bright attribute (which will persist) when it encounters AIXTERM hi-intensity SGR rather than coupling brightness/hi-intensity with the FG color itself (which is how AIXTERM hi-intensity should be rendered). To see this, in Terminal.app or other terminal emulator outside Emacs:

printf "default \033[94mhigh-intensity \033[34mblue (should be normal intensity)\033[0m\n"

If you do that in Emacs, you'll see everything after default in bright.

I'll work on a fix for that case. Thanks for bringing this to my attention.

On Wed, 10 Oct 2018 16:51:24 -0700, Lionel Henry notifications@github.com wrote:

Does this patch make any sense?

--- a/xterm-color.el
+++ b/xterm-color.el
@@ -299,6 +299,9 @@ Once that happens, we generate a single text property for the entire string.")
     ;; Reset to default FG color
     ((= 39 elem)
      (remhash 'foreground-color xterm-color--current)
+     (setq xterm-color--attributes
+           (logand xterm-color--attributes
+                   (lognot +xterm-color--bright+)))
      (setq elems (cdr elems)))
     ;; Reset to default BG color
     ((= 49 elem)
lionel- commented 5 years ago

Ah that makes sense, thanks! About the hang, I can't come up with a reproducible example outside the ESS inferior. There may be some interaction with one of our filters, I'll investigate more later.

atomontage commented 5 years ago

Fixed in master.

lionel- commented 5 years ago

Did the bug you fixed involve an infloop alloc? That was what caused freezings for me. Didn't take the time to investigate further, sorry. Thanks anyway!

atomontage commented 5 years ago

I fixed the AIXTERM foreground color issue. Also added AIXTERM background color and bold-as-bright.

I've never seen freezes with xterm-color.el, and I don't see an obvious part of the code that could lead to an infinite alloc loop. Maybe malicious/erroneous input that never properly terminates escape sequences could lead the state machine to not produce any output and accumulate bytes in the state variables, but that would never freeze Emacs or lead into an infinite loop.

May I suggest you try latest master, but set xterm-color-debug to T, if you run into a freeze send me the 'xterm-color:' output in Messages, if any. That may tell us if it's an xterm-color issue or something else and also allow me to fix it if that's the case.

On Fri, 15 Mar 2019 07:50:53 +0000 (UTC), Lionel Henry notifications@github.com wrote:

Did the bug you fixed involve an infloop alloc? That was what caused freezings for me. Didn't take the time to investigate further, sorry. Thanks anyway!