Repository navigation
Merged
Conversation
Reported from htop in the popup: arrows did nothing and clicking its
tabs did nothing. Two separate causes, both of which affect every pane
and not only the popup.
Arrow keys have two encodings. Normally an arrow is CSI-introduced
("\033[A"); once a program sets DECCKM, which is what ncurses does when
it calls keypad(), it is SS3-introduced ("\033OA"), and that is the form
terminfo tells the program to expect. Send the wrong one and the program
does not see a near miss, it sees no arrow key at all. The emulator has
tracked the mode all along and nothing ever read it back, so termdock
always sent the CSI form. Encoded per pane rather than once, because
with synchronized input two panes need not be in the same mode.
The mouse was never forwarded to anything. internal/vt10x records that a
program turned on reporting (modes 1000/1002/1003/1006) and there was no
Write anywhere in mouse.go: every click, drag and wheel event was
consumed by termdock's own gestures, so htop, vim and less never heard
about the mouse in any pane. They do now, with the state a terminal
mouse report needs and termdock did not have to keep before: tcell names
no button when it reports a release, and SGR reports have to name the
one being released, so the button that went down is remembered.
Shift takes it back, which is tmux's convention. It has to exist:
without it a pane running a full-screen program could not be focused,
selected from or resized with the mouse at all.
Two details worth pinning, and both are tested. tcell's button order and
X11's cross over (tcell's Button2 is the right button, X11's 2 is the
right one and 1 is the middle), which is the kind of mapping that gets
written backwards and never noticed. And the pre-SGR encoding puts a
coordinate in one byte biased by 32, so a click past column 223 cannot
be expressed: it is dropped rather than reported at the wrong cell.
Documented in the README and in the manual's keys page, since which of
the two owns a click is something you have to be told.
…ying Found by hunting rather than by report, which is the useful kind. An escape sequence could take down every session at once. "\033[9223372036854775807P" is a well-formed delete-characters, and the handler does "cursor column + n" and slices the line with the result: near the top of int64 that addition wraps negative, and the slice panics. The daemon holds every session in one process, so that is every pane of every session, from one line of output. Any program printing garbage could do it by accident. Clamped where the parameters are parsed rather than in the thirty-odd handlers that use one, since they all do arithmetic of that shape and fixing them one at a time would be waiting for the next report. The cap is xterm's, 65535, and negatives read as zero: ECMA-48 has no negative parameters but strconv.Atoi is happy to return one for "-1", which indexes backwards just as badly. Nothing legitimate notices, because every handler already clamps to the real grid. Separately, "termdock send-keys -t main:-1" typed into the *active* window without complaining. -1 is parseTarget's own "no window given" sentinel and a negative number parsed straight into it, so a spec that should have failed silently selected a different window instead. For a scripting interface whose main verb types keystrokes, acting on the wrong window is worse than refusing. Negative numbers are now read as a name, which fails to resolve and says so. Both come with the test that reproduces them: removing the clamp panics with the same negative slice bound, and removing the sign check puts the sentinel back. parseTarget and extractTarget had no tests at all, and now cover the specs worth being sure about. The hunt that turned these up is committed too: one test drives a whole session with random keys, clicks and resizes through every mode, and one throws well-formed but extreme escape sequences at the emulator. The second is what found the panic.
The jump picker's preview showed a black slab beside any pane that was
not full of text, and the braille thumbnail had black behind its dots.
Reported a while back, half-fixed then: the earlier pass changed what
draws the box, and this is what was actually filling it.
A preview cell carries the raw colour values vt10x uses, where "default"
is 1<<24 and up and anything under 256 is a palette index. Zero is
therefore not "unset", it is palette entry 0, which is black. Every
blank cell was built as proto.Cell{Ch: ' '}, so it went out as black on
black and was drawn as black whatever theme was set, right next to panes
the same theme was colouring correctly.
Three places built cells that way, including the one that draws the
braille dots, so the dots had a black backing too. All three now go
through one blankCell, which spells the default colours out, so there is
no longer a way to write the wrong thing by leaving a field off.
The test asserts on the colour values rather than on pixels, since that
is where the mistake lives: a zero colour is the bug, and "not zero" is
not enough to say, so it checks for the default range specifically.
…value Pressing S on the settings screen wrote termdock.conf and said so, into a status bar that was already showing "↑↓ move, enter to type a value, S save to file, esc close". Every modal screen keeps its own instructions there for as long as it is open, and those won, so from the one screen where knowing the file was written is the whole point, nothing appeared to happen. A flash is a message that outranks a screen's own hint for a few seconds and then gets out of the way, which is the right trade for both: the instructions are what you need for as long as the screen is open, the confirmation is what you need immediately after acting. The same fault made two worse messages invisible, both found looking for others like it. Typing a value the settings screen refuses left the screen open with the reason hidden, so a rejected value looked accepted. And pressing an arrow on a free-text setting explained that it has no list to step through, silently. flash schedules a redraw for when it expires. A frame is only built when something asks for one, and the periodic tick is every fifteen seconds, so without that the message would outstay its welcome by ten seconds or more and "a few seconds" would be a duration nothing honoured.
Every pane has a pump goroutine blocked reading its pty, and a session is meant to stay up for weeks, so one left behind per closed pane is a slow leak in a process that never restarts. Opening and closing twenty-four panes leaves the goroutine count exactly where it started, which is what this pins. Nothing was wrong: Close kills the process and closes the pty, which is what unblocks the read, and both paths that remove a pane call it first. Worth a test anyway, because the property is invisible until it has been failing for a fortnight and the failure is a daemon that has quietly grown to thousands of goroutines. The comment records the trap the first version of this test fell into. Closing panes with detachLeafIn leaks a goroutine every time and looks precisely like the bug being hunted, because detachLeafIn only takes a pane out of the layout tree: break-pane and move-to-window use it on panes they mean to keep running.
The counterpart to the pane lifecycle guard. Attach and detach is what a session does most over its life, and a goroutine left behind by each one would only show up in a session old enough that restarting it costs real work. Fifteen cycles, same goroutine count at the end as at the start. Nothing was wrong here either. Both of these are cheap to keep and would have caught the leak on the day it was introduced rather than a fortnight later.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven commits: two things that were reported, four found by hunting for
others like them, and the tests that did the hunting.
A malformed escape sequence could take the daemon down
\033[9223372036854775807Pis a well-formed delete-characters. Thehandler does "cursor column + n" and slices the line with the result,
which near the top of int64 wraps negative and panics. Every session
lives in the one daemon process, so that is every pane of every session.
Clamped where the parameters are parsed rather than in the thirty-odd
handlers that use one, since they all do arithmetic of that shape. The
cap is xterm's, 65535; negatives read as zero, because ECMA-48 has no
negative parameters but
strconv.Atoiis happy to return one for "-1",which indexes backwards just as badly.
On reachability, measured rather than assumed: 200 MB of random bytes
contained no sequence of this shape, so it is not something a
catof abinary hits by accident. It needs a program emitting a large parameter,
so this is robustness rather than a daily hazard. Worth releasing all
the same: a terminal emulator should not crash on any byte stream, and
this one is holding everybody's sessions.
Programs that ask for the mouse and the arrow keys now get them
Reported from htop in the popup, where clicking its tabs and pressing
arrows both did nothing. Two separate causes, and both applied to every
pane, not only the popup.
Arrow keys have two encodings: CSI (
\033[A) normally, SS3 (\033OA)once a program sets DECCKM, which is what ncurses does when it calls
keypad(). The emulator had tracked the mode all along and nothing readit back, so the CSI form always went out and the program saw no arrow
key at all. Encoded per pane, since with synchronized input two panes
need not be in the same mode.
The mouse was never forwarded to anything.
internal/vt10xrecords thata program turned reporting on (modes 1000/1002/1003/1006) and there was
no
Writeanywhere inmouse.go, so every click, drag and wheel eventwas consumed by termdock's own gestures. Shift takes it back, which is
tmux's convention and has to exist: without it a pane running a
full-screen program could not be focused or resized with the mouse.
This is the largest behavioural change here and the one worth
reviewing closest. Two details are easy to get backwards and both are
tested: tcell's button order and X11's cross over, and the pre-SGR
encoding puts a coordinate in one byte biased by 32, so a click past
column 223 cannot be expressed and is dropped rather than reported at
the wrong cell.
Four smaller ones
cell carries raw vt10x colour values, where "default" is 1<<24 and up
and anything under 256 is a palette index, so zero is not "unset", it
is black. Three places built empty cells as
proto.Cell{Ch: ' '},including the one drawing the braille dots. All three go through one
constructor now.
termdock.conf said so into a status bar already showing the screen's
own instructions, which won. The same fault hid two worse messages: a
refused value left the screen open with the reason invisible, so a bad
value looked accepted.
send-keys -t main:-1typed into the active window. -1 isparseTarget's own "no window given" sentinel and a negative numberparsed straight into it. For a scripting interface whose main verb
types keystrokes, hitting the wrong window is worse than refusing.
eval "$(termdock shell-init)"printed its explanatory note intoevery new shell. The note was on stderr, which command substitution
leaves pointed at the terminal. It is now conditional on stdout being
a terminal, so a bare run still explains itself.
Tests
About 1,180 of the 1,764 added lines are tests. Every fix comes with one
that reproduces the fault: removing the clamp panics with the same
negative slice bound, removing the sign check puts the sentinel back,
removing the flash leaves the screen's own hint in place.
Two are the hunt itself. One drives a whole session with random keys,
clicks and resizes through every mode; the other throws well-formed but
extreme escape sequences at the emulator, and is what found the crash.
Two more pin that panes and client attachments leak no goroutines,
neither of which was broken, but both are invisible until they have been
failing for a fortnight in a daemon meant to run for weeks.
parseTargetandextractTargethad no tests at all and now do.