GNOME Bugzilla – Bug 708496
Lines disappear after positioning back the cursor
Last modified: 2013-10-19 21:08:56 UTC
Created attachment 255444 [details] lsoutput.txt to reproduce the bug After repositioning the cursor to the top(ish) of the window, then resizing the window to be larger, then again repositioning the cursor, some rows get dropped from the internal data. The cells are not invalidated immediately, the data visually stays on until those lines are repainted. Steps to reproduce: Start vte-0.34.8 Execute cat lsoutput.txt (the attached file, a colored "ls -l" executed from vte source's root directory) Execute echo $'\e[2;0H' to move the cursor to the 3rd row Make the window taller, to around 40 lines Execute the same echo command again On mouse highlight or focus out, some lines disappear. Video: https://docs.google.com/file/d/0B9iCZ18clVMiUEo1dFJaSFByc0U/edit?usp=sharing Notice: I couldn't reproduce with black and white "ls -l --color=never" output.
An even simpler way to reproduce that doesn't require run-time resizing: ./src/vte2_90 -g 80x35 cat lsoutput.txt echo $'\e[2;0H' and the screen is garbled. Change the default ring->mask from 31 to 63 in the source and the above example doesn't reproduce the bug anymore. Probably it'd need a correspondingly larger screen to reproduce.
Those lines that disappear are actually still present at the very end of the (now truncated) file stream, and they come back to life with proper text but faulty colors (e.g. fully green lines) if you print some more stuff in the terminal and scroll back. Is attr_stream truncated incorrectly? Or maybe the last entry needs to be updated somehow?
In vte-0.34.8 ring.c _vte_ring_thaw_row() line 218-219: if (!_vte_stream_read (ring->attr_stream, record.attr_offset, (char *) &attr_change, sizeof (attr_change))) return; we do hit that return when it behaves buggy. More than suspicious.
Continue the test case of comment 1 by one more line: ./src/vte2_90 -g 80x35 cat lsoutput.txt echo $'\e[2;0H' # some lines disappear here seq 1 100 Scroll back, notice that 38 and 39 are missing. Press Enter, now 39 and 40 are missing, etc. We're clearly seeing a ring corruption, apparently vte doesn't recover from it until a terminal reset.
When it starts being buggy, in _vte_ring_thaw_row() in the do_truncate branch the "records[0].text_offset < ring->last_attr.text_offset" condition is true, but the next one is false. So ring->last_attr.whatever are not updated, and this seems to be strongly related. Within the do_truncate branch, swapping the order of this "if" stuff, and actually truncating the three streams looks good at the very first glimpse. It's 2am and I can hardly keep my eyes open and I have no idea what I'm doing :D Btw the pattern that we store attributes along with the text offset where those attributes _end_ is weird to me. I'm wondering if the code would be much simpler if we stored attributes along with offsets where they _start_. E.g. we probably wouldn't need that extra record called last_attr that we're not writing to the stream yet...
Created attachment 255453 [details] [review] I have no idea what I'm doing but it seems to work :)
Ugly things happen when truncating somewhere where the attributes change. In _vte_ring_freeze_row() where writing the attr stream, you first update last_attr.text_offset, then write out last_attr, then update last_attr.attr. This leads to the bizarre situation that while the in-memory last_attr has a color attribute and its starting offset, the filestream will contain a color attribute along with its end offset (as I've pointed out earlier). So in _vte_ring_thaw_row()'s do_truncate branch, reading back a record straight to last_attr is definitely wrong. Or (rather) reading back is right, but writing out (having different semantics for last_attr and attr_stream's records) is wrong. Apart from this, it seems that after truncating at an attribute change, attr_stream will contain one more record than desired: the one pointing to the new head of text_stream is not stripped off although it should be.
Review of attachment 255453 [details] [review]: forget it
Now that I think of it, storing (attr, end_offset) pairs in the text stream does make sense: this way you still know the starting color of a page after dropping the previous page. Storing (attr, start_offset) would require nasty hacks. So my current plan is to keep this behavior (so luckily I don't need to touch the nontrivial but well-tested freeze/thaw stuff), and only adjust _vte_ring_thaw_row()'s truncate part accordingly. I'm also planning to make the code more self-documenting: introduce two standalone variables instead of last_attr's two fields to emphasize the semantical difference (e.g. last_attr and last_attr_text_start_offset -- let me see if the latter one is actually required), and rename VteCellAttrChange's text_offset field to text_end_offset.
Created attachment 255563 [details] [review] Step 1: Refactoring only, no behavior change
Created attachment 255564 [details] [review] Step 2: The actual bugfix Please test and review. Divided into two separate patches, to be applied in top of each other. I hope it's easier to review this way. The first one only renames variables and similar refactoring, no change in behavior. The second one is the actual bugfix.
Created attachment 255568 [details] [review] Step 2: The actual bugfix (just clarified one comment)