Skip to content

Redraw Sync bounds#14805

Open
sftse wants to merge 4 commits intotauri-apps:devfrom
sftse:fix-unsound
Open

Redraw Sync bounds#14805
sftse wants to merge 4 commits intotauri-apps:devfrom
sftse:fix-unsound

Conversation

@sftse
Copy link
Contributor

@sftse sftse commented Jan 21, 2026

Fixes #14801

Depends on wry PR 1657.

I'm not intimately familiar with what is going on in this code, so I'm using heuristics to guide my fix:

  • most of the arguments passed to fn on_event from the Plugin trait are not Sync
  • WindowsStore is sandwiched between two types that are both not Sync, it contains types that are not Sync and it itself is contained in
unsafe impl<T: UserEvent> Sync for DispatcherMainThreadContext<T> {}

pub struct DispatcherMainThreadContext<T: UserEvent> {
  pub window_target: EventLoopWindowTarget<Message<T>>,
  pub web_context: WebContextStore,
  // changing this to an Rc will cause frequent app crashes.
  pub windows: Arc<WindowsStore>,
  #[cfg(feature = "tracing")]
  pub active_tracing_spans: ActiveTraceSpanStore,
}

and

pub struct EventLoopIterationContext<'a, T: UserEvent> {
  pub callback: &'a mut (dyn FnMut(RunEvent<T>) + 'static),
  pub window_id_map: WindowIdStore,
  pub windows: Arc<WindowsStore>,
  #[cfg(feature = "tracing")]
  pub active_tracing_spans: ActiveTraceSpanStore,
}

of which the last is not Sync.

For basic soundness based on the examples in #14801 if a non-Sync field is public in a public struct that struct must be not Sync as well.
We have two choices, make both these structs that contain public WindowsStores Sync or make them both not Sync. Attempting to make both of these structs Sync is a rat's nest, so let's take the alternative, let's assume they must be both not Sync. This implies the Sync impl of both WindowsStore and DispatcherMainThreadContext are mistaken.

Removing the Sync impl of DispatcherMainThreadContext leads to another rat's nest of errors. Context used to inherit Sync based on its fields, so removing Sync from DispatcherMainThreadContext makes it not Sync as well.

I can't reason whether Context should or should not be Sync, but in any case we are overriding Sync "at the edges" of the code, so there is a larger space in which the absence of Sync will be caught by the compiler.

In summary, these changes strictly reduce the ways to be unsound. More cases are caught that were previously not sound.

@sftse sftse requested a review from a team as a code owner January 21, 2026 20:32
@github-project-automation github-project-automation bot moved this to 📬Proposal in Roadmap Jan 21, 2026
@sftse
Copy link
Contributor Author

sftse commented Jan 21, 2026

Because of

unsafe impl<T: UserEvent> Send for DispatcherMainThreadContext<T> {}

#[derive(Clone)]
pub struct DispatcherMainThreadContext<T: UserEvent> {
  pub window_target: EventLoopWindowTarget<Message<T>>,
  pub web_context: WebContextStore,
  // changing this to an Rc will cause frequent app crashes.
  pub windows: Arc<WindowsStore>,
  #[cfg(feature = "tracing")]
  pub active_tracing_spans: ActiveTraceSpanStore,
}

there's another change necessary, originally missed.
Arc<T> is only Send if T: Send, but the Send on DispatcherMainThreadContext allows an unsound bypass:
Clone DispatcherMainThreadContext -> send clone to another thread -> take inner Arc<WindowsStore> -> access WindowsStore unsynchronized from both threads. So we have to do the same fix as with Sync and remove the Send impl, delay it until Context.

Maybe it is still unsound that Context is Send and Sync, but the scope for unsound mistakes is reduced.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Package Changes Through db99474

There are 9 changes which include tauri with minor, @tauri-apps/cli with minor, tauri-cli with minor, tauri-utils with patch, tauri-build with patch, tauri-macos-sign with patch, tauri-bundler with minor, tauri-runtime-wry with minor, tauri-runtime 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
tauri-utils 2.8.1 2.8.2
tauri-macos-sign 2.3.2 2.3.3
tauri-bundler 2.7.5 2.8.0
tauri-runtime 2.9.2 2.10.0
tauri-runtime-wry 2.9.3 2.10.0
tauri-codegen 2.5.2 2.5.3
tauri-macros 2.5.2 2.5.3
tauri-plugin 2.5.2 2.5.3
tauri-build 2.5.3 2.5.4
tauri 2.9.5 2.10.0
@tauri-apps/cli 2.9.6 2.10.0
tauri-cli 2.9.6 2.10.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 Author

sftse commented Jan 22, 2026

As the broken CI on wry PR 1657 shows, that fix was a bit optimistic.

I don't know when I can further look into this, but I've pushed a fix that pulls the unsafety at least to the internals of Tauri instead of exposing it publically. Maybe this is enough of an improvement to be merged?

Summarizing, tauri currently exposes unsoundness in its public api, we might be interested in merging something that breaks as little as possible to fix this, this is such a minimal suggestion.

The fact that wry does end up needing Send here for the request handler is a bit rough though. Somebody with a better grasp of the code should probably be looking into this.

          std::thread::spawn(move || match new_window_req_handler(uri, features) { ... });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📬Proposal

Development

Successfully merging this pull request may close these issues.

[bug] Unsound Sync implementation

1 participant