hack around Xlib deadlock by Be-ing · Pull Request #1923 · mixxxdj/mixxx

Be-ing · 2018-11-28T16:42:00Z

adapted code from https://gitlab.freedesktop.org/xorg/lib/libx11/issues/25#note_17248
https://bugs.launchpad.net/mixxx/+bug/1805559

I have not had time to extensively test this yet.

adapted code from https://gitlab.freedesktop.org/xorg/lib/libx11/issues/25#note_17248 https://bugs.launchpad.net/mixxx/+bug/1805559

Be-ing · 2018-11-28T17:11:00Z

Oops this breaks running with -platform wayland...

Otherwise calling Xlib functions segfaults when running as a Wayland client

uklotzde · 2018-11-28T22:13:34Z

I've cherry-pricked the commits for testing. But since I didn't experience the issue I can only test for any unwanted side effects.

Running Mixxx with QtWayland never really worked, even without this patch. The UI is incomplete and Mixxx is unresponsive, so I won't test this use case.

Be-ing · 2018-11-28T22:44:23Z

Yes the QPA Wayland backend has many issues that make it not ready for production. Just at first glance, high DPI scaling is somehow handled differently (a scale factor of 1.5 looks the same as 3.0 with X??) and my theme does not apply. I just wanted to make sure this didn't break running with the QPA Wayland backend any more than Qt is broken with it. I would not be surprised if it becomes the default in the future.

uklotzde · 2018-11-29T07:35:45Z

 #include "preferences/dialog/dlgprefmodplug.h"
 #endif

+#ifdef Q_OS_LINUX


Please replace all occurrences of #ifdef Q_OS_LINUX in this patch with #if defined(Q_WS_X11) that we already used for this purpose in other parts of Mixxx.

I can't find any documentation for Q_WS_X11 for Qt5. Conceptually it doesn't make sense. The window system is not determined at compile, but at runtime. I think Q_OS_LINUX is correct.

What happens if you neither have X11 nor its development headers installed? This might not happen soon, but it will happen. We use Qt as an abstraction layer and usually it doesn't matter. But here the abstraction leaks through at compile time.

I think we can defer that until it becomes a real problem. If Qt doesn't help us when that comes, we'll have to detect the presence of X headers in our build system.

uklotzde · 2018-11-29T07:55:32Z

+typedef Bool (*WireToErrorType)(Display*, XErrorEvent*, xError*);
+
+const int NUM_HANDLERS = 256;
+WireToErrorType __oldHandlers[NUM_HANDLERS] = {0};


I still don't get the magic behind this array. Which unknown side effect sets the elements to a value other than 0? Currently the error interceptor does nothing except ignoring and dropping all incoming extension errors. This can be implemented in a much simpler way.

Honestly, neither do I, nor do I understand the magic number 256. I've never worked with X11 code before, much less understand X's internals. I just copied and pasted from https://bugs.freedesktop.org/show_bug.cgi?id=59361#c4 and got it to build.

256 because the error code is an unsigned char with values ranging from 0 to 255. But all slots are initialized with 0 (= nullptr) and are never updated.

uklotzde · 2018-12-01T19:19:09Z

Missing dependency in .travis.yml!

uklotzde · 2018-12-01T19:24:51Z

After installing qt5-qtx11extras-devel I get this build error:

[CXX] src/mixxx.cpp
src/mixxx.cpp:84:10: fatal error: QtGui/QX11Info: No such file or directory
 #include <QtGui/QX11Info>
          ^~~~~~~~~~~~~~~~
compilation terminated.

This include works:

#include <QtX11Extras/QX11Info>

uklotzde · 2018-12-01T20:19:10Z

Next failure during linking:

/usr/bin/ld: lin64_build/mixxx.o: in function `MixxxMainWindow::MixxxMainWindow(QApplication*, CmdlineArgs const&)':
/home/uk/Projects/Mixxx/mixxx/src/mixxx.cpp:187: undefined reference to `qt_x11_wait_for_window_manager(QWidget*)'
collect2: error: ld returned 1 exit status

Be-ing · 2018-12-03T19:25:13Z

