Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't exit-program on X11 selection-event protocol errors #6135

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

loops
Copy link
Contributor

@loops loops commented Sep 14, 2024

There are situations where transient xcb errors can be generated in regard to copy/paste selection. One example was reported in connection with "wl-clip-persist" in issue #6128. And another in issue #5482.

But there's no reason for us to commit harakiri in response, so just quietly ignore such errors.

@wez
Copy link
Owner

wez commented Sep 14, 2024

Thanks for this, but I don't see evidence that wezterm is actually panicking or crashing here or in the linked issues, as the call stack behind the error propagation here doesn't panic, it just logs the error and shares it as a notification.

Is the goal really just to silence errors instead, or is there actually a panic?

@loops
Copy link
Contributor Author

loops commented Sep 14, 2024

Hi wez,

Hope you're doing well!

You're right it doesn't actually segfault. But the code reads like it will escape the "forever" loop, as the error is propagated up the call chain via the "anyhow" type. I was unable to reproduce the problem locally, so based my description on the results reported in the issues.

I am happy to make a more appropriate commit message, that changes "panic" to "exit program".

@loops loops changed the title Don't panic on X11 selection-event protocol errors Don't exit-program on X11 selection-event protocol errors Sep 14, 2024
@wez
Copy link
Owner

wez commented Sep 14, 2024

Ah, I see. OK, the way I look at this sort of error is, what is the highest level at which we can/should catch it?
I think right now this PR is a bit naughty in that it is suppressing errors and continuing to respond to the incoming request when it didn't really complete its work, which propagates undefined state across an IPC boundary, and that feels a bit bad.

What I propose instead is to unwind those changes and instead catch and log over here:

Event::X(xcb::x::Event::SelectionRequest(e)) => {
self.selection_request(e)?;
}

 Event::X(xcb::x::Event::SelectionRequest(e)) => { 
     if let Err(err) = self.selection_request(e) {
        // Don't propagate this, as it is not worth exiting the program over it.
        // <https://github.com/wez/wezterm/pull/6135>
        log::error!("error handling SelectionRequest: {err:#}");
     } 
 } 

It seems like similar treatment should be applied to the other selection related events in that file?

What do you think?

  There are situations where transient xcb errors can be generated
  in regard to copy/paste selection.  One example was reported
  in connection with "wl-clip-persist" in issue wez#6128.  And another
  in issue wez#5482.

  But there's no reason for us to terminate in response, so catch
  and report any selection-related errors, as per code review in
  PR  wez#6135
@loops
Copy link
Contributor Author

loops commented Sep 14, 2024

Makes a lot more sense. Pushed.

@wez wez merged commit 9668fa9 into wez:main Sep 14, 2024
14 of 15 checks passed
wez added a commit that referenced this pull request Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants