Closed Bug 968807 Opened 7 years ago Closed 6 years ago

Add more assertions in TextureClient

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(2 files)

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.
Attached patch Moar assertionsSplinter Review
Assignee: nobody → nical.bugzilla
Attachment #8371474 - Flags: review?(bjacob)
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 ^
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.
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 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.
Attachment #8371474 - Flags: review?(bjacob)
(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
Attachment #8371474 - Flags: review?(bjacob)
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.
Attachment #8371474 - Flags: review?(bjacob) → review+
A conversation about this can be found in bug 972861.
Attachment #8376344 - Flags: review?(nical.bugzilla) → review+
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
Depends on: 973869
Depends on: 973875
Depends on: 969388
When you land, can you please land my comments patch together with it? Thanks!
This includes both my patch and Benoit's:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c9f249fb7a
https://hg.mozilla.org/mozilla-central/rev/54c9f249fb7a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.