Skip to content

fix(windows): run new_window_req_handler on main thread#1660

Open
Legend-Master wants to merge 5 commits intotauri-apps:devfrom
Legend-Master:new-window-handler-no-send
Open

fix(windows): run new_window_req_handler on main thread#1660
Legend-Master wants to merge 5 commits intotauri-apps:devfrom
Legend-Master:new-window-handler-no-send

Conversation

@Legend-Master
Copy link
Contributor

@Legend-Master Legend-Master commented Jan 31, 2026

Implement the solution in #1657 (comment), closes #1657

Run new_window_req_handler on main thread and remove the Send + Sync restriction on it

Also fixed:

  • NewWindowResponse::Create not working if no custom protocol's registered
  • tauri-runtime-wry's usage of RefCell on another thread pointed out by @sftse in Redraw Sync bounds tauri#14805

@Legend-Master Legend-Master requested a review from a team as a code owner January 31, 2026 08:50
@Legend-Master Legend-Master changed the title New window handler no send fix(windows): run new_window_req_handler on main thread Jan 31, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2026

Package Changes Through dc93dcc

There are 1 changes which include wry with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
wry 0.54.1 0.55.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@sftse
Copy link
Contributor

sftse commented Jan 31, 2026

Thanks for taking this on.

Because the field is pub, the change is breaking.

pub struct Foo {
    pub f: Box<dyn Fn() + Send + Sync>, // remove Send + Sync breaks the code
}

fn main() {
    let f = move || {};
    let foo = Foo { f: Box::new(f) };
    std::thread::scope(|s| {
        s.spawn(|| &foo);
    });
}

Comment on lines 1138 to 1144
/// Send `function` to run on `hwnd`'s thread
///
/// ## SAFETY:
///
/// This function doesn't force a `Send` to make it easier to use,
/// the caller must call this function on the same thread as `hwnd`
/// or ensure the function is safe to `Send`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough about the code to judge this, but I just briefly sanity-checked whether the function passed is ever Send.
All the call sites pass a function that is not Send, so it might be sensible to drop the second precondition even if it were correct. Judging a more complex precondition is more difficult than a simple one. It also seems like a precondition that would warrant a comment referencing external documentation on PostMessageW, since this is the only unsafe function called in this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what's the better words here 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be incorrect or incomplete? The closures passed are already not Send.

Suggested change
/// Send `function` to run on `hwnd`'s thread
///
/// ## SAFETY:
///
/// This function doesn't force a `Send` to make it easier to use,
/// the caller must call this function on the same thread as `hwnd`
/// or ensure the function is safe to `Send`
/// Send `function` to run on `hwnd`'s thread
///
/// ## SAFETY:
///
/// The caller must call this function on the same thread as `hwnd`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite tricky here, our custom protocol handler is !Send but marked with unsafe impl Send to send to another thread for async tasks, and then send back and called through dispatch_handler, so technically, it is only accessed on the hwnd's thread but it is not called on hwnd's thread

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.

2 participants