Use RAII to unlock TextureHosts

RESOLVED FIXED in mozilla29

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla29
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8348815 [details] [diff] [review]
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.
Attachment #8348815 - Flags: review?(bjacob)
(Assignee)

Comment 2

4 years ago
Created attachment 8348829 [details] [diff] [review]
Use RAII to unlock TextureHost

woops
Attachment #8348815 - Attachment is obsolete: true
Attachment #8348815 - Flags: review?(bjacob)
Attachment #8348829 - Flags: review?(bjacob)
(Assignee)

Comment 3

4 years ago
Created attachment 8349652 [details] [diff] [review]
Use RAII to unlock TextureHost

some fixes
Attachment #8348829 - Attachment is obsolete: true
Attachment #8348829 - Flags: review?(bjacob)
Attachment #8349652 - Flags: review?(bjacob)
Attachment #8349652 - Flags: review?(bjacob) → review+
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/78057fe625dc
https://hg.mozilla.org/mozilla-central/rev/78057fe625dc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
Attachment #8349652 - Flags: review-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8350465 [details] [diff] [review]
additional fixup

This fixes the failed assertion for me
Attachment #8350465 - Flags: review?(nical.bugzilla)
Attachment #8350465 - Flags: review?(bjacob)
(Assignee)

Updated

4 years ago
Attachment #8350465 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8350465 - Flags: review?(bjacob) → review-
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.
(Assignee)

Comment 11

4 years ago
Created attachment 8350782 [details] [diff] [review]
additional fixup (bjacob-proof version)
Attachment #8350465 - Attachment is obsolete: true
Attachment #8350782 - Flags: review?(bjacob)
Attachment #8350782 - Flags: review?(bjacob) → review+
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b70511ded6
https://hg.mozilla.org/mozilla-central/rev/90b70511ded6
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.