Fix GL texture leaks when pixmaps are deleted

This fixes quite a lot of issues:

* QtOpenGL only registered qpixmap destruction hooks on X11 and
  those only cleanup the EGL/GLX surface, not the texture object.
* The QPixmap destruction hooks were only being called from the
  QPixmap destructor. However, this means when a QPixmap is assigned
  to another QPixmap, the hooks don't get called.

Task-number: QTBUG-7647
Reviewed-By: Samuel
Reviewed-By: Trond

6 files changed, 51 insertions, 30 deletions

diff --git a/src/gui/image/qimagepixmapcleanuphooks.cpp b/src/gui/image/qimagepixmapcleanuphooks.cpp
index 61d538f..ace4bb6 100644
--- a/src/gui/image/qimagepixmapcleanuphooks.cpp
+++ b/src/gui/image/qimagepixmapcleanuphooks.cpp
@@ -62,12 +62,12 @@ QImagePixmapCleanupHooks *QImagePixmapCleanupHooks::instance()
     return qt_image_and_pixmap_cleanup_hooks();
 }
 
-void QImagePixmapCleanupHooks::addPixmapModificationHook(_qt_pixmap_cleanup_hook_pm hook)
+void QImagePixmapCleanupHooks::addPixmapDataModificationHook(_qt_pixmap_cleanup_hook_pmd hook)
 {
     pixmapModificationHooks.append(hook);
 }
 
-void QImagePixmapCleanupHooks::addPixmapDestructionHook(_qt_pixmap_cleanup_hook_pm hook)
+void QImagePixmapCleanupHooks::addPixmapDataDestructionHook(_qt_pixmap_cleanup_hook_pmd hook)
 {
     pixmapDestructionHooks.append(hook);
 }
@@ -78,12 +78,12 @@ void QImagePixmapCleanupHooks::addImageHook(_qt_image_cleanup_hook_64 hook)
     imageHooks.append(hook);
 }
 
-void QImagePixmapCleanupHooks::removePixmapModificationHook(_qt_pixmap_cleanup_hook_pm hook)
+void QImagePixmapCleanupHooks::removePixmapDataModificationHook(_qt_pixmap_cleanup_hook_pmd hook)
 {
     pixmapModificationHooks.removeAll(hook);
 }
 
-void QImagePixmapCleanupHooks::removePixmapDestructionHook(_qt_pixmap_cleanup_hook_pm hook)
+void QImagePixmapCleanupHooks::removePixmapDataDestructionHook(_qt_pixmap_cleanup_hook_pmd hook)
 {
     pixmapDestructionHooks.removeAll(hook);
 }
@@ -93,24 +93,24 @@ void QImagePixmapCleanupHooks::removeImageHook(_qt_image_cleanup_hook_64 hook)
     imageHooks.removeAll(hook);
 }
 
-void QImagePixmapCleanupHooks::executePixmapModificationHooks(QPixmap* pm)
+void QImagePixmapCleanupHooks::executePixmapDataModificationHooks(QPixmapData* pmd)
 {
     QImagePixmapCleanupHooks *h = qt_image_and_pixmap_cleanup_hooks();
     for (int i = 0; i < h->pixmapModificationHooks.count(); ++i)
-        h->pixmapModificationHooks[i](pm);
+        h->pixmapModificationHooks[i](pmd);
 
     if (qt_pixmap_cleanup_hook_64)
-        qt_pixmap_cleanup_hook_64(pm->cacheKey());
+        qt_pixmap_cleanup_hook_64(pmd->cacheKey());
 }
 
-void QImagePixmapCleanupHooks::executePixmapDestructionHooks(QPixmap* pm)
+void QImagePixmapCleanupHooks::executePixmapDataDestructionHooks(QPixmapData* pmd)
 {
     QImagePixmapCleanupHooks *h = qt_image_and_pixmap_cleanup_hooks();
     for (int i = 0; i < h->pixmapDestructionHooks.count(); ++i)
-        h->pixmapDestructionHooks[i](pm);
+        h->pixmapDestructionHooks[i](pmd);
 
     if (qt_pixmap_cleanup_hook_64)
-        qt_pixmap_cleanup_hook_64(pm->cacheKey());
+        qt_pixmap_cleanup_hook_64(pmd->cacheKey());
 }
 
 void QImagePixmapCleanupHooks::executeImageHooks(qint64 key)
