fix: keep tao window_target in main thread#15411
Conversation
Package Changes Through e4c2c60There are 3 changes which include tauri-utils with patch, tauri-bundler with patch, tauri with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
| &self, | ||
| ) -> std::result::Result<DisplayHandle<'_>, raw_window_handle::HandleError> { | ||
| self.context.main_thread.window_target.display_handle() | ||
| todo!(); |
There was a problem hiding this comment.
Don't know how can we deal with this life time issue though...
| proxy: event_loop.create_proxy(), | ||
| main_thread: DispatcherMainThreadContext { | ||
| window_target: event_loop.deref().clone(), | ||
| window_target: Arc::downgrade(&window_target), |
There was a problem hiding this comment.
Do we actually need Arc? Since keep in main thread isn't cross thread. Or something like run_on_main ?
There was a problem hiding this comment.
The thing here is this context can be cloned and moved to other threads, and the inner rc from tao can't be cloned/dropped from multiple threads
|
Took the branch for a spin on Windows ( fn display_handle(
&self,
) -> std::result::Result<DisplayHandle<'_>, raw_window_handle::HandleError> {
let window_target = self
.context
.main_thread
.window_target
.upgrade()
.ok_or(raw_window_handle::HandleError::Unavailable)?;
let raw = window_target.display_handle()?.as_raw();
// SAFETY: `raw` is a plain `Copy` `RawDisplayHandle` pointing at the event
// loop's display connection, which stays valid for as long as the runtime
// (and therefore `&self`) is alive.
Ok(unsafe { DisplayHandle::borrow_raw(raw) })
}The |
…vent-target-in-main-thread
|
Well, with my change, the lifetime of the That being said, even though the old lifetime was correct, it carried the |
|
You're right, and thanks for the catch. The Digging into what the So I think the real choice is binary:
I'd lean toward (1), scoped to this PR: it's the smallest sound change that keeps the public API working where it can, and the X11/Wayland off-thread path is the one this PR is already making safe, so degrading it to a clean Either way, scratch my earlier offer to push the |
|
I think the direction would be either try another solution for the implicit |
|
Thanks, agreed the weak pointer can't satisfy the rwh lifetime contract in general. The split I'd suggest: Windows and macOS return the empty So the actual decision is just that one arm:
I lean toward (2): it keeps Windows/macOS sound and serves Linux as before, rather than dropping the Linux off-thread path to |
|
Yeah, a small unsoundness would out weight breaking everyone using that API (your |
|
Here's what I'd push. Flagging the SAFETY note for your review since it's the load-bearing part. Good to commit? fn display_handle(&self) -> Result<DisplayHandle<'_>, HandleError> {
// Windows/macOS: the display handle carries no pointer, so borrowing
// one is sound for any lifetime regardless of the Weak event loop.
#[cfg(windows)]
return Ok(unsafe { DisplayHandle::borrow_raw(
RawDisplayHandle::Windows(WindowsDisplayHandle::new())) });
#[cfg(target_os = "macos")]
return Ok(unsafe { DisplayHandle::borrow_raw(
RawDisplayHandle::AppKit(AppKitDisplayHandle::new())) });
#[cfg(not(any(windows, target_os = "macos")))]
{
let window_target = self
.context
.main_thread
.window_target
.upgrade()
.ok_or(HandleError::Unavailable)?;
let raw = window_target.display_handle()?.as_raw();
// SAFETY: `raw` borrows the X11/Wayland connection pointer owned by
// the main-thread EventLoopWindowTarget. The Weak above can't keep
// that target alive, so this technically violates the rwh lifetime
// contract: callers must not retain or use this handle past a point
// where the main thread may have dropped the runtime. In practice
// this holds for typical surface-init consumers, which use the
// handle synchronously and don't outlive the runtime. Going via the
// Weak (rather than dropping to Err) preserves prior behavior on
// Linux, which was already de-facto relied on.
Ok(unsafe { DisplayHandle::borrow_raw(raw) })
}
}Plus a one-line restoration of the One thing I want to be straight about on the SAFETY note: "callers must not retain or use this handle past a point where the main thread may have dropped the runtime" is a precondition a wgpu/softbuffer/glutin caller can't actually verify on its own. That's why this arm is documented-unsound rather than sound; there's no formulation that closes the gap without restructuring the trait. The note acknowledges the gap rather than pretending to close it, which is the trade we settled on. |
|
Just so we're clear, I would like to hold onto this one see if we can come up with another method (not the weak pointer approach used here) before preceding with this unsound implemention |
|
I went looking for a non-weak method before adding docs, and the useful finding is that two axes are getting conflated: the drop fix and For the off-thread X11/Wayland arm specifically, there are two in-repo endings:
And two that are out of scope for this PR: a tao/winit refactor that splits the connection into a My recommendation is On the Win/Mac arm, either ending returns the empty handle while the runtime is alive. One behavior delta worth making intentional rather than incidental: routing Win/Mac through If you take |
3 participants
An attempt to fix #15408
By keeping an
ArcinWryand only keep anWeakinContext, we make sure we only ever upgrade/use/drop thewindow_targeton main thread. (previously, the clone/drop can be made in another thread from cloning and sending to another thread)