Closed Bug 904343 Opened 8 years ago Closed 8 years ago

We never clear our surface descriptors in shmem texture clients

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: nrc, Assigned: nrc)

References

Details

Attachments

(2 files)

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?
Attached patch patchSplinter Review
Attachment #789336 - Flags: review?(nical.bugzilla)
Attachment #789336 - Flags: feedback?(bjacob)
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 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.
Attachment #789336 - Flags: review?(nical.bugzilla) → review+
Duplicate of this bug: 904511
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).
Attachment #789336 - Flags: feedback?(bjacob) → feedback+
Duplicate of this bug: 905266
Confirmed: attachment 789336 [details] [diff] [review] fixes the lockscreen crash on Inari.
Blocks: 904658
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
https://hg.mozilla.org/mozilla-central/rev/2b003d033fa4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Duplicate of this bug: 904658
Duplicate of this bug: 904817
You need to log in before you can comment on or make changes to this bug.