Skip to content

fix: prevent compositor crash on screen hotplug#1017

Draft
deepin-wm wants to merge 1 commit into
linuxdeepin:masterfrom
deepin-wm:fix/null-ptr-dangling-surface-crash
Draft

fix: prevent compositor crash on screen hotplug#1017
deepin-wm wants to merge 1 commit into
linuxdeepin:masterfrom
deepin-wm:fix/null-ptr-dangling-surface-crash

Conversation

@deepin-wm

@deepin-wm deepin-wm commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix crash when hot-plugging screens during control center operation.

Changes

  1. shellhandler.cpp: Connect SurfaceWrapper::aboutToBeInvalidated in registerSurfaceToForeignToplevel to call removeSurface on invalidation, fixing a dangling pointer in ForeignToplevelManagerInterfaceV1::m_surfaces left when a registered surface is destroyed without removal.

  2. inputmanagerinterfacev1.cpp: Skip input-manager resources whose client has no wl_seat binding in bind_resource, sendCapabilityAvailable/sendCapabilityUnavailable, and onInputAdded/onInputRemoved. Capability events reference wl_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:

  • Remove foreign toplevel surfaces when their associated SurfaceWrapper objects are destroyed to avoid dangling pointers.
  • Skip capability broadcasting for input manager resources with null wlr_seat_client instances to prevent null dereference crashes.

@deepin-wm deepin-wm marked this pull request as draft June 18, 2026 12:15
@sourcery-ai

sourcery-ai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This 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 manager

sequenceDiagram
    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)
Loading

Sequence diagram for seatClient null handling in capability broadcast

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
Ensure foreign toplevel manager does not retain dangling pointers to destroyed SurfaceWrappers.
  • Extended registerSurfaceToForeignToplevel to connect to the wrapper's QObject::destroyed signal.
  • On destruction of a SurfaceWrapper, invoke removeSurface on the foreign toplevel manager to keep its surface list in sync.
src/core/shellhandler.cpp
Harden input manager against null wlr_seat_client when broadcasting capabilities and handling input device changes.
  • In bind_resource, bail out early if client_for_wl_client returns a null seat client before iterating resources.
  • In sendCapabilityAvailable and sendCapabilityUnavailable, skip resources whose associated seat client is null.
  • In onInputAdded and onInputRemoved, guard the seat client lookup and continue the loop when it is null to avoid dereferencing freed or missing seat clients.
src/modules/input-manager/inputmanagerinterfacev1.cpp

Assessment against linked issues

Issue Objective Addressed Explanation
#183 Synchronize QML surface resizing with client size-acknowledgement so that QML size does not update before the client acknowledges the size change. The PR only adds null checks around seat clients in the input manager and connects SurfaceWrapper destruction to removeSurface in the foreign toplevel manager. It does not modify any resize, ack, or repaint synchronization logic between QML and the client.
#183 Eliminate visual artifacts during fast window resizing (surface clipping, stretching, or shaking) caused by the desynchronization between QML size and client surface size. No changes in the PR relate to rendering, geometry updates, or resize event handling. The modifications address crashes (dangling pointer and null seatClient) rather than the repaint behavior or visual artifacts during resizing.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot

Copy link
Copy Markdown

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-wm deepin-wm force-pushed the fix/null-ptr-dangling-surface-crash branch from fb935f4 to 4a8f6c2 Compare June 18, 2026 13:03
@deepin-bot

deepin-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown

TAG Bot

New tag: 0.8.12
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #1036

@deepin-bot

deepin-bot Bot commented Jun 26, 2026

Copy link
Copy Markdown

TAG Bot

New tag: 0.8.13
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #1063

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
@deepin-wm deepin-wm force-pushed the fix/null-ptr-dangling-surface-crash branch from 4a8f6c2 to 287433b Compare July 2, 2026 14:02
@deepin-wm deepin-wm changed the title fix: prevent crash from dangling ptr and null seatClient fix: prevent compositor crash on screen hotplug Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resizing repaint not synchoronized

2 participants