904343 - We never clear our surface descriptors in shmem texture clients

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

ContentClientDoubleBuffered::~ContentClientDoubleBuffered() calls SetDescriptor(SurfaceDescriptor()) as the standard way to blank out a SurfaceDescriptor.
Then in DeprecatedTextureClientShmem::SetDescriptor we check IsSurfaceDescriptorValid
which will be no, because it is the empty Descriptor (T__None) and then we will not unset mDescriptor. Which seems bad. Am I missing something?

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

Comment 2

[Nick Cameron [:nrc]]

Attachment: or maybe this version?

this version does the old thing for null surface descriptors and resets for None ones.

Note that all the null checking on surfaces is not really related to the important thing here which is the destruction sequence for content clients via SetDescriptor.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 789336 [details] [diff] [review]
patch

Review of attachment 789336 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane to me. The thing is that DeprecatedTextureClient feels like it's on an awkward equilibrium because it doesn't have the same semantic on ImageLayers and ThebesLayers. So changing something for one may break the other.
Please make sure you have tested this patch with a video (preferably with async-video) before landing.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 789336 [details] [diff] [review]
patch

Review of attachment 789336 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know / remember much about the code around here, I can just say that nothing here conflicts with whatever little I know.

As Nical says, the old TextureClient stuff is very hard to reason about, so make sure you've tested both ContentClients (some web browsing) and ImageClients (use the camera app).

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Confirmed: attachment 789336 [details] [diff] [review] fixes the lockscreen crash on Inari.

Comment 8

[Nick Cameron [:nrc]]

Tested on Android and b2g (and Windows) - images, video, and camera. Hope I covered all the bases.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b003d033fa4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b003d033fa4