Comment on attachment 9251475 [details]
Bug 1741997 - DMABufSurface::FenceDelete should not require GL context to close mSyncFd handle.
Beta/Release Uplift Approval Request
- User impact if declined: Users with EGL and DMABuf enabled (all Wayland and X11+EGL users which the majority likely get with the release of 94) will experience file handle leaks when background tabs have WebGL canvas instances. Eventually they will run out and the process will crash.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The code would be covered by automated tests except for the fact that our Mesa version for our Linux instances is too old and so we don't take this path. The CI team is aware that we need a newer image for Linux to test various features (distros had already chosen of their own accord to ship Wayland in newer releases for example).
I don't believe there is any meaningful risk to accepting this patch. I changed the order of releasing some resources to ensure the file handle in question is always closed. There is no dependency between them as they are completely independent. The leak only happens when the resource was not used for rendering (hence why the tab needs to be backgrounded), and thus the buffer state is very simple -- it has some file handles that need to be closed and that's about it. We only release the resources when we are done with the buffer.
The alternative for users is to disable WebGL + DMABuf via a pref change (set
false). This will cause us to fall back to reading the pixels out of the WebGL canvas into a shared memory buffer. Performance is not as good, but we lived with it up until 94. I am not sure how many users were testing EGL without WebGL + DMABuf however since EGL more or less implies DMABuf for the users we are shipping to, and there is a runtime check to see if DMABuf is supported (we don't have any telemetry on this). There is no reason to believe it will cause problems however since that is our fallback path for all platforms.
- String changes made/needed: