Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unsound SendSyncWrapper #3303

Merged
merged 2 commits into from
Dec 26, 2023

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Dec 24, 2023

I noticed that SendSyncWrapper caused some real issues here and there. First #3270 and #3288, then #3297.

Also noticed that we simply used SendSyncWrapper on Fullscreen inside WindowBuilder, but there is a getter that exposes this information to the user, bypassing the !Sync on Fullscreen on MacOS, depending if this !Sync was correct or not, that may have been unsound.

Additionally on Web there was an issue where dropping WindowBuilder outside the main thread could cause it to try and drop HtmlCanvasElement, which is unsound as well (not entirely sure if that is correct, but it's at least semantically correct).

Additionally implementing Debug on SendSyncWrapper was unsound as well, because Debug could access the inner value off the only thread it was intended to be accessed on. Which was the case for WindowBuilder on Web with HtmlCanvasElement and the custom wrapper I used for CustomCursor.

All in all I would say SendSyncWrapper is a footgun and apart from the RawWindowHandle v0.4 and v0.5 we didn't even really need it.

@kchibisov
Copy link
Member

Additionally implementing Debug on SendSyncWrapper was unsound as well, because Debug could access the inner value off the only thread it was intended to be accessed on. Which was the case for WindowBuilder on Web with HtmlCanvasElement and the custom wrapper I used for CustomCursor.

reading pointer value is not unsound by any means, if you deref raw pointers during Debug it's already concerning.

Also noticed that we simply used SendSyncWrapper on Fullscreen inside WindowBuilder, but there is a getter that exposes this information to the user, bypassing the !Sync on Fullscreen on MacOS, depending if this !Sync was correct or not, that may have been unsound.

you can't do anything with it outside the main thread, and if you can do then builder has nothing to do with it, we have the same API throughout the code base on a Send + Sync types with the exact same values.

@daxpedda
Copy link
Member Author

daxpedda commented Dec 24, 2023

reading pointer value is not unsound by any means, if you deref raw pointers during Debug it's already concerning.

I think you are talking about RawWindowHandle. HtmlCanvasElement is not a pointer and neither is Fullscreen.
But that is the footgun I was talking about, we were using SendSyncWrapper on types that aren't a pointer.

you can't do anything with it outside the main thread, and if you can do then builder has nothing to do with it, we have the same API throughout the code base on a Send + Sync types with the exact same values.

I think there is a misunderstanding here.
WindowBuilder is Send + Sync, the public API allows you to extract the Fullscreen type from the WindowBuilder and access it. Now the user can do this over multiple threads at the same time, because WindowBuilder is Send + Sync, and Fullscreen grants access to a VideoMode, which has a bunch of methods that users could call on a type in parallel that isn't Sync.

If this isn't an issue, then VideoMode should be Sync, making Fullscreen Sync automatically.

@kchibisov
Copy link
Member

I think there is a misunderstanding here.
WindowBuilder is Send + Sync, the public API allows you to extract the Fullscreen type from the WindowBuilder and access it. Now the user can do this over multiple threads at the same time, because WindowBuilder is Send + Sync, and Fullscreen grants access to a VideoMode, which has a bunch of methods that users could call on a type in parallel that isn't Sync.

If this isn't an issue, then VideoMode should be Sync, making Fullscreen Sync automatically.

Window is Send + Sync as well and uses the same API, if macOS is bugged macOS should be fixed, nothing.

@daxpedda
Copy link
Member Author

Window is Send + Sync as well and uses the same API, if macOS is bugged macOS should be fixed, nothing.

This isn't the same thing, Window doesn't return references to Fullscreen, but owned values, the resulting Fullscreen is then !Sync, so no violations there.

I hope the problem is apparent now, SendSynWrapper is just a footgun because it's not simple to reason about these things, especially when we have so many very different backends.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Flyby review, unsure what's the correct approach here.

src/platform_impl/macos/window.rs Outdated Show resolved Hide resolved
/// which is `unsafe`.
#[derive(Debug, Clone)]
#[cfg(feature = "rwh_06")]
pub(crate) struct SendSyncRawWindowHandle(pub(crate) rwh_06::RawWindowHandle);
Copy link
Member

Choose a reason for hiding this comment

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

See also #3142 for improvements here.

@daxpedda
Copy link
Member Author

I split out the Web part into #3320.
So now all that needs to be done here is figure out Sync for VideoMode for MacOS.

@daxpedda daxpedda added B - bug Dang, that shouldn't have happened and removed DS - web labels Dec 26, 2023
@daxpedda daxpedda merged commit 34e42ff into rust-windowing:master Dec 26, 2023
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - macos
Development

Successfully merging this pull request may close these issues.

None yet

3 participants