993417 - [OMTC Windows] CairoTextureClientD3D9's destruction can race with the creation of it's texture host

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

If a CairoTextureClientD3D9 is destroyed after being serialized but before it's texture is deserialized on the host size, the content of the SurfaceDescriptorD3D9 (a pointer to a IDirect3DTexture9) ends up being a dangling pointer. and we crash in the host side when deserializing it.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Fix the race by addreffing the texture on the client side and releasing it on the host side

Comment 2

[Andreas Gal :gal]

This is pretty hacky. Modeling this with a return type that implies that its addref-ed would be nicer.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8403293 [details] [diff] [review]
Fix the race by addreffing the texture on the client side and releasing it on the host side

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +644,5 @@
>    if (!IsAllocated()) {
>      return false;
>    }
>  
> +  mTexture->AddRef(); // Release in TextureHostD3D9::TextureHostD3D9

This is very scary, it requires careful documentation, as this basically depends strongly on the SurfaceDescriptor that's being retrieved -actually- being used somewhere to create a texturehost. That's okay, but it will be very easy to accidentally break in the future.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Make ToSurfaceDescriptor protected and document it's quirk

Agreed. The deprecated code used to do exactly that so I didn't look for a fancier solution. This patch makes ToSurfaceDescriptor protected and adds a comment, which isn't quite as good as a bunch of well placed assertions but I guess it'll make do for now.
I have a few ideas to better track the internal state of TextureClient and make it easier to assert the things we care about, but I'd rather focus on getting OMTC everywhere at the moment.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Make ToSurfaceDescriptor protected and document it's quirk (fixed)

Oops, I was building the wrong tree and the previous version of this patch didn't build.

Comment 6

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba08bcd1d729
https://hg.mozilla.org/integration/mozilla-inbound/rev/7906728a1b88

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba08bcd1d729
https://hg.mozilla.org/mozilla-central/rev/7906728a1b88