968807 - Add more assertions in TextureClient

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

SOme of the bugs we have had recently were there because some TextureClient backends are more permissive than others. Let's add as many assertions as we can to catch these problems early on.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Moar assertions

Comment 2

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

Comment on attachment 8371474 [details] [diff] [review]
Moar assertions

Review of attachment 8371474 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/TextureClient.cpp
@@ +578,5 @@
>      mUsingFallbackDrawTarget = false;
>      return;
>    }
>  
> +  MOZ_ASSERT(mDrawTarget.refcount() == 1);

Please explain that ^

Comment 3

[Nicolas Silva [:nical]]

It is a way to ensure that people don't keep the DrawTarget outside of the lock/unlock interval. After the texture is unlocked, the underlying memory that the draw target is pointing to is inaccessible. using the DrawTarget will, for instance, trigger a crash inside D2D on windows.

Comment 4

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

Sorry, I still don't understand. DrawTarget is reference counted. Surely that means that it is OK for multiple objects to hold a reference to one simultaneously, even if only one of them is going to actually lock and unlock the DrawTarget? In other words, I don't see how it is relevant that the underlying graphics buffer is inaccessible.

I also don't understand how this compiles: what is this all-lowercase refcount() method? The one "refcount" method that I know of, is RefCounted<DrawTarget>::refCount() <-- see the uppercase C.

Maybe that is the source of my confusion: maybe you are talking about a totally different kind of "refcount" than what I have in mind? I don't know DrawTarget well. I might be a bad reviewer for this patch.

Comment 5

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

Comment on attachment 8371474 [details] [diff] [review]
Moar assertions

Cancelling the review until I understand what's going on here; mDrawTarget.refcount() either fails to compile, or does something that I don't understand at all.

Comment 6

[Nicolas Silva [:nical]]

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Sorry, I still don't understand. DrawTarget is reference counted. Surely
> that means that it is OK for multiple objects to hold a reference to one
> simultaneously, even if only one of them is going to actually lock and
> unlock the DrawTarget? In other words, I don't see how it is relevant that
> the underlying graphics buffer is inaccessible.

It's a matter of how strict you want the API to be. I can assure you (after having untangled the ContentClient mess) that if you keep around the DrawTarget of a TextureClient after it is unlocked, you will most likely end up crashing somewhere in the drawing backend.

DrawTarget's reference counting makes sense when it owns its memory. In this case it is wrapping memory that it doesn't own so I think we should assert that it doesn't outlive this memory.

> 
> I also don't understand how this compiles: what is this all-lowercase
> refcount() method? The one "refcount" method that I know of, is
> RefCounted<DrawTarget>::refCount() <-- see the uppercase C.

Yeah, I had it fixed locally but I forgot to hg qrefresh before I uploaded the patch.

> 
> Maybe that is the source of my confusion: maybe you are talking about a
> totally different kind of "refcount" than what I have in mind? I don't know
> DrawTarget well. I might be a bad reviewer for this patch.

It is the RefPtr's refCount, and you are a good reviewer

Comment 7

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

Comment on attachment 8371474 [details] [diff] [review]
Moar assertions

Review of attachment 8371474 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/TextureClient.cpp
@@ +578,5 @@
>      mUsingFallbackDrawTarget = false;
>      return;
>    }
>  
> +  MOZ_ASSERT(mDrawTarget.refcount() == 1);

OK, had a skype call with :nical about that. This assert is needed because of details of moz2d and direct2d which I don't fully understand, but an identical assert is already present in TextureClientD3D11::Unlock, r=Bas, so, future people blaming this code, ask him about that.

Comment 8

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

Attachment: Add comments about the assertions on DrawTarget refcounts

Comment 9

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

A conversation about this can be found in bug 972861.

Comment 10

[Nicolas Silva [:nical]]

Since I wrote the patch, more code that breaks the assertions has landed so this can't land just yet.
A try push to evaluate the damage: https://tbpl.mozilla.org/?tree=Try&rev=9be111f5ce69

Comment 11

[Nicolas Silva [:nical]]

try push with 3 fixes: https://tbpl.mozilla.org/?tree=Try&rev=c21a1ee65de3

Comment 12

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

When you land, can you please land my comments patch together with it? Thanks!

Comment 13

[Nicolas Silva [:nical]]

This includes both my patch and Benoit's:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c9f249fb7a

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/54c9f249fb7a