1691475 - Shared surfaces created in the compositor process can be freed too early

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

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

When a shared surface is created and lives in the compositor process, we actually free it differently from shared surfaces created in the content process:

https://searchfox.org/mozilla-central/rev/7067bbd8194f4346ec59d77c33cd88f06763e090/gfx/layers/ipc/SharedSurfacesChild.cpp#426

Since we don't post the release to the compositor thread, we don't have any ordering guarantees with the resource updates that we get when we use IPDL to send the message.

Comment 1

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

Attachment: Bug 1691475 - Remove shared surfaces on the compositor thread.

We add a shared surface on the main thread to the shared surfaces table
when in the compositor process because it uses a different wrapper
object if the shared surface is in a different process. This structure
was mirrored for the removal of a shared surface created in the
compositor process, however that created a race between the main thread
freeing the surface before the display list creation bound an image key
to the shared surface's external image ID. As such, this patch makes
removal always post to the compositor thread, whether in the compositor
process or not.

Comment 2

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

This variant of the crash can only happen if you don't have a GPU process.

Comment 3

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

Comment on attachment 9201832 [details]
Bug 1691475 - Remove shared surfaces on the compositor thread.

Beta/Release Uplift Approval Request

• User impact if declined: May crash if using WebRender
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Verifying in nightly is difficult since the crash rate is too low. However we see this crash as an intermittent in CI. I confirmed via logs this was the problem, and cannot reproduce the intermittent with the fix. The fix is pretty small and clearly solves solves a data race.
• String changes made/needed:

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64a26b4a51ae
Remove shared surfaces on the compositor thread. r=jrmuizel

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/64a26b4a51ae

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9201832 [details]
Bug 1691475 - Remove shared surfaces on the compositor thread.

We don't have a lot of crashes on desktop but we have more crash volume for this signature on Fenix. Approved for our next 86 beta, thanks.

Comment 7

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a5a30f4e77f