fix: prevent crash when closing wayland popup surfaces#453
Merged
Conversation
Reviewer's GuideDefers Wayland popup window destruction to avoid reentrancy-related crashes in QWaylandShmBackingStore::beginPaint and adds a global event filter to drop paint requests for windows with invalid Wayland handles. Sequence diagram for deferred Wayland popup close handlingsequenceDiagram
participant WaylandCompositor
participant QWaylandShmBackingStore
participant PluginPopupSurface
participant QApplication
participant PopupWidget
participant QWindow
WaylandCompositor->>QWaylandShmBackingStore: frame begins
QWaylandShmBackingStore->>QWaylandShmBackingStore: beginPaint
QWaylandShmBackingStore->>WaylandCompositor: process Wayland events
WaylandCompositor-->>PluginPopupSurface: plugin_popup_close
PluginPopupSurface->>PluginPopupSurface: NullHandleGuard::install
PluginPopupSurface->>QApplication: QMetaObject::invokeMethod
note over QApplication: Queue lambda to close popup via Qt::QueuedConnection
QWaylandShmBackingStore-->>QWaylandShmBackingStore: beginPaint resumes safely
QApplication-->>A QApplication: next event loop iteration
QApplication->>A QApplication: execute queued lambda
A QApplication->>QApplication: find popup top level widget
QApplication->>PopupWidget: hide
QApplication->>QWindow: close
QWindow->>WaylandCompositor: destroy Wayland surface
Updated class diagram for PluginPopupSurface and NullHandleGuardclassDiagram
class QObject
class QApplication
class QWidget
class QWindow
class QEvent
class NullHandleGuard {
+NullHandleGuard(parent : QObject)
+static install() void
+eventFilter(watched : QObject, event : QEvent) bool
}
class PluginSurface {
+PluginSurface(manager : PluginManagerIntegration, window : QWindow)
}
class PluginPopupSurface {
-m_window : QWindow*
+PluginPopupSurface(manager : PluginManagerIntegration, window : QWindow)
+~PluginPopupSurface()
+plugin_popup_close() void
+plugin_popup_geometry(x : int32_t, y : int32_t, width : int32_t, height : int32_t) void
}
class PluginManagerIntegration
QObject <|-- NullHandleGuard
QObject <|-- QApplication
QObject <|-- QWidget
QObject <|-- QWindow
PluginSurface --> QWindow : owns window
PluginPopupSurface --> QWindow : uses m_window
PluginPopupSurface --> PluginManagerIntegration : uses
NullHandleGuard ..> QApplication : installsEventFilter
NullHandleGuard ..> QWidget : inspects
NullHandleGuard ..> QWindow : inspects handle
NullHandleGuard ..> QEvent : filters UpdateRequest
PluginPopupSurface ..> NullHandleGuard : uses install
PluginPopupSurface ..> QApplication : uses invokeMethod and topLevelWidgets
PluginPopupSurface ..> QWidget : finds popupWidget
PluginPopupSurface ..> QWindow : close via queued lambda
Flow diagram for NullHandleGuard UpdateRequest filteringflowchart TD
A[QApplication event loop receives event] --> B{event type is UpdateRequest}
B -- no --> Z[Pass event to QObject eventFilter]
B -- yes --> C[Determine watched object type]
C --> D{watched is QWidget}
C --> E{watched is QWindow}
D -- yes --> F[Resolve QWindow from QWidget via windowHandle or window]
D -- no --> E
E -- yes --> G[Assign watched as QWindow]
E -- no --> H[No QWindow and watched may be QWidget]
F --> I{QWindow exists}
G --> I
I -- no --> J[Drop UpdateRequest return true]
I -- yes --> K{QWindow handle is null}
K -- yes --> J
K -- no --> Z
H --> L{watched is QWidget and no QWindow}
L -- yes --> J
L -- no --> Z
Z[Fallback to QObject eventFilter return QObject::eventFilter]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider scoping
NullHandleGuardto only watch the specific tray/popup windows instead of installing a global filter onqApp, to reduce the risk of dropping legitimateUpdateRequestevents for unrelated widgets/windows. - In
NullHandleGuard::eventFilter, the branch that dropsUpdateRequestfor anyQWidgetthat has lost itsQWindowis quite broad; it may be safer to tighten the condition (e.g., check widget type or ownership) so that transient states during normal widget lifetime are not accidentally blocked. - The
NullHandleGuard::installstatic flag is not thread-safe; if popup close can be triggered from different threads in the future, consider guarding installation withstd::call_onceor ensuring it is only ever called from the GUI thread.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `NullHandleGuard` to only watch the specific tray/popup windows instead of installing a global filter on `qApp`, to reduce the risk of dropping legitimate `UpdateRequest` events for unrelated widgets/windows.
- In `NullHandleGuard::eventFilter`, the branch that drops `UpdateRequest` for any `QWidget` that has lost its `QWindow` is quite broad; it may be safer to tighten the condition (e.g., check widget type or ownership) so that transient states during normal widget lifetime are not accidentally blocked.
- The `NullHandleGuard::install` static flag is not thread-safe; if popup close can be triggered from different threads in the future, consider guarding installation with `std::call_once` or ensuring it is only ever called from the GUI thread.
## Individual Comments
### Comment 1
<location path="src/tray-wayland-integration/pluginsurface.cpp" line_range="19" />
<code_context>
namespace Plugin {
+
+class NullHandleGuard : public QObject {
+public:
+ static void install() {
</code_context>
<issue_to_address>
**issue (complexity):** Consider scoping the crash guard and widget handling directly to the popup window to avoid global hooks and implicit searches.
You can keep the crash workaround but make it more local and explicit with two small refactors:
1. **Scope the guard to the popup window instead of qApp**
Rather than a global event filter inspecting every `UpdateRequest`, track the specific popup window(s) and filter only those. This keeps the behavior but makes the flow more obvious.
```c++
class NullHandleGuard : public QObject {
public:
explicit NullHandleGuard(QWindow *window, QObject *parent = nullptr)
: QObject(parent), m_window(window) {
if (m_window)
m_window->installEventFilter(this);
}
protected:
bool eventFilter(QObject *watched, QEvent *event) override {
if (watched != m_window)
return QObject::eventFilter(watched, event);
if (event->type() == QEvent::UpdateRequest) {
// Drop UpdateRequest to prevent QWaylandShmBackingStore from crashing
// in beginPaint/decoration() when trying to paint a destroyed window.
if (m_window && !m_window->handle())
return true;
}
return QObject::eventFilter(watched, event);
}
private:
QPointer<QWindow> m_window;
};
```
Then, in `PluginPopupSurface` (e.g. ctor or a dedicated init method), attach the guard:
```c++
PluginPopupSurface::PluginPopupSurface(...)
: ...
{
// ...
m_nullHandleGuard.reset(new NullHandleGuard(m_window, m_window));
}
```
(Where `m_nullHandleGuard` is a `std::unique_ptr<NullHandleGuard>` or `QScopedPointer<NullHandleGuard>` member.)
This removes the global `qApp` filter and the QWidget/QWindow probing logic while preserving the “drop paint on destroyed window” behavior for the affected popup.
2. **Avoid `topLevelWidgets()` scan by storing the popup widget**
Instead of scanning all top-level widgets in the deferred lambda, store an explicit association when the popup is created. For example, if you can get the widget at construction time:
```c++
class PluginPopupSurface : public QtWaylandClient::QWaylandShellSurface {
// ...
QPointer<QWidget> m_popupWidget;
};
// In the ctor or setup:
PluginPopupSurface::PluginPopupSurface(...)
: ...
{
// Example: if PluginPopup::get(m_window) returns/knows the widget:
if (auto popup = PluginPopup::get(m_window))
m_popupWidget = popup->widget(); // adjust to your actual API
}
```
Then `plugin_popup_close()` becomes simpler and avoids the global search:
```c++
void PluginPopupSurface::plugin_popup_close()
{
QPointer<QWindow> safeWindow(m_window);
QPointer<QWidget> safeWidget(m_popupWidget);
QMetaObject::invokeMethod(qApp, [safeWindow, safeWidget]() {
if (!safeWindow)
return;
if (safeWidget)
safeWidget->hide();
safeWindow->close();
}, Qt::QueuedConnection);
}
```
This keeps the deferred destruction and crash avoidance intact, but removes the cross-cutting globals (`qApp` event filter + `topLevelWidgets()` scan) and makes the lifecycle easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
yixinshark
force-pushed
the
fix-windowCloseCrash
branch
from
ac598d8 to
243971f
Compare
- Problem: dde-tray-loader crashes with SEGFAULT in QWaylandWindow::decoration() during popup closing. - Reason: Reentrancy in QWaylandShmBackingStore::beginPaint(). It dispatches Wayland events synchronously, which can trigger plugin_popup_close() in the middle of a paint operation. Synchronously closing the window destroys the platform handle, causing beginPaint to crash when it resumes. - Solution 1: Use QMetaObject::invokeMethod with Qt::QueuedConnection in plugin_popup_close() to defer window destruction until the current paint operation completes. - Solution 2: Implement a global NullHandleGuard event filter to intercept and drop any UpdateRequest events directed at widgets or windows whose Wayland handles have been invalidated. - 问题: 弹窗关闭时 dde-tray-loader 崩溃，堆栈指向 QWaylandWindow::decoration()。 - 原因: QWaylandShmBackingStore::beginPaint() 在绘制过程中同步分发 Wayland 事件，导致 plugin_popup_close() 被重入调用。同步关闭窗口会销毁底层 handle，导致 beginPaint 恢复执行时访问空指针崩溃。 - 解决 1: 在 plugin_popup_close() 中使用 QMetaObject::invokeMethod 配合队列连接，将窗口销毁操作推迟到当前绘制周期结束之后。 - 解决 2: 实现全局事件过滤器 NullHandleGuard，拦截并丢弃所有指向已销毁 Wayland 表面或无效窗口的 UpdateRequest 请求，防止僵尸绘制事件触发崩溃。 Log: prevent crash when closing wayland popup surfaces Pms: BUG-358679 Change-Id: I2610d04675ee628080eab2c52171f39d9d752c91
yixinshark
force-pushed
the
fix-windowCloseCrash
branch
from
243971f to
6157880
Compare
deepin pr auto review这份代码 diff 主要是为了修复 Wayland 环境下，在窗口关闭过程中因 Wayland Surface 被销毁而导致的崩溃问题。引入了 以下是对这段代码的详细审查和改进建议： 1. 语法与逻辑审查
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
5. 改进意见优化建议 1：优化 Widget 查找逻辑在 如果无法保存，建议利用 优化建议 2：日志级别调整建议将 // 修改前
qDebug() << "NullHandleGuard Dropped UpdateRequest for" << watched << "(null Wayland handle)";
// 修改后
qWarning() << "NullHandleGuard Dropped UpdateRequest for" << watched << "(null Wayland handle)";优化建议 3：代码注释与格式代码注释非常详细，解释了为什么要推迟销毁（防止 /**
* @brief NullHandleGuard
* 全局事件过滤器，用于防止在 Wayland 窗口销毁过程中因底层 Surface 失效
* 而导致的绘制崩溃。
*
* 它拦截 UpdateRequest 事件，并检查 QWindow 的 handle 是否有效。
* 如果无效，则丢弃该事件，防止 QWaylandShmBackingStore 访问空指针。
*/
class NullHandleGuard : public QObject {
// ...
};优化建议 4：
|
18202781743
reviewed
Apr 24, 2026
|
|
||
| protected: | ||
| bool eventFilter(QObject *watched, QEvent *event) override { | ||
| if (event->type() == QEvent::UpdateRequest) { |
18202781743
approved these changes
Apr 24, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: dde-tray-loader crashes with SEGFAULT in QWaylandWindow::decoration() during popup closing.
Reason: Reentrancy in QWaylandShmBackingStore::beginPaint(). It dispatches Wayland events synchronously, which can trigger plugin_popup_close() in the middle of a paint operation. Synchronously closing the window destroys the platform handle, causing beginPaint to crash when it resumes.
Solution 1: Use QMetaObject::invokeMethod with Qt::QueuedConnection in plugin_popup_close() to defer window destruction until the current paint operation completes.
Solution 2: Implement a global NullHandleGuard event filter to intercept and drop any UpdateRequest events directed at widgets or windows whose Wayland handles have been invalidated.
问题: 弹窗关闭时 dde-tray-loader 崩溃，堆栈指向 QWaylandWindow::decoration()。
原因: QWaylandShmBackingStore::beginPaint() 在绘制过程中同步分发 Wayland 事件，导致 plugin_popup_close() 被重入调用。同步关闭窗口会销毁底层 handle，导致 beginPaint 恢复执行时访问空指针崩溃。
解决 1: 在 plugin_popup_close() 中使用 QMetaObject::invokeMethod 配合队列连接，将窗口销毁操作推迟到当前绘制周期结束之后。
解决 2: 实现全局事件过滤器 NullHandleGuard，拦截并丢弃所有指向已销毁 Wayland 表面或无效窗口的 UpdateRequest 请求，防止僵尸绘制事件触发崩溃。
Log: prevent crash when closing wayland popup surfaces
Pms: BUG-358679
Change-Id: I2610d04675ee628080eab2c52171f39d9d752c91
Summary by Sourcery
Defer destruction of Wayland popup windows and guard against painting invalidated surfaces to prevent crashes when closing Wayland popups.
Bug Fixes:
Enhancements:
Documentation: