Prevented threading related crash in OpenGL module.

If the last shallow copy of a QImage which is cached in the
QGLTextureCache is destroyed in a thread at the same time as the
QGLContext which the texture was initialized in is active in a different
thread, the QImage thread incorrectly tries to make the context active
in two threads at once. To prevent this from happening it's best to
always do the texture clean-up in the main thread.

Task-number: QT-4238
Reviewed-by: Eskil Abrahamsen Blomfeldt

3 files changed, 169 insertions, 24 deletions

diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp
index dbd295f..84cfa97 100644
--- a/src/opengl/qgl.cpp
+++ b/src/opengl/qgl.cpp
@@ -127,7 +127,12 @@ Q_GLOBAL_STATIC(QGLDefaultOverlayFormat, defaultOverlayFormatInstance)
 Q_GLOBAL_STATIC(QGLSignalProxy, theSignalProxy)
 QGLSignalProxy *QGLSignalProxy::instance()
 {
-    return theSignalProxy();
+    QGLSignalProxy *proxy = theSignalProxy();
+    if (proxy && proxy->thread() != qApp->thread()) {
+        if (proxy->thread() == QThread::currentThread())
+            proxy->moveToThread(qApp->thread());
+    }
+    return proxy;
 }
 
 
diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h
index f86c77f..18b2765 100644
--- a/src/opengl/qgl_p.h
+++ b/src/opengl/qgl_p.h
@@ -434,20 +434,6 @@ public:
     static void setCurrentContext(QGLContext *context);
 };
 
-// ### make QGLContext a QObject in 5.0 and remove the proxy stuff
-class Q_OPENGL_EXPORT QGLSignalProxy : public QObject
-{
-    Q_OBJECT
-public:
-    QGLSignalProxy() : QObject() {}
-    void emitAboutToDestroyContext(const QGLContext *context) {
-        emit aboutToDestroyContext(context);
-    }
-    static QGLSignalProxy *instance();
-Q_SIGNALS:
-    void aboutToDestroyContext(const QGLContext *context);
-};
-
 Q_DECLARE_OPERATORS_FOR_FLAGS(QGLExtensions::Extensions)
 
 // Temporarily make a context current if not already current or
@@ -490,6 +476,48 @@ private:
     QGLContext *m_ctx;
 };
 
+// ### make QGLContext a QObject in 5.0 and remove the proxy stuff
+class Q_OPENGL_EXPORT QGLSignalProxy : public QObject
+{
+    Q_OBJECT
+public:
+    QGLSignalProxy() : QObject() {
+        qRegisterMetaType<GLuint>("GLuint");
+        connect(this, SIGNAL(freeTexture(QGLContext *, QPixmapData *, GLuint)),
+                this, SLOT(freeTexture_slot(QGLContext *, QPixmapData *, GLuint)));
+    }
+    void emitAboutToDestroyContext(const QGLContext *context) {
+        emit aboutToDestroyContext(context);
+    }
+    void emitFreeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id) {
+        emit freeTexture(context, boundPixmap, id);
+    }
+    static QGLSignalProxy *instance();
+Q_SIGNALS:
+    void aboutToDestroyContext(const QGLContext *context);
+    void freeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id);
+
+private slots:
+    void freeTexture_slot(QGLContext *context, QPixmapData *boundPixmap, GLuint id) {
+#if defined(Q_WS_X11)
+        if (boundPixmap) {
+            QGLContext *oldContext = const_cast<QGLContext *>(QGLContext::currentContext());
+            context->makeCurrent();
+            // Although glXReleaseTexImage is a glX call, it must be called while there
+            // is a current context - the context the pixmap was bound to a texture in.
+            // Otherwise the release doesn't do anything and you get BadDrawable errors
+            // when you come to delete the context.
+            QGLContextPrivate::unbindPixmapFromTexture(boundPixmap);
+            glDeleteTextures(1, &id);
+            oldContext->makeCurrent();
+            return;
+        }
+#endif
+        QGLShareContextScope scope(context);
+        glDeleteTextures(1, &id);
+    }
+};
+
 class QGLTexture {
 public:
     QGLTexture(QGLContext *ctx = 0, GLuint tx_id = 0, GLenum tx_target = GL_TEXTURE_2D,
@@ -506,16 +534,10 @@ public:
     ~QGLTexture() {
         if (options & QGLContext::MemoryManagedBindOption) {
             Q_ASSERT(context);
-            QGLShareContextScope scope(context);
-#if defined(Q_WS_X11)
-            // Although glXReleaseTexImage is a glX call, it must be called while there
-            // is a current context - the context the pixmap was bound to a texture in.
-            // Otherwise the release doesn't do anything and you get BadDrawable errors
-            // when you come to delete the context.
-            if (boundPixmap)
-                QGLContextPrivate::unbindPixmapFromTexture(boundPixmap);
+#if !defined(Q_WS_X11)
+            QPixmapData *boundPixmap = 0;
 #endif
-            glDeleteTextures(1, &id);
+            QGLSignalProxy::instance()->emitFreeTexture(context, boundPixmap, id);
         }
      }
 
diff --git a/tests/auto/qgl/tst_qgl.cpp b/tests/auto/qgl/tst_qgl.cpp
index e411508..4220b45 100644
--- a/tests/auto/qgl/tst_qgl.cpp
+++ b/tests/auto/qgl/tst_qgl.cpp
@@ -96,6 +96,7 @@ private slots:
     void shareRegister();
     void qglContextDefaultBindTexture();
     void textureCleanup();
+    void threadImages();
 };
 
 tst_QGL::tst_QGL()
@@ -2253,6 +2254,123 @@ void tst_QGL::textureCleanup()
 #endif
 }
 
+namespace ThreadImages {
+
+class Producer : public QObject
+{
+    Q_OBJECT
+public:
+    Producer()
+    {
+        startTimer(20);
+
+        QThread *thread = new QThread;
+        thread->start();
+
+        connect(this, SIGNAL(destroyed()), thread, SLOT(quit()));
+
+        moveToThread(thread);
+        connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));
+    }
+
+signals:
+    void imageReady(const QImage &image);
+
+protected:
+    void timerEvent(QTimerEvent *)
+    {
+        QImage image(256, 256, QImage::Format_RGB32);
+        QLinearGradient g(0, 0, 0, 256);
+        g.setColorAt(0, QColor(255, 180, 180));
+        g.setColorAt(1, Qt::white);
+        g.setSpread(QGradient::ReflectSpread);
+
+        QBrush brush(g);
+        brush.setTransform(QTransform::fromTranslate(0, delta));
+        delta += 10;
+
+        QPainter p(&image);
+        p.fillRect(image.rect(), brush);
+
+        if (images.size() > 10)
+            images.removeFirst();
+
+        images.append(image);
+
+        emit imageReady(image);
+    }
+
+private:
+    QList<QImage> images;
+    int delta;
+};
+
+
+class DisplayWidget : public QGLWidget
+{
+    Q_OBJECT
+public:
+    DisplayWidget(QWidget *parent) : QGLWidget(parent) {}
+    void paintEvent(QPaintEvent *)
+    {
+        QPainter p(this);
+        p.drawImage(rect(), m_image);
+    }
+
+public slots:
+    void setImage(const QImage &image)
+    {
+        m_image = image;
+        update();
+    }
+
+private:
+    QImage m_image;
+};
+
+class Widget : public QWidget
+{
+    Q_OBJECT
+public:
+    Widget()
+        : iterations(0)
+        , display(0)
+    {
+        startTimer(400);
+    }
+
+    int iterations;
+
+protected:
+    void timerEvent(QTimerEvent *)
+    {
+        ++iterations;
+
+        delete display;
+        display = new DisplayWidget(this);
+        connect(&producer, SIGNAL(imageReady(const QImage &)), display, SLOT(setImage(const QImage &)));
+
+        display->setGeometry(rect());
+        display->show();
+    }
+
+private:
+    Producer producer;
+    DisplayWidget *display;
+};
+
+}
+
+void tst_QGL::threadImages()
+{
+    ThreadImages::Widget *widget = new ThreadImages::Widget;
+    widget->show();
+
+    while (widget->iterations <= 5) {
+        qApp->processEvents();
+    }
+}
+
 class tst_QGLDummy : public QObject
 {
 Q_OBJECT