Closed Bug 1043929 Opened 7 years ago Closed 7 years ago

CompositableHost::AddMaskEffect should unlock the texture in case of failure


(Core :: Graphics: Layers, defect)

Not set





(Reporter: nical, Assigned: nical)



(2 files)

If the TextureHost is locked but doesn't return a TextureSource (which is unlikely but nothing proves that it can't happen), we return without unlocking the TextureHost. It's important to always unlock locked textures to avoid getting into a weird state in the next frame.
This isn't likely to bite us but let's fix it now that I noticed it otherwise it'll be harder to debug if it happens some day.
As a second step we should go further and ensure that if lock succeeds, there will always be a valid TextureSource. It might already be implicitly the case, or very close to it so it's probably mostly about adding some assertions and documenting the API.
Attachment #8462554 - Flags: review?(bas)
There will be followups, but with that we can already catch most of the failure cases
Attachment #8462568 - Flags: review?(bas)
Comment on attachment 8462554 [details] [diff] [review]
make warnings more explicit about what's going on and unlock the host if needed.

Review of attachment 8462554 [details] [diff] [review]:

I'm not sure I see a lot of value in this but it also can't hurt I suppose.
Attachment #8462554 - Flags: review?(bas) → review+
Comment on attachment 8462568 [details] [diff] [review]
Ensure that a locked TextureHost yields a valid TextureSource for some of the texture types and add some assertions

Review of attachment 8462568 [details] [diff] [review]:

In general I think this a good improvement.

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +836,5 @@
>  void
>  TextureHostD3D9::Updated(const nsIntRegion* aRegion)
>  {
> +  MOZ_ASSERT(mTexture);

We should still have an early return here in addition to the assertion.
Attachment #8462568 - Flags: review?(bas) → review+
One of the patches requires the TextureHost to have a Compositor in order to Lock() successfully, which is a valid assumption but TestTextures.cpp tries to test the texture class in isolation so it had to be fixed (in the test).
You need to log in before you can comment on or make changes to this bug.