go-vgo / robotgo

RobotGo, Go Native cross-platform RPA and GUI automation @vcaesar
Apache License 2.0
9.6k stars 879 forks source link

Fix x11 display leak #590

Closed jraby closed 1 year ago

jraby commented 1 year ago

On X11, calling GetTitle a couple times would crash the program with Maximum number of clients reached + segfault.

This plugs the leaks that I was hitting but there might be more:

Maximum number of clients reached
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0xf8 pc=0x7ffa68d71d6d]

runtime stack:
runtime.throw({0x663236?, 0x298f500?})
        /usr/lib/go/src/runtime/panic.go:1047 +0x5d fp=0x7fff0f782230 sp=0x7fff0f782200 pc=0x43bcdd
runtime.sigpanic()
        /usr/lib/go/src/runtime/signal_unix.go:821 +0x3e9 fp=0x7fff0f782290 sp=0x7fff0f782230 pc=0x4528e9

goroutine 9 [syscall]:
runtime.cgocall(0x5ed7f0, 0xc000054708)
        /usr/lib/go/src/runtime/cgocall.go:157 +0x5c fp=0xc0000546e0 sp=0xc0000546a8 pc=0x40937c
github.com/go-vgo/robotgo._Cfunc_get_main_title()
        _cgo_gotypes.go:616 +0x49 fp=0xc000054708 sp=0xc0000546e0 pc=0x5ea709
github.com/go-vgo/robotgo.GetTitle({0x0?, 0x0?, 0xc000054760?})
        /home/jean/ws/jr/exp/robotrainer/robotgo/robotgo.go:998 +0x7a fp=0xc000054740 sp=0xc000054708 pc=0x5eab3a
github.com/go-vgo/robotgo_test.TestGetTitle(0xc0000d04e0?)
        /home/jean/ws/jr/exp/robotrainer/robotgo/robot_info_test.go:51 +0x29 fp=0xc000054770 sp=0xc000054740 pc=0x5eb1a9
testing.tRunner(0xc0000d0b60, 0x6819e0)
        /usr/lib/go/src/testing/testing.go:1576 +0x10b fp=0xc0000547c0 sp=0xc000054770 pc=0x4d570b
testing.(*T).Run.func1()
        /usr/lib/go/src/testing/testing.go:1629 +0x2a fp=0xc0000547e0 sp=0xc0000547c0 pc=0x4d674a
runtime.goexit()
        /usr/lib/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc0000547e8 sp=0xc0000547e0 pc=0x46fb21
created by testing.(*T).Run
        /usr/lib/go/src/testing/testing.go:1629 +0x3ea

I'm not familiar with X11 code, but is it customary to call XOpenDisplay + XCloseDisplay multiple times? I get the feeling that it would be more efficient and less error prone to keep the display open through the lifetime of the application instead of the open/close dance that is currently done. Or at least it seems strange that open/close is called multiple times when calling GetTitle.

The current shape of the code might not lend itself to this pattern very well since the same functions are called for all OSes, carrying a fd throughout might not be straightforward.

This might fix #168

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.