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

Fixes deadlock between Mouse cuts and xfideventread #102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hherman1
Copy link

@hherman1 hherman1 commented Dec 3, 2022

When cutting with the mouse it was possible for:

  1. Text select to release the biglock, while maintaining a winlock
  2. xfideventread to then acquire the biglock, and wait for the winlock

Deadlocking. After fixing this deadlock, the following appeared:

  1. xfideventwrite releases the big lock, but holds the winlock
  2. Text select reacqiures the big lock and blocks on the winlock

Another deadlock.

This was reproducible by me in https://github.com/hherman1/acme-go by running Watch echo 1 2 3 and attempting to cut anything from the Watch window.

When cutting with the mouse it was possible for:

1. Text select to release the biglock, while maintaining a winlock
2. xfideventread to then acquire the biglock, and wait for the winlock

Deadlocking. After fixing this deadlock, the following appeared:

1. xfideventwrite releases the big lock, but holds the winlock
2. Text select reacqiures the big lock and blocks on the winlock

Another deadlock.
@pedramos
Copy link

This seems to cause a segfault when pasting to acme's top panel because in that situation t.W is nil (in cmd/acme/internal/ui/text.go:550).

This should be able to fix that:

		if t.W != nil {
			wind.Winunlock(t.W)
		}

		BigUnlock()
 		for Mouse.Buttons == b {
 			*Mouse = <-Mousectl.C
 		}

		BigLock()
		if t.W != nil {
			wind.Winlock(t.W, t.W.Owner)
		}

I also had to increase the size of rch channel on draw/mouse.go:43 from 2 to 10. There is no special reason behind 10 aside from it was the first value I tried and it worked right away. Without this, for large acme windows (this happened with a 4k screen with a maximized acme window), Cut & Paste, with the mouse, on a Watch window, still freezes acme for me. Maybe this should be a different commit though.

@hherman1
Copy link
Author

The if checking seems like a good change to this diff. I guess I literally never paste in the top bar which is why I didn’t encounter that issue. Are you able to edit this pr directly? I don’t know how often @rsc checks this repo, so I’m also willing to accept this change on my fork in the mean time if that is helpful

@pedramos
Copy link

pedramos commented Dec 20, 2022

Meanwhile I noticed my change breaks copy paste on win because wind.Winlock sets the t.W.Owner to 0.
So, this seems to fix it:

		var owner rune
		if t.W != nil  {
			owner = t.W.Owner
			wind.Winunlock(t.W)
		}

		BigUnlock()
		for Mouse.Buttons == b {
			*Mouse = <-Mousectl.C
		}

		BigLock()
		if t.W != nil {
			wind.Winlock(t.W, owner)
		}

I will keep testing it to make sure nothing else breaks.

Are you able to edit this pr directly?

I don't think I can :/

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