In some cases ImageHost may lock a texture hosts without unlocking it. Let's always use RAII to make sure that doesn't happen.

There was already a RAII helper in ContentHost.cpp, let's use move it to TextureHost.cpp and use it with all CompositableHosts.

woops

some fixes

Comment on attachment 8349652 [details] [diff] [review] Use RAII to unlock TextureHost :( This changed semantics and causes an assertion failure. >-class MOZ_STACK_CLASS AutoLockTextureHost >-{ >-public: >- AutoLockTextureHost(TextureHost* aHost) >- : mHost(aHost) >- { >- mLockSuccess = mHost ? mHost->Lock() : true; Note that the old code handled mHost == nullptr, by just saying success = true. >- if (!mTextureHost || >- !lock.IsValid() || >- !lockOnWhite.IsValid()) { >+ if (lock.Failed() || lockOnWhite.Failed()) { so this fails if mTextureHostOnWhite is nullptr, because the lock will have Failed(). I don't know if semantically we should treat a lock of a null texture as success, though.

This fixes the failed assertion for me

Comment on attachment 8350465 [details] [diff] [review] additional fixup Review of attachment 8350465 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/TextureHost.h @@ +786,4 @@ > AutoLockTextureHost(TextureHost* aTexture) > : mTexture(aTexture) > { > + mLocked = aTexture ? aTexture->Lock() : true; It's now too hard to see how the logic here deciding if we lock, matches the one in the destructor. At least, one problem is that mLocked is a lie if mTexture is null.

Doesn't matter, IMO. mLocked is an internal implementation detail. The external interface to the RAII helper only has a "Failed()" method.. which properly returns "didn't fail" if you try to lock a null texture. If you'd want it to match the destructor, it can be written as if (aTexture) { mLocked = aTexture->Lock(); } else { mLocked = true; // null textures are OK } But I don't think that buys much other than vertical space.

You can get r+ here either by renaming the internal mLocked variable to a name that reflects what it is, or by changing the logic --- your choice, but we shouldn't have variables, even internal, named mLocked, that can be true even if no texture was locked.