Skip to content

Update winit_test and re-enable tests/present_and_fetch#179

Closed
ids1024 wants to merge 2 commits into
rust-windowing:masterfrom
ids1024:winit-test
Closed

Update winit_test and re-enable tests/present_and_fetch#179
ids1024 wants to merge 2 commits into
rust-windowing:masterfrom
ids1024:winit-test

Conversation

@ids1024

@ids1024 ids1024 commented Nov 14, 2023

Copy link
Copy Markdown
Member

No description provided.

@ids1024 ids1024 requested a review from notgull November 14, 2023 16:40
@ids1024 ids1024 requested a review from john01dav as a code owner November 14, 2023 16:40
@ids1024

ids1024 commented Nov 14, 2023

Copy link
Copy Markdown
Member Author

Looks like the test needs to be updated for WASM. The SIGSEGV on macOS is more concerning though.

@notgull

notgull commented Nov 14, 2023

Copy link
Copy Markdown
Collaborator

@madsmtm Any insight into this segfault?

@madsmtm

madsmtm commented Nov 15, 2023

Copy link
Copy Markdown
Member

Hmm, yeah, that is troubling, I'm looking into it.

@madsmtm

madsmtm commented Nov 15, 2023

Copy link
Copy Markdown
Member

Oh, I thought I had migrated softbuffer to use objc2 already, it would've prevented this issue...

The issue is fairly simple once you see it, both CALayer and the window reference are not retained (in part due to servo/core-foundation-rs#643), so I think the window ends up being double-freed (or free'd before the CALayer has a chance to release it's own reference to it).

@madsmtm madsmtm mentioned this pull request Nov 15, 2023
Merged
@madsmtm

madsmtm commented Nov 15, 2023

Copy link
Copy Markdown
Member

Fixed in #180

@notgull notgull requested a review from daxpedda November 15, 2023 03:20
@daxpedda

daxpedda commented Nov 15, 2023

Copy link
Copy Markdown
Member

This is unfortunately not going to work on Wasm, because Window::inner_size() will always return None on Window creation. You have to wait for a Resized event for that to work properly.

I guess this is an issue in winit-test, see notgull/winit-test#11.

daxpedda
daxpedda reviewed Nov 15, 2023
View reviewed changes
Comment thread tests/present_and_fetch.rs

// winit does not wait for the window to be mapped... sigh
#[cfg(not(target_arch = "wasm32"))]
std::thread::sleep(std::time::Duration::from_millis(1));

@daxpedda daxpedda Nov 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just leaving this here as another argument for notgull/winit-test#11: my system needed longer to work correctly.

@notgull

notgull commented May 7, 2024

Copy link
Copy Markdown
Collaborator

The test was removed as part of #214, so I'll close this PR. We will revisit this in the future.

@notgull notgull closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxpedda daxpedda daxpedda left review comments

@notgull notgull Awaiting requested review from notgull

@john01dav john01dav Awaiting requested review from john01dav

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ids1024 @notgull @madsmtm @daxpedda