Well this is interesting. That link error is because I switched from Q_WS_X11 to Q_OS_LINUX. I tested switching that ifdef back to Q_WS_X11 and adding a qWarning, and the qWarning was not printed, so it seems Q_WS_X11 is not defined with Qt 5. The call to qt_x11_wait_for_window_manager was added by @daschuer when implementing the splash screen for the 2.0 release. Considering that it hasn't actually been used since switching to Qt 5 and this doesn't seem to be causing any regressions, I presume it's safe to remove.

This was previously guarded by an #ifdef Q_WS_X11, but switching to Q_OS_LINUX caused a link error: /usr/bin/ld: lin64_build/mixxx.o: in function `MixxxMainWindow::MixxxMainWindow(QApplication*, CmdlineArgs const&)': /home/uk/Projects/Mixxx/mixxx/src/mixxx.cpp:187: undefined reference to `qt_x11_wait_for_window_manager(QWidget*)' It turns out that Q_WS_X11 is no longer defined with Qt5, yet it does not seem that building Mixxx without the call to qt_x11_wait_for_window_manager has caused any issues, so I presume it is safe to remove it now. This was the only place in Mixxx where Q_WS_X11 was referenced.

uklotzde · 2018-12-03T21:46:18Z

libqt5x11extras5-dev is still missing in .travis.yml to fix the CI build

uklotzde · 2018-12-04T07:46:33Z

Using the unmodified patch is feasible since we have no clue how the code actually is intended to work. As far as I understand it suppresses errors to avoid getting stuck at a subsequent lock.

If this workaround is effective (what I'm not able to validate) I have no objections merging it. LGTM.

Be-ing · 2018-12-05T16:17:55Z

I experienced no crash after playing for 1.5 hours. Of course that doesn't necessarily mean the problem is solved, but I'll keep testing.

Be-ing added the major bug label Nov 28, 2018

Be-ing added this to the 2.2.0 milestone Nov 28, 2018

Be-ing force-pushed the xlib_deadlock branch from 36a9c3d to d700ea5 Compare November 28, 2018 16:43

hack around Xlib deadlock

8c7b955

adapted code from https://gitlab.freedesktop.org/xorg/lib/libx11/issues/25#note_17248 https://bugs.launchpad.net/mixxx/+bug/1805559

Be-ing force-pushed the xlib_deadlock branch from d700ea5 to 8c7b955 Compare November 28, 2018 16:44

only hack around Xlib bug when running as an X client

c82b315

Otherwise calling Xlib functions segfaults when running as a Wayland client

uklotzde reviewed Nov 29, 2018

View reviewed changes

Comment thread src/mixxx.cpp Outdated

Be-ing force-pushed the xlib_deadlock branch 3 times, most recently from 83e5780 to 02af45b Compare November 29, 2018 22:02

use #if defined rather than #ifdef

8fa52d4

Be-ing force-pushed the xlib_deadlock branch from 02af45b to 8fa52d4 Compare November 29, 2018 22:05

Be-ing added 2 commits December 3, 2018 13:32

fix #include

d60b287

add libqt5x11extras5-dev to Travis dependencies

7e5557a

Be-ing merged commit d37d407 into mixxxdj:2.2 Dec 5, 2018

Be-ing deleted the xlib_deadlock branch April 25, 2019 01:02

mixxxbot mentioned this pull request Aug 23, 2022

Xlib deadlock in GUI thread #9533

Closed

Uh oh!

Conversation

Be-ing commented Nov 28, 2018

Uh oh!

Be-ing commented Nov 28, 2018

Uh oh!

uklotzde commented Nov 28, 2018

Uh oh!

Be-ing commented Nov 28, 2018

Uh oh!

uklotzde Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

Be-ing Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

uklotzde Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

uklotzde Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

Be-ing Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

uklotzde Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uklotzde commented Dec 1, 2018

Uh oh!

uklotzde commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Dec 1, 2018

Uh oh!

Be-ing commented Dec 3, 2018

Uh oh!

uklotzde commented Dec 3, 2018

Uh oh!

uklotzde commented Dec 4, 2018

Uh oh!

Be-ing commented Dec 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

uklotzde Nov 29, 2018 •

edited

Loading

uklotzde commented Dec 1, 2018 •

edited

Loading