fix: prevent compositor crash on screen hotplug#1017
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes two crash scenarios by cleaning up foreign toplevel surfaces when their wrappers are destroyed and by adding defensive null checks around seat client lookups during input capability handling. Sequence diagram for SurfaceWrapper destruction cleanup in foreign toplevel managersequenceDiagram
participant ShellHandler
participant SurfaceWrapper
participant TreelandForeignToplevel
ShellHandler->>ShellHandler: registerSurfaceToForeignToplevel(wrapper)
ShellHandler->>TreelandForeignToplevel: addSurface(wrapper)
ShellHandler->>SurfaceWrapper: connect(QObject::destroyed, lambda)
SurfaceWrapper-->>SurfaceWrapper: QObject::destroyed
SurfaceWrapper-->>ShellHandler: destroyed signal
ShellHandler->>TreelandForeignToplevel: removeSurface(wrapper)
Sequence diagram for seatClient null handling in capability broadcastsequenceDiagram
participant InputManager as TreelandInputManagerInterfaceV1
participant Helper
participant Seat
participant SeatHandle
participant SeatClient as wlr_seat_client
InputManager->>Helper: instance()
Helper-->>InputManager: helper
InputManager->>Seat: seat()
Seat-->>InputManager: seat
InputManager->>SeatHandle: handle()
SeatHandle-->>InputManager: handle
InputManager->>SeatHandle: client_for_wl_client(wl_client)
SeatHandle-->>InputManager: seatClient
alt [seatClient exists]
InputManager->>SeatClient: iterate resources
InputManager->>SeatClient: send_capability_available(handle, types, clientResource)
else [seatClient is null]
InputManager-->>InputManager: return or continue without sending
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ShellHandler::registerSurfaceToForeignToplevel, consider connecting to thedestroyed(QObject*)signal and using the passedQObject*(casted if needed) instead of capturingwrapperin the lambda, to avoid relying on a pointer to an object that is in the process of being destroyed. - The repeated pattern of
client_for_wl_clientlookup plus null-check forseatClientacrossbind_resource,sendCapabilityAvailable,sendCapabilityUnavailable,onInputAdded, andonInputRemovedcould be factored into a small helper to reduce duplication and keep the flow in those functions clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ShellHandler::registerSurfaceToForeignToplevel`, consider connecting to the `destroyed(QObject*)` signal and using the passed `QObject*` (casted if needed) instead of capturing `wrapper` in the lambda, to avoid relying on a pointer to an object that is in the process of being destroyed.
- The repeated pattern of `client_for_wl_client` lookup plus null-check for `seatClient` across `bind_resource`, `sendCapabilityAvailable`, `sendCapabilityUnavailable`, `onInputAdded`, and `onInputRemoved` could be factored into a small helper to reduce duplication and keep the flow in those functions clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-wm 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 |
fb935f4 to
4a8f6c2
Compare
|
TAG Bot New tag: 0.8.12 |
|
TAG Bot New tag: 0.8.13 |
1. Connect SurfaceWrapper::aboutToBeInvalidated in registerSurfaceToForeignToplevel to call removeSurface on invalidation, fixing a dangling pointer in ForeignToplevelManagerInterfaceV1 surfaces left when a registered surface is destroyed 2. Skip input-manager resources whose client has no wl_seat binding in bind_resource and the capability broadcast paths; capability events reference wl_seat, so an unbound client cannot receive them and is skipped by protocol semantics, fixing a SEGV in the disconnect race Log: Fixed crash when hot-plugging screens while the control center is opening Influence: 1. Test hot-plugging external screens while the control center opens 2. Verify no crash when a client disconnects during capability broadcast 3. Test stability across rapid screen plug/unplug cycles fix: 修复屏幕热插拔时合成器崩溃 1. 在 registerSurfaceToForeignToplevel 中连接 SurfaceWrapper::aboutToBeInvalidated,在失效时调用 removeSurface, 修复注册 surface 被销毁时 ForeignToplevelManagerInterfaceV1 中残留的 悬空指针 2. 在 bind_resource 及能力广播路径中跳过未绑定 wl_seat 的 input-manager resource;能力事件引用 wl_seat,未绑定客户端无法接收, 按协议语义跳过,修复客户端断连竞态中的 SEGV Log: 修复控制中心开启过程中热插拔屏幕导致的崩溃 Influence: 1. 测试控制中心开启时热插拔外接屏幕 2. 验证客户端在能力广播期间断连不崩溃 3. 测试快速插拔屏幕循环的系统稳定性 Fixes: linuxdeepin#183
4a8f6c2 to
287433b
Compare
Summary
Fix crash when hot-plugging screens during control center operation.
Changes
shellhandler.cpp: Connect
SurfaceWrapper::aboutToBeInvalidatedinregisterSurfaceToForeignToplevelto callremoveSurfaceon invalidation, fixing a dangling pointer inForeignToplevelManagerInterfaceV1::m_surfacesleft when a registered surface is destroyed without removal.inputmanagerinterfacev1.cpp: Skip input-manager resources whose client has no
wl_seatbinding inbind_resource,sendCapabilityAvailable/sendCapabilityUnavailable, andonInputAdded/onInputRemoved. Capability events referencewl_seat, so a client without that binding cannot receive them and is skipped by protocol semantics — fixing a SEGV in the client-disconnect race (root-cause fix, not a defensive null check).Related Issue
Fixes #183
Multica: WM-36
Summary by Sourcery
Prevent crashes during control center operation by cleaning up destroyed foreign toplevel surfaces and guarding against null seat clients when broadcasting input capabilities.
Bug Fixes: