Skip to content

fix: use safeConnect for aboutToBeInvalidated#605

Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs:master
Oct 24, 2025
Merged

fix: use safeConnect for aboutToBeInvalidated#605
zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs:master

Conversation

@zccrs
Copy link
Member

@zccrs zccrs commented Oct 23, 2025

The WSurface's aboutToBeInvalidated signal connection was incorrectly using QObject::connect instead of safeConnect. This could lead to issues when the WSurface object is modified using setSurface, as the previous object's aboutToBeInvalidated signal would not be disconnected, potentially causing a crash or unexpected behavior. This change replaces the regular QObject::connect with safeConnect to ensure proper disconnection and avoid potential issues when the surface is replaced.

Influence:

  1. Test replacing a WSurface multiple times with different WSurface objects using setSurface.
  2. Verify that the releaseResources function is only called when the current WSurface is truly invalidated and not prematurely.
  3. Observe application behavior under heavy WSurface replacement scenarios to ensure stability and prevent crashes.

fix: 使用 safeConnect 连接 aboutToBeInvalidated 信号

WSurface 的 aboutToBeInvalidated 信号连接错误地使用了
QObject::connect 而不是 safeConnect。当使用 setSurface 修改 WSurface 对象时,这可能导致问题,因为前一个对象的 aboutToBeInvalidated 信号不会断开,可能导致崩溃或意外行为。此更改将常规的 QObject::connect
替换为 safeConnect,以确保正确断开连接并避免在表面替换时出现潜在问题。

Influence:

  1. 使用 setSurface 多次用不同的 WSurface 对象替换 WSurface。
  2. 验证 releaseResources 函数仅在当前 WSurface 真正失效时才被调用,而 不是过早调用。
  3. 观察在大量 WSurface 替换场景下的应用程序行为,以确保稳定并防止崩溃。

Summary by Sourcery

Bug Fixes:

  • Replace QObject::connect with safeConnect for the aboutToBeInvalidated signal in WSurfaceItem to avoid stale connections and unexpected behavior when setting a new surface.

The WSurface's `aboutToBeInvalidated` signal connection was incorrectly
using `QObject::connect` instead of `safeConnect`. This could lead
to issues when the WSurface object is modified using `setSurface`,
as the previous object's `aboutToBeInvalidated` signal would not be
disconnected, potentially causing a crash or unexpected behavior. This
change replaces the regular `QObject::connect` with `safeConnect` to
ensure proper disconnection and avoid potential issues when the surface
is replaced.

Influence:
1. Test replacing a WSurface multiple times with different WSurface
objects using `setSurface`.
2. Verify that the `releaseResources` function is only called when the
current WSurface is truly invalidated and not prematurely.
3. Observe application behavior under heavy WSurface replacement
scenarios to ensure stability and prevent crashes.

fix: 使用 safeConnect 连接 aboutToBeInvalidated 信号

WSurface 的 `aboutToBeInvalidated` 信号连接错误地使用了
`QObject::connect` 而不是 `safeConnect`。当使用 `setSurface` 修改
WSurface 对象时,这可能导致问题,因为前一个对象的 `aboutToBeInvalidated`
信号不会断开,可能导致崩溃或意外行为。此更改将常规的 `QObject::connect`
替换为 `safeConnect`,以确保正确断开连接并避免在表面替换时出现潜在问题。

Influence:
1. 使用 `setSurface` 多次用不同的 WSurface 对象替换 WSurface。
2. 验证 `releaseResources` 函数仅在当前 WSurface 真正失效时才被调用,而
不是过早调用。
3. 观察在大量 WSurface 替换场景下的应用程序行为,以确保稳定并防止崩溃。
@zccrs zccrs requested a review from wineee October 23, 2025 07:54
@sourcery-ai
Copy link

sourcery-ai bot commented Oct 23, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This patch updates the WSurfaceItem initialization to use the safeConnect helper for the aboutToBeInvalidated signal instead of QObject::connect, ensuring that previous signal connections are properly disconnected when the surface is replaced and avoiding potential crashes.

Sequence diagram for safe signal connection during WSurface replacement

sequenceDiagram
participant WSurfaceItem
participant WSurface
participant "releaseResources()"
WSurfaceItem->>WSurface: setSurface(newSurface)
WSurface->>WSurfaceItem: aboutToBeInvalidated
WSurfaceItem->>"releaseResources()": Call only for current WSurface
Note over WSurfaceItem,WSurface: safeConnect ensures old signals are disconnected
Loading

Class diagram for updated signal connection in WSurfaceItemPrivate

classDiagram
class WSurface {
  +aboutToBeInvalidated
  +hasSubsurfaceChanged
  +safeConnect()
}
class WSurfaceItem {
  +releaseResources()
  +onSurfaceCommit()
}
class WSurfaceItemPrivate {
  +initForSurface()
}
WSurfaceItemPrivate -- WSurfaceItem : q
WSurfaceItemPrivate -- WSurface : surface
WSurfaceItemPrivate : initForSurface() uses safeConnect for aboutToBeInvalidated
Loading

File-Level Changes

Change Details Files
Use safeConnect for aboutToBeInvalidated signal
  • Replaced QObject::connect with surface->safeConnect for aboutToBeInvalidated
  • Retained Qt::DirectConnection to preserve signal delivery semantics
waylib/src/server/qtquick/wsurfaceitem.cpp

Possibly linked issues

  • #N/A: The PR fixes an unsafe signal connection in WSurfaceItem's aboutToBeInvalidated signal, directly addressing the crash during SurfaceWrapper and WSurfaceItem destruction in the issue.
  • #N/A: The PR fixes an incorrect WSurface signal connection that could cause crashes related to surface invalidation, which might be the underlying cause of the Helper::activateSurface crash.
  • refactor: optimize input popup surface #28: The PR fixes a crash caused by an incorrect QObject::connect for WSurface's aboutToBeInvalidated signal, aligning with the issue's stack trace.

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

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wineee, zccrs

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

@zccrs zccrs merged commit 1866331 into linuxdeepin:master Oct 24, 2025
10 of 12 checks passed
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.

3 participants