| Summary: | On X11, plasmashell and Firefox hang in QXcbClipboard::waitForClipboardEvent() when Firefox attempts to update the clipboard and show a notification at the same time | ||
|---|---|---|---|
| Product: | [Plasma] plasmashell | Reporter: | nyanpasu64 <nyanpasu64> |
| Component: | Clipboard widget & pop-up | Assignee: | Plasma Bugs List <plasma-bugs> |
| Status: | CONFIRMED --- | ||
| Severity: | normal | CC: | 2724167997, ad.ruckel, alexander.s.m, calvin.f.hoy, chris.longros, edarocha1324, grgbrn, hardpenguin13, jamesxmcintosh, kdedev, MurzNN, nate, oleksandr, piotrekkr, plasma-bugs, post, postix, qydwhotmail, silvan.calarco, stransky |
| Priority: | NOR | ||
| Version: | 5.23.4 | ||
| Target Milestone: | 1.0 | ||
| Platform: | Other | ||
| OS: | Linux | ||
| URL: | https://bugreports.qt.io/browse/QTBUG-67822?jql=text%20~%20%22waitForClipboardEvent%22 | ||
| See Also: |
https://bugs.kde.org/show_bug.cgi?id=337961 https://bugs.kde.org/show_bug.cgi?id=480312 |
||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
|
Description
nyanpasu64
2021-12-06 21:06:07 UTC
(In reply to Fushan Wen from comment #1) > Addressed in > https://github.com/piroor/copy-selected-tabs-to-clipboard/pull/28 I tested that PR and it doesn't fix the hang on my machine, and I don't think that removing a call to await prevents the deadlock I described. Firefox still blocks on sending a notification, and plasmashell still blocks on reading clipboard data. Given my limited expertise, I *think* that it's reasonable for Firefox to copy something to the clipboard and then show a notification and expect it to finish synchronously, and it's unreasonable for plasmashell to assume that apps will respond to clipboard content requests, and to stop processing notifications while waiting for clipboard contents. So perhaps plasmashell and Klipper should be rewritten to not block while waiting for clipboard contents, but that's quite involved (perhaps asynchronously, which may require bypassing QXcbClipboard entirely, perhaps having background threads request clipboard contents). Alternatively Firefox could use an asynchronous function instead of libnotify's `notify_notification_show`, to respond to clipboard requests while trying to show a notification. I'm not sure how difficult that is, AKA how many round-trips d-bus's notification protocol requires, and how many states need to be created in Firefox's state machine. I can confirm this affects me as well when making a screenshot in Firefox and then trying to copy it to Klipper. Steps to reproduce: 1. CTRL+SHIFT+S to make a screenshot 2. Select desired area 3. Select "Copy" button under the selected area. Should I start another bug? The description of the observed result here fits perfectly. I use Firefox with XFCE on another computer and there was no such problem problem. What is more, I think this proves the bug is not related to the "Copy Selected Tabs to Cliboard" Firefox addon but rather it is KDE related. Following up with details: Operating System: Debian GNU/Linux KDE Plasma Version: 5.23.5 KDE Frameworks Version: 5.88.0 Qt Version: 5.15.2 Kernel Version: 5.15.0-2-amd64 (64-bit) Graphics Platform: X11 Firefox version: 96.0 (64-bits) *** Bug 453562 has been marked as a duplicate of this bug. *** > So perhaps plasmashell and Klipper should be rewritten to not block while waiting for clipboard contents
Blocking the main plasmashell thread sounds like a bad idea in general, since it always risks a freeze of the entire shell. I've experienced clipboard-related freezes in Plasma for around a decade now so it would be amazing to see this fixed at some point. :)
## Popping up menus upon copying URLs At https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L825-833, you call URLGrabber::checkNewData() (which pops up a menu if Klipper's Action Menu is enabled and you're not copying from Firefox), *then* skip assigning lastURLGrabberText if the text matches. I believe the intent was to skip calling URLGrabber::checkNewData() if the text matches, but that would mean copying the same text twice doesn't popup a menu the second time. Should this be changed to the design intent (which changes clipboard behavior), or should we remove m_lastURLGrabberTextSelection and m_lastURLGrabberTextClipboard altogether? ## Fixing the bug through asynchronous work On the topic of this bug, QMimeData::text() is blocking and there's no continuation-based alternative (even though the underlying xcb is asynchronous). - The hard solution is to get text through raw xcb (and I'm not familiar with X so I can't do this without a lot of new learning). - The easiest solution I can think of, which minimizes learning unknown APIs and code changes, is to move fetching clipboard contents (which can block) to either a new thread or a job in QThreadPool::globalInstance(). - This job will call HistoryItem::create(clipData), and sends the data back to a new Klipper::onClipData(HistoryItemPtr) method called in the main thread, which inserts the resulting HistoryItemPtr into history (hopefully in the same order as you copied text). - How do we send a HistoryItemPtr back to Klipper on the main thread? - Add our own message queue which workers push onto, and having the Qt event loop poll it and invoke Klipper (probably difficult to achieve). - The worker job holds a QPointer<Klipper> and sends a custom QEvent subclass there. Overload QObject::event() in Klipper to dispatch on our new event ID. (Creates a use-after-free if the main thread deletes Klipper after it checks it's alive but before it sends a message.) - The worker job holds a QPointer<Klipper> and calls QMetaObject::invokeMethod(Klipper*, &Klipper::onClipData, HistoryItemPtr) (which internally sends a QMetaCallEvent, but Qt handles invoking the right method for us). (Creates a use-after-free if the main thread deletes Klipper after it checks it's alive but before it sends a message.) - Give the worker job a signal clipDataReady(HistoryItemPtr), and connect it to Klipper::onClipData(HistoryItemPtr) with a QueuedConnection. (I don't like dynamically adding connections, but it's immune to use-after-free I hope. But QRunnable can't have signals unless you inherit from QObject *and* QRunnable! https://forum.qt.io/post/357008) - Let the worker job update a QFutureInterface (stable but undocumented internal Qt type, similar to the documented QPromise in Qt 6), wrap a QFutureWatcher around the QFuture, and connect to Klipper::onClipData(HistoryItemPtr). At this point it's so complicated I'd rather inherit from QObject and QRunnable. - Should we wait for all worker threads to finish before destroying Klipper? Probably doesn't matter, it's fine to destroy the signal connections with Klipper, and let the background threads work until the process exits. (Does QApplication::exec() wait for all QThreadPool tasks to finish? I don't know. I sure hope that we don't get stuck background threads preventing plasmashell termination.) The least bad solution I've come up with is a worker object inheriting from QObject and QRunnable, emitting a clipDataReady signal before exiting. Is there a better solution? I hope so. ## Reentrancy Also we will need to make sure that splitting functionality across asynchronous callbacks does not interact incorrectly with Klipper::m_locklevel (apparently designed to prevent reentrant calls?). I'm not sure all the ways Klipper::m_locklevel is manipulated, but the check in Klipper::applyClipChanges() (https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L657) seems to be completely redundant. Klipper::newClipData() (https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L681) returns early if m_locklevel is nonzero, before calling -> Klipper::checkClipData -> Klipper::applyClipChanges, with no increments to m_locklevel along the way. The latter two methods have no other callers or references, but are bizarrely protected methods despite Klipper having no subclasses in all of plasma-workspace (and I hope it has no external subclasses in other Qt projects, https://lxr.kde.org/search?%21v=kf5-qt5&_filestring=&_string=Klipper&_casesensitive=1 didn't return anything). Because there is no other path to call Klipper::applyClipChanges() except through Klipper::newClipData() (which returns if m_locklevel != 0), then m_locklevel must be 0 in Klipper::applyClipChanges(). But I'd make this into an assertion to verify my reasoning, rather than remove this check entirely. So in any case I don't *think* Klipper::m_locklevel matters. Problem: QMimeData is non-copyable, and obtained from KSystemClipboard (either QtClipboard or WaylandClipboard), and the main thread might erase it while the worker thread is in the middle of calling QMimeData::text() to request data from another process. Do we need to move all KSystemClipboard access to another thread, and only send contents to the main thread? Or is qGuiApp->clipboard() only safe to call (or call methods on QClipboard) from the main thread? In which case I see no solution without abandoning KSystemClipboard/QClipboard altogether. *** Bug 459766 has been marked as a duplicate of this bug. *** Temporary solution that worked for me is to disable Firefox notifications: https://support.mozilla.org/en-US/questions/1283277 https://support.mozilla.org/en-US/questions/1345694 *** Bug 462622 has been marked as a duplicate of this bug. *** *** Bug 360262 has been marked as a duplicate of this bug. *** Still happens with Firefox 125.0.1 and KDE Framework Version 5.115.0, Qt Version 5.15.12 (built against 5.15.12). Workaround with setting Firefox flag in about:config alerts.useSystemBackend=false but this is a workaround and should be fixed properly. *** Bug 393804 has been marked as a duplicate of this bug. *** This seems somewhat related to this other bug, although the crashes are different https://bugs.kde.org/show_bug.cgi?id=480312 |