Do not crash on wayland, gracefully exit with error instead
ClosedPublic

Authored by alexeymin on May 17 2019, 11:14 PM.

Details

Summary

Sadly, XTestQueryExtension just segfaults under wayland.
Avoid it by detecting QPA used.

BUG: 406599

Test Plan

Nice error message when running in Wayland session.

Still works in X11

Diff Detail

Repository
R437 Desktop Sharing
Branch
fix_406599
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11916
Build 11934: arc lint + arc unit
alexeymin requested review of this revision.May 17 2019, 11:14 PM
alexeymin created this revision.
alexeymin edited the test plan for this revision. (Show Details)May 17 2019, 11:40 PM
pino added a subscriber: pino.May 18 2019, 7:00 AM
pino added inline comments.
krfb/main.cpp
45–46

then most probably only the platform name check should be enough, shouldn't it?

50–51

please no exclamation marks in error messages, as they "increase the tone" of the message itself.
What about a message like: "Desktop Sharing is not running under an X11 Server. Other display servers are currently not supported."

alexeymin updated this revision to Diff 58246.May 18 2019, 9:18 AM
  • Check only for platformName and rephrase the error message.
alexeymin added inline comments.May 18 2019, 9:19 AM
krfb/main.cpp
45–46

Yeah, probably platform name check should be enough.

50–51

Okay

alexeymin marked 4 inline comments as done.May 18 2019, 9:19 AM
alexeymin added a comment.EditedMay 18 2019, 9:32 AM

@pino thanks for looking at this! Fixed your comments.

The more I use wayland, the more I think it was designed not to help users, but to cause more pain.

You cannot (easily) grab screen on wayland, and you cannot emulate global user input, so vnc server on wayland is hardly possible. I know, there is D20402, but it is still not done, and even with that pile of layers just to get screen contents, input emulation problem is still not solved ( https://phabricator.kde.org/D18115#389996 ).

alexeymin edited the test plan for this revision. (Show Details)May 18 2019, 9:34 AM
alexeymin added reviewers: pino, ngraham.
pino added inline comments.May 18 2019, 10:12 AM
krfb/main.cpp
45

looking at similar checks in other applications, shouldn't this be QX11Info::isPlatformX11()?

pino added a comment.May 18 2019, 10:15 AM

Another idea is to borrow a small bit from D20402, in particular the change in main() that wraps checkX11Capabilities() within a X11 check: as "else" case for that if, you add the error message added here.
This way, after this is done, D20402 will need to add a no-op check for wayland.

alexeymin updated this revision to Diff 58247.May 18 2019, 10:50 AM
  • Wrap checkX11Capabilities() in QX11Info::isPlatformX11() check
In D21267#466632, @pino wrote:

Another idea is to borrow a small bit from D20402, in particular the change in main() that wraps checkX11Capabilities() within a X11 check: as "else" case for that if, you add the error message added here.
This way, after this is done, D20402 will need to add a no-op check for wayland.

Yeah, you are right, QX11Info::isPlatformX11() does exactly that - https://code.qt.io/cgit/qt/qtx11extras.git/tree/src/x11extras/qx11info_x11.cpp#n98 .
Done :)

pino accepted this revision.May 18 2019, 11:04 AM

LGTM now, thanks.
Works in X11; I assume you tested it under wayland, right?

This revision is now accepted and ready to land.May 18 2019, 11:04 AM
In D21267#466653, @pino wrote:

LGTM now, thanks.
Works in X11; I assume you tested it under wayland, right?

Of course, I've tested it with both X11 and wayland. In wayland the error message pops up as expected:

FYI without this patch the stack trace to SIGSEGV looks like this:

Thread 1 (Thread 0x7f607c898100 (LWP 4022)):
[KCrash Handler]
#7  0x00007f6077eb3d30 in _DYNAMIC () from /usr/lib64/libQt5DBus.so.5
#8  0x00007f607bfd8b0f in _XSend (dpy=dpy@entry=0x560414309f40, data=data@entry=0x0, size=size@entry=0) at /var/tmp/portage/x11-libs/libX11-1.6.7/work/libX11-1.6.7/src/xcb_io.c:496
#9  0x00007f607bfd8e80 in _XFlush (dpy=0x560414309f40) at /var/tmp/portage/x11-libs/libX11-1.6.7/work/libX11-1.6.7/src/xcb_io.c:516
#10 0x00007f607bfdba25 in _XGetRequest (dpy=dpy@entry=0x560414309f40, type=type@entry=98 'b', len=len@entry=8) at /var/tmp/portage/x11-libs/libX11-1.6.7/work/libX11-1.6.7/src/XlibInt.c:1717
#11 0x00007f607bfcf13d in XQueryExtension (dpy=dpy@entry=0x560414309f40, name=name@entry=0x7f607ab42fe0 "XInputExtension", major_opcode=major_opcode@entry=0x7ffca078b2cc, first_event=first_event@entry=0x7ffca078b2d0, first_error=first_error@entry=0x7ffca078b2d4) at /var/tmp/portage/x11-libs/libX11-1.6.7/work/libX11-1.6.7/src/QuExt.c:44
#12 0x00007f607ab42056 in get_xinput_base (dpy=0x560414309f40) at /var/tmp/portage/x11-libs/libXtst-1.2.3-r1/work/libXtst-1.2.3/src/XTest.c:79
#13 find_display (dpy=0x560414309f40) at /var/tmp/portage/x11-libs/libXtst-1.2.3-r1/work/libXtst-1.2.3/src/XTest.c:83
#14 0x00007f607ab42232 in XTestQueryExtension (dpy=0x560414309f40, event_base_return=0x7ffca078b3bc, error_base_return=0x7ffca078b3c8, major_return=0x7ffca078b3d0, minor_return=0x7ffca078b3e8) at /var/tmp/portage/x11-libs/libXtst-1.2.3-r1/work/libXtst-1.2.3/src/XTest.c:101
#15 0x0000560412579253 in main ()

P.S. If I uderstand it right, this change goes to master and not to Applications/19.04 because of the new translatable string added, though it is only bug fix?

pino added a comment.May 18 2019, 11:35 AM

P.S. If I uderstand it right, this change goes to master and not to Applications/19.04 because of the new translatable string added, though it is only bug fix?

There are three options (at least that I see):

  1. only master, Applications/19.04 will keep crashing as before (although even this patch does not change the fact that krfb does not yet work under wayland)
  2. this patch to master, and a console message to Applications/19.04 instead of the message box (which won't be seen by users launching krfb using the menu, though)
  3. ask for a string exception to the translator to kde-i18n-doc ml, and if granted commit this patch to Applications/19.04

Choose one...

This revision was automatically updated to reflect the committed changes.