diff --git a/src/gui/image/qimagepixmapcleanuphooks_p.h b/src/gui/image/qimagepixmapcleanuphooks_p.h
index 7176044..88dd3a6 100644
--- a/src/gui/image/qimagepixmapcleanuphooks_p.h
+++ b/src/gui/image/qimagepixmapcleanuphooks_p.h
@@ -58,7 +58,8 @@
 QT_BEGIN_NAMESPACE
 
 typedef void (*_qt_image_cleanup_hook_64)(qint64);
-typedef void (*_qt_pixmap_cleanup_hook_pm)(QPixmap*);
+typedef void (*_qt_pixmap_cleanup_hook_pmd)(QPixmapData*);
+
 
 class QImagePixmapCleanupHooks;
 
@@ -71,27 +72,27 @@ public:
     static void enableCleanupHooks(const QPixmap &pixmap);
     static void enableCleanupHooks(QPixmapData *pixmapData);
 
-    // Gets called when a pixmap is about to be modified:
-    void addPixmapModificationHook(_qt_pixmap_cleanup_hook_pm);
+    // Gets called when a pixmap data is about to be modified:
+    void addPixmapDataModificationHook(_qt_pixmap_cleanup_hook_pmd);
 
-    // Gets called when a pixmap is about to be destroyed:
-    void addPixmapDestructionHook(_qt_pixmap_cleanup_hook_pm);
+    // Gets called when a pixmap data is about to be destroyed:
+    void addPixmapDataDestructionHook(_qt_pixmap_cleanup_hook_pmd);
 
     // Gets called when an image is about to be modified or destroyed:
     void addImageHook(_qt_image_cleanup_hook_64);
 
-    void removePixmapModificationHook(_qt_pixmap_cleanup_hook_pm);
-    void removePixmapDestructionHook(_qt_pixmap_cleanup_hook_pm);
+    void removePixmapDataModificationHook(_qt_pixmap_cleanup_hook_pmd);
+    void removePixmapDataDestructionHook(_qt_pixmap_cleanup_hook_pmd);
     void removeImageHook(_qt_image_cleanup_hook_64);
 
-    static void executePixmapModificationHooks(QPixmap*);
-    static void executePixmapDestructionHooks(QPixmap*);
+    static void executePixmapDataModificationHooks(QPixmapData*);
+    static void executePixmapDataDestructionHooks(QPixmapData*);
     static void executeImageHooks(qint64 key);
 
 private:
     QList<_qt_image_cleanup_hook_64> imageHooks;
-    QList<_qt_pixmap_cleanup_hook_pm> pixmapModificationHooks;
-    QList<_qt_pixmap_cleanup_hook_pm> pixmapDestructionHooks;
+    QList<_qt_pixmap_cleanup_hook_pmd> pixmapModificationHooks;
+    QList<_qt_pixmap_cleanup_hook_pmd> pixmapDestructionHooks;
 };
 
 QT_END_NAMESPACE
diff --git a/src/gui/image/qpixmap.cpp b/src/gui/image/qpixmap.cpp
index f823fdc..d1e5c40 100644
--- a/src/gui/image/qpixmap.cpp
+++ b/src/gui/image/qpixmap.cpp
@@ -320,8 +320,6 @@ QPixmap::QPixmap(const char * const xpm[])
 QPixmap::~QPixmap()
 {
     Q_ASSERT(!data || data->ref >= 1); // Catch if ref-counting changes again
-    if (data && data->is_cached && data->ref == 1) // ref will be decrememnted after destructor returns
-        QImagePixmapCleanupHooks::executePixmapDestructionHooks(this);
 }
 
 /*!
@@ -1025,12 +1023,8 @@ qint64 QPixmap::cacheKey() const
     if (isNull())
         return 0;
 
-    int classKey = data->classId();
-    if (classKey >= 1024)
-        classKey = -(classKey >> 10);
-    return ((((qint64) classKey) << 56)
-            | (((qint64) data->serialNumber()) << 32)
-            | ((qint64) (data->detach_no)));
+    Q_ASSERT(data);
+    return data->cacheKey();
 }
 
 static void sendResizeEvents(QWidget *target)
@@ -1943,7 +1937,7 @@ void QPixmap::detach()
     }
 
     if (data->is_cached && data->ref == 1)
-        QImagePixmapCleanupHooks::executePixmapModificationHooks(this);
+        QImagePixmapCleanupHooks::executePixmapDataModificationHooks(data.data());
 
 #if defined(Q_WS_MAC)
     QMacPixmapData *macData = id == QPixmapData::MacClass ? static_cast<QMacPixmapData*>(data.data()) : 0;
diff --git a/src/gui/image/qpixmap_x11.cpp b/src/gui/image/qpixmap_x11.cpp
index 0e66e09..169a2ec 100644
--- a/src/gui/image/qpixmap_x11.cpp
+++ b/src/gui/image/qpixmap_x11.cpp
@@ -68,6 +68,7 @@
 #include "qx11info_x11.h"
 #include <private/qdrawhelper_p.h>
 #include <private/qimage_p.h>
+#include <private/qimagepixmapcleanuphooks_p.h>
 
 #include <stdlib.h>
 
@@ -1228,6 +1229,12 @@ void QX11PixmapData::fill(const QColor &fillColor)
 
 QX11PixmapData::~QX11PixmapData()
 {
+    // Cleanup hooks have to be called before the handles are freed
+    if (is_cached) {
+        QImagePixmapCleanupHooks::executePixmapDataDestructionHooks(this);
+        is_cached = false;
+    }
+
     release();
 }
 
diff --git a/src/gui/image/qpixmapdata.cpp b/src/gui/image/qpixmapdata.cpp
index 65032da..ea4fe6b 100644
--- a/src/gui/image/qpixmapdata.cpp
+++ b/src/gui/image/qpixmapdata.cpp
@@ -45,6 +45,7 @@
 #include <QtGui/qimagereader.h>
 #include <private/qgraphicssystem_p.h>
 #include <private/qapplication_p.h>
+#include <private/qimagepixmapcleanuphooks_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -80,6 +81,16 @@ QPixmapData::QPixmapData(PixelType pixelType, int objectId)
 
 QPixmapData::~QPixmapData()
 {
+    // Sometimes the pixmap cleanup hooks will be called from derrived classes, which will
+    // then set is_cached to false. For example, on X11 QtOpenGL needs to delete the GLXPixmap
+    // or EGL Pixmap Surface for a given pixmap _before_ the native X11 pixmap is deleted,
+    // otherwise some drivers will leak the GL surface. In this case, QX11PixmapData will
+    // call the cleanup hooks itself before deleting the native pixmap and set is_cached to
+    // false.
+    if (is_cached) {
+        QImagePixmapCleanupHooks::executePixmapDataDestructionHooks(this);
+        is_cached = false;
+    }
 }
 
 QPixmapData *QPixmapData::createCompatiblePixmapData() const
diff --git a/src/gui/image/qpixmapdata_p.h b/src/gui/image/qpixmapdata_p.h
index 1125515..827fa18 100644
--- a/src/gui/image/qpixmapdata_p.h
+++ b/src/gui/image/qpixmapdata_p.h
@@ -117,6 +117,14 @@ public:
     inline int colorCount() const { return metric(QPaintDevice::PdmNumColors); }
     inline int depth() const { return d; }
     inline bool isNull() const { return is_null; }
+    inline qint64 cacheKey() const {
+        int classKey = id;
+        if (classKey >= 1024)
+            classKey = -(classKey >> 10);
+        return ((((qint64) classKey) << 56)
+                | (((qint64) ser_no) << 32)
+                | ((qint64) detach_no));
+    }
 
 #if defined(Q_OS_SYMBIAN)
     virtual void* toNativeType(NativeType type);