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 741406 - Crash at 'screen -r'
Crash at 'screen -r'
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.39.x
Other Linux
: Normal critical
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-12-11 17:41 UTC by Egmont Koblinger
Modified: 2014-12-11 19:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Tiny test case (53 bytes, text/plain)
2014-12-11 17:41 UTC, Egmont Koblinger
  Details
Fix (2.50 KB, patch)
2014-12-11 18:34 UTC, Egmont Koblinger
committed Details | Review

Description Egmont Koblinger 2014-12-11 17:41:08 UTC
Created attachment 292554 [details]
Tiny test case

On a certain remote setup (ssh, zsh, oh-my-zsh etc.) the command 'screen -r' sometimes makes vte crash:

Vte:ERROR:vte.c:3928:vte_terminal_process_incoming: assertion failed: (terminal->pvt->cursor.row >= terminal->pvt->screen->insert_delta)

Broken since 5a434e6 (bug 731205), in master branch only.
Comment 1 Egmont Koblinger 2014-12-11 18:34:48 UTC
Created attachment 292561 [details] [review]
Fix

A soft reset (echo -ne "\e[!p") brings us back to the normal screen, however, cursor.row (which is supposed to contain insert_delta which is screen-specific) is not updated.

According to xterm's behavior, soft reset should not switch back to normal screen.

(The linked change broke it because it introduced one single cursor.row rather than the two screen-specific ones, and I forgot to adjust its value the same way as I did in vte_sequence_handler_*_screen()).
Comment 2 Egmont Koblinger 2014-12-11 18:39:46 UTC
(Forget the first half of the patch, it's pebkac.)

I hijacked the "clear_history" parameter to switch back to alternate screen. It's a public function, so we either go for an API change (I really don't want, especially not for this fix), or hijack (I think it's acceptable, probably noone would want to clear the history of the normal screen while staying on the alternate), or I could have gone for the soft reset having its own method (not exposed via API).

We might want switch to this latter one at some point, I have a guts feeling that \e[!p resets much more than it should.
Comment 3 Egmont Koblinger 2014-12-11 19:26:25 UTC
Comment on attachment 292561 [details] [review]
Fix

committed (without the vteapp garbage)
Comment 4 Egmont Koblinger 2014-12-11 19:28:33 UTC
According to http://www.vt100.net/docs/vt510-rm/DECSTR, soft reset should actually reset quite a few things (but normal/alternate screen not being one of them).  So let's hope our implementation is okay now.  If not, we can reopen this or file a new bug.