Fix locking a destroyed mutex.
Surface should not lock the resource of a parent texture at destruction, because
it can already have been destroyed. For example when a texture's image was bound
as a render target, and the texture is deleted by the app, then the image holds
the last reference to the texture. When the render target image gets deleted, it
first releases its parent texture, and then the underlying surface gets
destroyed.
This is fixed by synchronizing, by locking and unlocking the (parent) resource,
earlier. The derived class is responsible for calling Surface::sync() before
releasing the parent resource.
Bug chromium:716803
Change-Id: Ifc3685dcf9e25e8419000af65d4bb7407f26bbcb
Reviewed-on: https://swiftshader-review.googlesource.com/9750
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>
diff --git a/src/Common/Resource.hpp b/src/Common/Resource.hpp
index 379b769..0acfa48 100644
--- a/src/Common/Resource.hpp
+++ b/src/Common/Resource.hpp
@@ -24,7 +24,7 @@
PUBLIC, // Application/API access
PRIVATE, // Renderer access, shared by multiple threads if read-only
MANAGED, // Renderer access, shared read/write access if partitioned
- DESTRUCT
+ EXCLUSIVE
};
class Resource
diff --git a/src/D3D8/Direct3DCubeTexture8.cpp b/src/D3D8/Direct3DCubeTexture8.cpp
index d8b4a3c..e12b9b5 100644
--- a/src/D3D8/Direct3DCubeTexture8.cpp
+++ b/src/D3D8/Direct3DCubeTexture8.cpp
@@ -37,7 +37,7 @@
for(unsigned int level = 0; level < sw::MIPMAP_LEVELS; level++)
{
if(level < this->levels)
- {
+ {
surfaceLevel[face][level] = new Direct3DSurface8(device, this, width, height, format, pool, D3DMULTISAMPLE_NONE, true, usage);
surfaceLevel[face][level]->bind();
}
@@ -54,8 +54,6 @@
Direct3DCubeTexture8::~Direct3DCubeTexture8()
{
- resource->lock(sw::DESTRUCT);
-
for(unsigned int face = 0; face < 6; face++)
{
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
@@ -67,8 +65,6 @@
}
}
}
-
- resource->unlock();
}
long Direct3DCubeTexture8::QueryInterface(const IID &iid, void **object)
diff --git a/src/D3D8/Direct3DTexture8.cpp b/src/D3D8/Direct3DTexture8.cpp
index c7d8341..7ecd385 100644
--- a/src/D3D8/Direct3DTexture8.cpp
+++ b/src/D3D8/Direct3DTexture8.cpp
@@ -48,8 +48,6 @@
Direct3DTexture8::~Direct3DTexture8()
{
- resource->lock(sw::DESTRUCT);
-
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
{
if(surfaceLevel[level])
@@ -58,8 +56,6 @@
surfaceLevel[level] = 0;
}
}
-
- resource->unlock();
}
long Direct3DTexture8::QueryInterface(const IID &iid, void **object)
diff --git a/src/D3D8/Direct3DVolumeTexture8.cpp b/src/D3D8/Direct3DVolumeTexture8.cpp
index 90cecde..62262e3 100644
--- a/src/D3D8/Direct3DVolumeTexture8.cpp
+++ b/src/D3D8/Direct3DVolumeTexture8.cpp
@@ -49,8 +49,6 @@
Direct3DVolumeTexture8::~Direct3DVolumeTexture8()
{
- resource->lock(sw::DESTRUCT);
-
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
{
if(volumeLevel[level])
@@ -59,8 +57,6 @@
volumeLevel[level] = 0;
}
}
-
- resource->unlock();
}
long Direct3DVolumeTexture8::QueryInterface(const IID &iid, void **object)
diff --git a/src/D3D9/Direct3DCubeTexture9.cpp b/src/D3D9/Direct3DCubeTexture9.cpp
index cfe1ed7..b0b1925 100644
--- a/src/D3D9/Direct3DCubeTexture9.cpp
+++ b/src/D3D9/Direct3DCubeTexture9.cpp
@@ -55,8 +55,6 @@
Direct3DCubeTexture9::~Direct3DCubeTexture9()
{
- resource->lock(sw::DESTRUCT);
-
for(unsigned int face = 0; face < 6; face++)
{
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
@@ -68,8 +66,6 @@
}
}
}
-
- resource->unlock();
}
long Direct3DCubeTexture9::QueryInterface(const IID &iid, void **object)
diff --git a/src/D3D9/Direct3DTexture9.cpp b/src/D3D9/Direct3DTexture9.cpp
index 1b8a562..a964782 100644
--- a/src/D3D9/Direct3DTexture9.cpp
+++ b/src/D3D9/Direct3DTexture9.cpp
@@ -49,8 +49,6 @@
Direct3DTexture9::~Direct3DTexture9()
{
- resource->lock(sw::DESTRUCT);
-
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
{
if(surfaceLevel[level])
@@ -59,8 +57,6 @@
surfaceLevel[level] = 0;
}
}
-
- resource->unlock();
}
long Direct3DTexture9::QueryInterface(const IID &iid, void **object)
diff --git a/src/D3D9/Direct3DVolumeTexture9.cpp b/src/D3D9/Direct3DVolumeTexture9.cpp
index efe3d57..35caf43 100644
--- a/src/D3D9/Direct3DVolumeTexture9.cpp
+++ b/src/D3D9/Direct3DVolumeTexture9.cpp
@@ -50,8 +50,6 @@
Direct3DVolumeTexture9::~Direct3DVolumeTexture9()
{
- resource->lock(sw::DESTRUCT);
-
for(int level = 0; level < sw::MIPMAP_LEVELS; level++)
{
if(volumeLevel[level])
@@ -60,8 +58,6 @@
volumeLevel[level] = 0;
}
}
-
- resource->unlock();
}
long Direct3DVolumeTexture9::QueryInterface(const IID &iid, void **object)
diff --git a/src/OpenGL/common/Image.cpp b/src/OpenGL/common/Image.cpp
index cd27753..02f7f0f 100644
--- a/src/OpenGL/common/Image.cpp
+++ b/src/OpenGL/common/Image.cpp
@@ -1182,6 +1182,8 @@
Image::~Image()
{
+ sync(); // Wait for any threads that use this image to finish.
+
if(parentTexture)
{
parentTexture->release();
diff --git a/src/OpenGL/common/Image.hpp b/src/OpenGL/common/Image.hpp
index 63375b0..6cf0a59 100644
--- a/src/OpenGL/common/Image.hpp
+++ b/src/OpenGL/common/Image.hpp
@@ -254,9 +254,7 @@
virtual ~AndroidNativeImage()
{
- // Wait for any draw calls that use this image to finish
- resource->lock(sw::DESTRUCT);
- resource->unlock();
+ sync(); // Wait for any threads that use this image to finish.
nativeBuffer->common.decRef(&nativeBuffer->common);
}
diff --git a/src/OpenGL/libGL/Texture.cpp b/src/OpenGL/libGL/Texture.cpp
index 0265a1c..e0756d2 100644
--- a/src/OpenGL/libGL/Texture.cpp
+++ b/src/OpenGL/libGL/Texture.cpp
@@ -285,8 +285,6 @@
Texture2D::~Texture2D()
{
- resource->lock(sw::DESTRUCT);
-
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
{
if(image[i])
@@ -296,8 +294,6 @@
}
}
- resource->unlock();
-
mColorbufferProxy = nullptr;
}
@@ -642,8 +638,6 @@
TextureCubeMap::~TextureCubeMap()
{
- resource->lock(sw::DESTRUCT);
-
for(int f = 0; f < 6; f++)
{
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
@@ -656,8 +650,6 @@
}
}
- resource->unlock();
-
for(int i = 0; i < 6; i++)
{
mFaceProxies[i] = nullptr;
diff --git a/src/OpenGL/libGLES_CM/Texture.cpp b/src/OpenGL/libGLES_CM/Texture.cpp
index 2f99200..a5c86a7 100644
--- a/src/OpenGL/libGLES_CM/Texture.cpp
+++ b/src/OpenGL/libGLES_CM/Texture.cpp
@@ -347,8 +347,6 @@
Texture2D::~Texture2D()
{
- resource->lock(sw::DESTRUCT);
-
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
{
if(image[i])
@@ -358,8 +356,6 @@
}
}
- resource->unlock();
-
if(mSurface)
{
mSurface->setBoundTexture(nullptr);
diff --git a/src/OpenGL/libGLESv2/Texture.cpp b/src/OpenGL/libGLESv2/Texture.cpp
index 244041c..48bd363 100644
--- a/src/OpenGL/libGLESv2/Texture.cpp
+++ b/src/OpenGL/libGLESv2/Texture.cpp
@@ -523,8 +523,6 @@
Texture2D::~Texture2D()
{
- resource->lock(sw::DESTRUCT);
-
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
{
if(image[i])
@@ -534,8 +532,6 @@
}
}
- resource->unlock();
-
if(mSurface)
{
mSurface->setBoundTexture(nullptr);
@@ -1004,8 +1000,6 @@
TextureCubeMap::~TextureCubeMap()
{
- resource->lock(sw::DESTRUCT);
-
for(int f = 0; f < 6; f++)
{
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
@@ -1018,8 +1012,6 @@
}
}
- resource->unlock();
-
for(int i = 0; i < 6; i++)
{
mFaceProxies[i] = nullptr;
@@ -1486,8 +1478,6 @@
Texture3D::~Texture3D()
{
- resource->lock(sw::DESTRUCT);
-
for(int i = 0; i < IMPLEMENTATION_MAX_TEXTURE_LEVELS; i++)
{
if(image[i])
@@ -1497,8 +1487,6 @@
}
}
- resource->unlock();
-
if(mSurface)
{
mSurface->setBoundTexture(nullptr);
diff --git a/src/Renderer/Surface.cpp b/src/Renderer/Surface.cpp
index 4eb77ca..c7ca71a 100644
--- a/src/Renderer/Surface.cpp
+++ b/src/Renderer/Surface.cpp
@@ -1270,9 +1270,11 @@
Surface::~Surface()
{
- // Synchronize so we can deallocate the buffers below
- resource->lock(DESTRUCT);
- resource->unlock();
+ // sync() must be called before this destructor to ensure all locks have been released.
+ // We can't call it here because the parent resource may already have been destroyed.
+ ASSERT(external.lock == LOCK_UNLOCKED);
+ ASSERT(internal.lock == LOCK_UNLOCKED);
+ ASSERT(stencil.lock == LOCK_UNLOCKED);
if(!hasParent)
{
@@ -3158,6 +3160,12 @@
}
}
+ void Surface::sync()
+ {
+ resource->lock(EXCLUSIVE);
+ resource->unlock();
+ }
+
bool Surface::isEntire(const SliceRect& rect) const
{
return (rect.x0 == 0 && rect.y0 == 0 && rect.x1 == internal.width && rect.y1 == internal.height && internal.depth == 1);
diff --git a/src/Renderer/Surface.hpp b/src/Renderer/Surface.hpp
index fa04393..ae37df5 100644
--- a/src/Renderer/Surface.hpp
+++ b/src/Renderer/Surface.hpp
@@ -291,6 +291,8 @@
inline int getStencilPitchB() const;
inline int getStencilSliceB() const;
+ void sync(); // Wait for lock(s) to be released
+
inline int getMultiSampleCount() const;
inline int getSuperSampleCount() const;
@@ -345,10 +347,9 @@
static void setTexturePalette(unsigned int *palette);
- protected:
+ private:
sw::Resource *resource;
- private:
typedef unsigned char byte;
typedef unsigned short word;
typedef unsigned int dword;
