912040 - TextureClient can race with the deallocation of its shared data if RemoveTextureClient is invoked manually

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

CompositableClient::RemoveTexture is supposed to be triggered only by the texture cient being destroyed.
We must not call it in UpdateImage because other objects are still holding on to the texture client and may read it (RemoveTexture most times leads to the shared data being deallocated.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Do not share a texture with several compositables + do not call RemoveTextureClient before destroying the texture.

Here is what I have so far when trying to fix things "the right way" (that is without breaking the possibility to get a video element displaying the output of another video element).
That patches is a work in progress that contains the fixes for both this bug and 911941. I'll make separate patch when I get a solution that works. 

M3 is still red on try so it is not enough and it blows up with gralloc on the test page: http://people.mozilla.org/~nsilva/mediastream_autoplay.html?1

Comment 2

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #0)
> CompositableClient::RemoveTexture is supposed to be triggered only by the
> texture cient being destroyed.
> We must not call it in UpdateImage because other objects are still holding
> on to the texture client and may read it (RemoveTexture most times leads to
> the shared data being deallocated.

Can you explain about it more? I can understand the case that other objects holding the buffer of the TexutreClient. But I am not sure about the case of other objects holding the TexutreClient.

Comment 3

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> Can you explain about it more? I can understand the case that other objects
> holding the buffer of the TexutreClient. But I am not sure about the case of
> other objects holding the TexutreClient.

Other objects should not hold the buffer of TextureClient. The fact that it happens right now with gralloc is unfortunate and it is meant to change over time. If objects want to access shared texture data they should always do it through a TextureClient. That is the only way we can ensure that the memory management is correct.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Make sure shared texture deallocation doesn't race with TextureClient accessing it

I came up with an easier solution for the problem (I am less happy with it but it is much easier to implement in the current setup and more importantly with the approaching deadlines).

The problem with RemoveTexture right now is that TextureClient can still read the the texture data after RemoveTexture has been called.
The right way to avoid this race is to define that RemoveTexture is called when the texture client dies which means that by definition (since the texture client is the only object that can provide access to the shared data) the data is untouchable on the client side and the compositor side can safely deallocate the texture if the TEXTURE_DEALLOCATE_HOST flag is set.
It turns out to be a bit tricky to implement until we have PTexture and TextureClient becomes more independent from CompositableClient.

So in the mean time, when RemoveTexture is called, TextureClient enters an invalid state (MarkInvaid()) and we assert everywhere that nothing is ever trying to access the memory passed that point.

I still want to remove manual calls to RemoveTexture at some point in the future but this can wait for after the upcoming deadlines.

Comment 5

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=80334f762005

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 800058 [details] [diff] [review]
Make sure shared texture deallocation doesn't race with TextureClient accessing it

Looks good to me. This patch is safer than previous one in current TextureClient's implementation.

Comment 7

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/b2g-inbound/rev/af7a69606aa8

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/af7a69606aa8