1918778 - Correct recycling issues with MacIOSurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

In theory, ::IOSurfaceLock can fail, and all calls to MacIOSurface::Lock should verify success:
https://searchfox.org/mozilla-central/rev/ab8043ccefe85c30ef78b97193918934d6aa93d1/gfx/2d/MacIOSurface.cpp#399

The allocation can fail, we have very low volume crashes on it, and we should check for it since we can gracefully fallback:
https://searchfox.org/mozilla-central/rev/181e5bb2645236a617d42e3740420098097f7a0f/gfx/layers/MacIOSurfaceImage.cpp#90

We need to verify the recycled surface matches the given parameters (aColorSpace, aColorRange, aColorDepth), and discard it if not:
https://searchfox.org/mozilla-central/rev/181e5bb2645236a617d42e3740420098097f7a0f/gfx/layers/MacIOSurfaceImage.cpp#274

The last one might be the cause of the high volume safeShift10BitBy6 crash.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1918778 - Correct ownership/recycling issues with MacIOSurface.

This patch corrects a few different issues related to recycling
MacIOSurface objects.

1. When recycling a surface, we must check that the cached surfaces
   match all of the requested parameters, not just the size. If we do
   not, we should just flush the whole cache immediately since they
   should all be created with the same parameters.

2. Allocations can fail, and we should check for failing to get a
   surface from the allocator and fall back if so.

3. Locking can fail, and we should check that return value at all of the
   call sites.

This may help resolve a number of otherwise difficult to understand
crash signatures. It may also solve display corruption issues in rare
cases where the parameters that changed were roughly equivalent such
that everything appears to work, but they differ enough to change the
presentation.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e91a137053f9
Correct ownership/recycling issues with MacIOSurface. r=bradwerth

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

I think we should uplift to 131 beta if nightly seems okay for a few days, but I think it is too much change for a dot release even if one were scheduled, particularly since it depends on changes in bug 1906527. 131 beta already contains the dependency. If it resolves the issue, we can uplift both to 128 esr.

Comment 4

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/e91a137053f9

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:aosmond, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox131 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9424826 [details]
Bug 1918778 - Correct ownership/recycling issues with MacIOSurface.

Beta/Release Uplift Approval Request

• User impact if declined: High volume streaming service crash on OSX only
• Is this code covered by automated tests?: Yes
• 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 patch mostly consists of enforcing invariant assumptions that we did not check before. The surface cache clearing, even we were somehow too aggressive, would at worst trade additional CPU/GPU usage for stability. The lock status was never checked, and as such, we would either crash or have unpredictable results before. The changes are limited to OSX only pathways.
• String changes made/needed:
• Is Android affected?: No

Comment 7

[Dianna Smith [:diannaS]]

Comment on attachment 9424826 [details]
Bug 1918778 - Correct ownership/recycling issues with MacIOSurface.

Approved for 131.0b8

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/56ad959da4a3

Comment 9

[Dianna Smith [:diannaS]]

:aosmond can you nominate this for esr128 when you get a chance?

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1918778 - Correct ownership/recycling issues with MacIOSurface.

This patch corrects a few different issues related to recycling
MacIOSurface objects.

1. When recycling a surface, we must check that the cached surfaces
   match all of the requested parameters, not just the size. If we do
   not, we should just flush the whole cache immediately since they
   should all be created with the same parameters.

2. Allocations can fail, and we should check for failing to get a
   surface from the allocator and fall back if so.

3. Locking can fail, and we should check that return value at all of the
   call sites.

This may help resolve a number of otherwise difficult to understand
crash signatures. It may also solve display corruption issues in rare
cases where the parameters that changed were roughly equivalent such
that everything appears to work, but they differ enough to change the
presentation.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/8eb3b3604c35