After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738964 - Stream's tail advances its head
Stream's tail advances its head
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.38.x
Other Linux
: Normal normal
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-21 21:27 UTC by Egmont Koblinger
Modified: 2014-11-20 20:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
trigger assertion failure (438 bytes, patch)
2014-10-21 21:28 UTC, Egmont Koblinger
committed Details | Review
Fix (783 bytes, patch)
2014-11-16 14:48 UTC, Egmont Koblinger
committed Details | Review

Description Egmont Koblinger 2014-10-21 21:27:14 UTC
Apply the attached patch, start vte, start vim or mc (or switch to alternate screen by some other means) => assertion fails.

In the alternate screen's stream (why does it have any? I guess it shouldn't) the tail advances beyond the head.

Not sure if it imposes runtime bugs or not, but nevertheless it should be fixed.
Comment 1 Egmont Koblinger 2014-10-21 21:28:21 UTC
Created attachment 289080 [details] [review]
trigger assertion failure

(patch that imposes the bug; not a patch that fixes it :))
Comment 2 Behdad Esfahbod 2014-10-21 21:55:47 UTC
Definitely shouldn't happen.  And we should assert that.  Though, I don't think anything bad is happening right now.  Haven't checked the code though.
Comment 3 Egmont Koblinger 2014-11-16 13:47:35 UTC
The bug is not specific to the alternate screen. You can also trigger on the normal screen if your scrollback is very small. E.g. apply the patch, start ./src/vte-2.91 -n 30, press and hold Enter at the shell prompt.

The problem is: _vte_ring_discard_one_row falsely assumes that the row to be discarded has already made it into the stream, it doesn't handle the case if it's still in ring's circular array (i.e. hasn't been frozen).

On a slightly related note:

To make vte easier to debug and less like to contain a nasty bug that relates to vte's height vs. powers of 2, I'd like to modify the circular buffer so that a row is frozen as soon as it scrolls out of the screen, rather than when it no longer fits in the ring.

The ring's size could remain a power of 2, but it could also be changed to match the window height. I don't think doing an arbitrary "modulo" operand is noticeably slower than bitmask operations. Changing the ring size is a rare user-initiated event (window resize), so there's no need to optimize there for fewer mallocs either.
Comment 4 Egmont Koblinger 2014-11-16 14:48:57 UTC
Created attachment 290790 [details] [review]
Fix
Comment 5 Behdad Esfahbod 2014-11-17 17:47:29 UTC
Sounds good to me.  Thanks.
Comment 6 Christian Persch 2014-11-19 18:57:19 UTC
Is this safe to put into 0-38 ? If so, please cherry-pick :-)
Comment 7 Egmont Koblinger 2014-11-19 23:19:26 UTC
I'm planning to fix bug 740347 too, maybe more (the one-fd patch still crashes sometimes on some assertions), and will cherry-pick them all at once if they seem to be safe&stable.

These are not critical at all, I'd rather go for absolutely sure than risking breaking anything in the stable branch.

Is there a 0.38.3 planned?
Comment 8 Christian Persch 2014-11-20 20:44:20 UTC
Not exactly planned, but if enough good bug fixes pile up it may happen :-)