GNOME Bugzilla – Bug 789954
Soft reset shouldn't wipe out selection
Last modified: 2018-03-05 20:33:02 UTC
From https://bugzilla.xfce.org/show_bug.cgi?id=13976: Soft reset ("\e[!p") shouldn't wipe out the selection/clipboard. (Maybe hard reset shouldn't either, not sure about that.)
Wiping the selection is pretty explicit in vte.cc VteTerminalPrivate::reset(), ~20 lines beginning at the comment "/* Reset selection. */". When the scrollback is cleared, the ring is reset (does counting the rows restart then from 0? I can't remember). Leaving the selection intact then might result in weird behavior, even crashes I'm afraid if someone tries to extend that with Shift+click. So we should be extra careful if we want to preserve the selection (like xterm does). I don't feel like digging into the code that much. When the scrollback isn't cleared, it looks pretty clean that we can just leave the selection intact. Not sure if we can piggyback on the "clear_history" parameter, or introduce a separate "clear_selection" one. Or if it would be nicer to pass a "soft/hard" parameter instead, as we have it in vteseq, and let reset() decide what they mean.
> does counting the rows restart then from 0? I can't remember Nope, it doesn't restart (IIRC, for the encryption layer not to re-use the same IV, and to save us from the expensive operation of generating new keys). At this moment it seems safe to me just to unconditionally wipe out this code. Selection/clipboard will then be retained even across hard resets. Do we want this though? Or do we want this to be wiped out on reset... why? For privacy reasons perhaps (what's the thinking behind it, then)? Note that currently we do keep the selection on a "clear scrollback" ("\e[3J") as well other forms of the selected text just getting overwritten or scrolling out for good. So the question is specifically for the "hard reset" operation ("\e[c" or API). Just checking Firefox: navigating to a different site, or closing the tab keeps the clipboard. If we're about to mimic this, we shouldn't wipe out the clipboard on hard resets either.
*** Bug 791582 has been marked as a duplicate of this bug. ***
(In reply to Egmont Koblinger from comment #2) > At this moment it seems safe to me just to unconditionally wipe out this > code. Selection/clipboard will then be retained even across hard resets. > > Do we want this though? Or do we want this to be wiped out on reset... why? > For privacy reasons perhaps (what's the thinking behind it, then)? > > Note that currently we do keep the selection on a "clear scrollback" > ("\e[3J") as well other forms of the selected text just getting overwritten > or scrolling out for good. So the question is specifically for the "hard > reset" operation ("\e[c" or API). > > Just checking Firefox: navigating to a different site, or closing the tab > keeps the clipboard. If we're about to mimic this, we shouldn't wipe out the > clipboard on hard resets either. Yes, I think it's ok to preserve the selection on reset; the operation resets the terminal state, not the view state. (OTOH this forces us to either pregenerate the selection text (as we do currently) or keep the dropped rows in the scrollback (if we were to move to generating the selection text on request) [which is the model I want to move to])...)
I'd like to preserve the selection on soft reset for 0.52. My brain tells me it should really be okay to preserve it on hard reset as well. Whatever code we have currently has to be foolproof against things scrolling out and such, so it _should be_ safe. On the other hand, as we know, "should be" != "is", and I have bad guts feeling about it, especially 2 days before the code freeze without having tested it thoroughly. I think I'd defer it to 0.53. Maybe I'm just paranoid here. If you are confident it's okay, we can go forward now here as well. (Worst case we'll revert, and even make an out-of-band release.)
I don't see what I could break; the selection data is stored and doesn't need to go to ring or anything to generate when it's requested. So it should be ok.
Created attachment 369231 [details] [review] Fix Okay. It should be this simple then.
Comment on attachment 369231 [details] [review] Fix - m_selecting = FALSE; - m_selecting_restart = FALSE; - m_selecting_had_delta = FALSE; Not sure about these 3, they are used while selecting... maybe should keep them? The rest seems ok to me.
I was just about to ask you whether it's safe if selecting is in progress... are you confident that crashes can't happen then either? However, I guess it should be the other way around than you suggest: Just as we dropped resetting mouse_last_position to (-1,-1) on reset, I guess a reset shouldn't tamper with properties of the current selection either. I mean, even if decided that the selection needs to be wiped out on a reset, we should still remove these three lines, shouldn't we??
For a reset-without-clear, yes. For with-clear, that changes the display and thus the selection should be aborted.
Created attachment 369232 [details] [review] Fix, v2 Like this? If selecting is aborted, don't we need to deselect_all() as well? And/or keep those memset's below? I guess we'd need to take a closer look at the selection code to tell. I guess now I remember why I had worries with this change :) This is a pretty obscure use case anyway. Escape-sequence resets _shouldn't_ occur while selecting since input processing _should_ be stopped then. It is not always: bug 769695. API resets shouldn't occur while selecting in any sane vte-based app: you can't select and invoke the menu entry at the same time, however it could still happen if an app is stress-testing vte. Ouch...
Maybe move the entire code that v1 removes into an if (clear_history && m_selecting) ?? Really not sure what we should do if selecting is aborted. Perhaps primary should be gone, to match the visual highlight, whereas clipboard should remain. It's getting too complicated.
Do you think bug 769695 can cause a crash if a full reset (incl. scrollback) is received via escape sequences while selecting is in progress? If so, we should really fix that bug. If not, I believe we should be safe with fix v1 over here: just let selecting continue on the new content.
(In reply to Egmont Koblinger from comment #11) > Created attachment 369232 [details] [review] [review] > Fix, v2 > > Like this? > > If selecting is aborted, don't we need to deselect_all() as well? And/or > keep those memset's below? I guess we'd need to take a closer look at the > selection code to tell. Interestingly, those aren't memset to 0 anywhere else, only here on reset. Weird indeed. How about keeping those in the if(clear_history) branch with a comment to investigate? > I guess now I remember why I had worries with this change :) Yes :-) > This is a pretty obscure use case anyway. Escape-sequence resets _shouldn't_ > occur while selecting since input processing _should_ be stopped then. It is > not always: bug 769695. API resets shouldn't occur while selecting in any > sane vte-based app: you can't select and invoke the menu entry at the same > time, however it could still happen if an app is stress-testing vte. (In reply to Egmont Koblinger from comment #13) > Do you think bug 769695 can cause a crash if a full reset (incl. scrollback) > is received via escape sequences while selecting is in progress? > > If so, we should really fix that bug. > > If not, I believe we should be safe with fix v1 over here: just let > selecting continue on the new content. I don't think it could crash; extend_selection[_expand]() looks safe.
(In reply to Christian Persch from comment #14) > How about keeping those in the if(clear_history) branch with a comment to > investigate? I'm getting totally lost... :) Keeping the entire debated 20-ish lines inside an if(clear_history) is the only absolutely safe bet I can see here now – which keeps the selection on soft reset only, not on hard one, as per the bug title and comments 0 & 5. Can we loosen this up a bit? How?
What wipes out the selection is freeing and nulling out m_selection[*]. If you just delete that bit and keep all the reset, that should accomplish the desired result, no?
Created attachment 369340 [details] [review] Fix, v3 > If you just delete that bit and keep all the reset, that should accomplish the desired result Plus I guess we shouldn't do anything, absolutely anything, not even terminate selecting (which is only possible due to the aforementioned bug) on a soft reset. A soft reset should be damn harmless, it shouldn't do anything to the visible content (well, except for resetting the palette). I believe it's equivalent to resetting most of the things you could reset via escape sequences, such as \e[0m \e[?1000l \e[?2004l \e(B etc..., something that should be safe to place at the beginning of PS1 to mostly clean up if an app crashes and leaves the terminal messed up. As such, a newly discovered thing: soft reset shouldn't scroll the viewport either. So how about this patch?
Comment on attachment 369340 [details] [review] Fix, v3 Yes :-)
Submitted.
*** Bug 571341 has been marked as a duplicate of this bug. ***