All users were logged out of Bugzilla on October 13th, 2018

Add more assertions in TextureClient

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8371474 [details] [diff] [review]
Moar assertions
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 ^
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Updated

5 years ago
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+
Created attachment 8376344 [details] [diff] [review]
Add comments about the assertions on DrawTarget refcounts
Attachment #8376344 - Flags: review?(nical.bugzilla)
A conversation about this can be found in bug 972861.
(Assignee)

Updated

5 years ago
Attachment #8376344 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 10

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 973869
(Assignee)

Updated

5 years ago
Depends on: 973875
(Assignee)

Updated

5 years ago
Depends on: 969388
When you land, can you please land my comments patch together with it? Thanks!
(Assignee)

Comment 13

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.