Skip to content

refactor(modules): drop redundant wl_resource type checks - #1451

Merged
wineee merged 1 commit into
linuxdeepin:masterfrom
deepin-wm:agent/developer/b72c7f2fcdc2
Sep 29, 2026
Merged

wineee merged 1 commit into
linuxdeepin:masterfrom
deepin-wm:agent/developer/b72c7f2fcdc2

Conversation

@deepin-wm

@deepin-wm deepin-wm commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

libwayland already validates object arguments before a request handler runs:

  • wl_connection_demarshal() rejects id 0 for non-nullable object arguments.
  • wl_closure_lookup_objects() rejects unknown object ids and type mismatches with WL_DISPLAY_ERROR_INVALID_METHOD; the handler is never invoked on error. Server-side destroyed resources are removed from the client object map, so their ids fail lookup as well.

Hence in the touched request handlers the object arguments are always non-NULL and of the protocol-declared type; the self-made strcmp(wl_resource_get_class(...)) checks were unreachable dead code. Remove them and the now-unused <cstring>/<string.h> includes.

Real checks are kept: cross-client ownership (decoration), inert seat (wlr_seat_client_from_resource), removed output (WOutput::fromHandle NULL), missing WSurface wrapper, and the protocol size/anchor enum validations. The capture module / waylib Q_ASSERT(wl_resource_instance_of(...)) debug asserts (mirroring wlroots' own wlr_*_from_resource style) are intentionally left untouched.

变更说明

删除四个模块请求处理函数中自行做的 wl_resource 类型检查:libwayland 分发层已在校验对象参数的类型与有效性(wl_closure_lookup_objects 对未知 id / 类型不匹配报 WL_DISPLAY_ERROR_INVALID_METHOD,不可空参数不会为 NULL,已销毁 resource 的 id 在服务端对象表中查不到),这些检查不可达。保留跨 client 归属检查、inert resource 判空与协议错误校验。

Files changed (+2/−42)

  • src/modules/decoration/decorationmanagerinterfacev1.cpp
  • src/modules/active-notify/activenotifymanagerinterfacev1.cpp
  • src/modules/region-watch/regionwatchmanagerinterfacev1.cpp
  • src/modules/xwindow-control/xwindowcontrolinterfacev1.cpp

Verification

Full build passes on the build machine: cmake --preset=default && cmake --build build -j50 (1915/1915 targets).

Summary by Sourcery

Rely on libwayland’s request dispatch validation instead of duplicating unreachable resource checks in module handlers.

Enhancements:

  • Remove redundant Wayland resource type and null checks from four module request handlers, relying on libwayland’s dispatch validation while retaining meaningful ownership, lifecycle, wrapper, and protocol validation.

Chores:

  • Remove unused C string headers from the affected modules.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@sourcery-ai

sourcery-ai Bot commented Sep 29, 2026 •

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

Reviewer's Guide

The handlers now rely on libwayland to reject null, unknown, destroyed, or incorrectly typed object arguments before invocation, eliminating duplicated resource checks while retaining checks for client ownership, resource state, wrapper availability, and protocol-specific validation.

Sequence diagram for libwayland request object validation

sequenceDiagram
    participant Client
    participant Wayland as libwayland
    participant Handler as RequestHandler
    participant Resource as ResourceAPI

    Client->>Wayland: request with object id
    Wayland->>Wayland: wl_connection_demarshal()
    alt invalid object id or type
        Wayland-->>Client: WL_DISPLAY_ERROR_INVALID_METHOD
    else valid protocol object
        Wayland->>Handler: invoke request handler
        Handler->>Resource: perform ownership/state/wrapper checks
        Resource-->>Handler: validation result
    end
Loading

Flow diagram for simplified object request handling

flowchart TD
    A[Wayland request received] --> B{libwayland object validation}
    B -->|invalid, null, unknown, destroyed, or wrong type| C[Reject before handler]
    B -->|valid protocol-declared object| D[Invoke module handler]
    D --> E{Meaningful module checks}
    E -->|ownership, resource state, wrapper, or protocol validation fails| F[Post protocol error]
    E -->|valid| G[Perform requested operation]
Loading

File-Level Changes

Change Details Files
Remove redundant Wayland resource class and null checks from request handlers, relying on libwayland dispatch validation.
  • Delete unreachable checks for seat, surface, output, and anchor resource types.
  • Remove now-unused C string comparison headers.
  • Preserve meaningful ownership, inert-resource, wrapper, and protocol validation paths.
src/modules/active-notify/activenotifymanagerinterfacev1.cpp
src/modules/decoration/decorationmanagerinterfacev1.cpp
src/modules/region-watch/regionwatchmanagerinterfacev1.cpp
src/modules/xwindow-control/xwindowcontrolinterfacev1.cpp

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 reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@deepin-wm
deepin-wm force-pushed the agent/developer/b72c7f2fcdc2 branch from 8e7a52b to f09db59 Compare September 29, 2026 09:08
@deepin-wm

Copy link
Copy Markdown
Contributor Author

recheck

@deepin-wm
deepin-wm force-pushed the agent/developer/b72c7f2fcdc2 branch 2 times, most recently from e754fd7 to 9ec8e45 Compare September 29, 2026 09:52
@deepin-wm
deepin-wm marked this pull request as draft September 29, 2026 09:52
@deepin-wm
deepin-wm marked this pull request as ready for review September 29, 2026 09:56

@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 reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-wm, wineee

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

1. Remove the unreachable strcmp(wl_resource_get_class()) checks in the
   decoration, active-notify, region-watch and xwindow-control request
   handlers.
2. libwayland already validates object arguments before a handler runs:
   non-nullable object args are never NULL, and unknown ids or type
   mismatches fail dispatch with WL_DISPLAY_ERROR_INVALID_METHOD.
3. Drop the now-unused <cstring>/<string.h> includes; keep the real
   checks (cross-client ownership, inert resources, missing wrappers).

Log: Remove redundant wl_resource type checks already done by libwayland dispatch

Influence:
1. Build treeland and confirm it compiles and links.
2. Start treeland, run a client that drives decoration, active-notify,
   region-watch and xwindow-control requests, and confirm normal behavior.
3. Send a request with an invalid object id from a client and verify the
   compositor reports a protocol error instead of crashing.

refactor(modules): 清理冗余的 wl_resource 类型检查

1. 删除 decoration、active-notify、region-watch、xwindow-control 请求处理函数中不可达的 strcmp(wl_resource_get_class()) 类型检查。
2. libwayland 分发层在调用处理函数前已完成校验:不可空对象参数不会为 NULL,未知对象 id 或类型不匹配会以 WL_DISPLAY_ERROR_INVALID_METHOD 分发失败。
3. 顺带删除不再使用的 <cstring>/<string.h> 头文件;保留跨客户端归属、inert resource 判空与缺失包装对象等必要检查。

Log: 清理请求处理函数中冗余的 wl_resource 类型检查,libwayland 分发层已校验对象类型

Influence:
1. 编译 treeland 确认通过。
2. 启动 treeland,运行客户端遍历 decoration、active-notify、region-watch、xwindow-control 相关请求,确认功能正常。
3. 客户端传入非法对象 id 时,确认合成器返回协议错误而非崩溃。

PMS: TASK-390751
Multica Issue: WM-558
@deepin-wm
deepin-wm force-pushed the agent/developer/b72c7f2fcdc2 branch from 9ec8e45 to 96b5f05 Compare September 29, 2026 11:28
@wineee
wineee merged commit 1773338 into linuxdeepin:master Sep 29, 2026
7 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.

4 participants