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

Device drop lost #4835

Closed
wants to merge 5 commits into from
Closed

Device drop lost #4835

wants to merge 5 commits into from

Conversation

bradwerth
Copy link
Contributor

@bradwerth bradwerth commented Dec 5, 2023

Connections
N/A

Description
A User agent may reasonably assume that any supplied DeviceLostCallbacks is invoked eventually on a device. For example, the user agent may hold resources waiting for a callback, and if it is never invoked, the user agent will leak that memory. Triggering "lose the device" during device.drop ensures that the callback is called in all cases.

Testing
Added new test DEVICE_DROP_THEN_LOST.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@bradwerth bradwerth requested a review from a team as a code owner December 5, 2023 22:08
@ErichDonGubler
Copy link
Member

NV12 failures are just #4825, but the SEGV for the multithreaded_compute seems to be a regression in these changes.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

I don't think this is correct as it stands.

It's perfectly valid, say, to let go of your handle to the device before you're done using it. There's not a ton you can do, but you can still submit to the queue, map buffers, etc all after the user handle to the user device handle has died.

In general, I'm not sure we actually need to call the device loss callback on any kind of device drop (we need to call the C callback so the C callback can clean itself up, but in rust land you don't need that.

@bradwerth
Copy link
Contributor Author

I don't think this is correct as it stands.

It's perfectly valid, say, to let go of your handle to the device before you're done using it. There's not a ton you can do, but you can still submit to the queue, map buffers, etc all after the user handle to the user device handle has died.

Good point. I guess this is on embedders to ensure that they don't rely on the callback being called.

@bradwerth bradwerth closed this Dec 6, 2023
@jimblandy
Copy link
Member

Firefox does need to know when the callback gets dropped without being used. Our C callback owns data that needs to be freed. In discussion this morning, we thought it might serve if DeviceLostClosure had a Drop implementation that checked if inner was a DeviceLostClosure::C, and then invoked the callback passing DeviceLostReason::Destroyed.

The Rust callbacks can just have the closure own values that implement Drop, so they don't need any special handling. Although it might be more consistent to invoke the callback in that case as well.

@bradwerth
Copy link
Contributor Author

I've revived this approach in #4862, more narrowly-tailored to only affect the behavior of DeviceLostClosureC.

@bradwerth bradwerth deleted the deviceDropLost branch January 16, 2024 22:09
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.

4 participants