951218 - Use RAII to unlock TextureHosts

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

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

Comment 1

[Nicolas Silva [:nical]]

Attachment: Use RAII to unlock TextureHost

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

Comment 2

[Nicolas Silva [:nical]]

Attachment: Use RAII to unlock TextureHost

woops

Comment 3

[Nicolas Silva [:nical]]

Attachment: Use RAII to unlock TextureHost

some fixes

Comment 4

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78057fe625dc

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/78057fe625dc

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: additional fixup

This fixes the failed assertion for me

Comment 8

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

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.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 10

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

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.

Comment 11

[Nicolas Silva [:nical]]

Attachment: additional fixup (bjacob-proof version)

Comment 12

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90b70511ded6

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/90b70511ded6