Merge lp:~gerboland/qtmir/frameSwapped-crash-fix into lp:qtmir

[Gerry Boland]

Manage frameSwapped signal/slot connection with MirSurface more strictly to avoid crash.

Direct Signal/slot connections across thread boundaries incur the same risks as any cross-thread calls. While connect/disconnect are thread safe methods, it is possible for a slot to be called while the slot owner is being deconstructed - and so not yet disconnected.

So watch for the Item's window change signal and disconnect signal immediately. Also move slot ownership to MirSurfaceItem to auto-disconnect more aggressively.

[Gerry Boland (gerboland)]

To test, run the entire UITK AP suite and verify unity8 doesn't crash.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:414
http://jenkins.qa.ubuntu.com/job/qtmir-ci/568/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/264
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/264
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/264/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/146
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/301/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/301/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/146/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/568/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:415
http://jenkins.qa.ubuntu.com/job/qtmir-ci/569/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/265
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/265
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/265/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/147
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/302/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/302/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/147/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/569/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

I think this code in setSurface() is now redundant:

"""
        if (m_window) {
            connect(m_window, &QQuickWindow::frameSwapped, this, &MirSurfaceItem::onCompositorSwappedBuffers,
                (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
        }
"""

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:416
http://jenkins.qa.ubuntu.com/job/qtmir-ci/570/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/266
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/266
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/266/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/148
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/303/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/303/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/148/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/570/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:417
http://jenkins.qa.ubuntu.com/job/qtmir-ci/574/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/270
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/270
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/270/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/152
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/307/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/307/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/152/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/574/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:418
http://jenkins.qa.ubuntu.com/job/qtmir-ci/575/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/271
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/271
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/271/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/153
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/308/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/308/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/153/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/575/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Code looks ok. Need to test still.

review: Approve

[Daniel d'Andrada (dandrader)]

review: Approve (code)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:419
http://jenkins.qa.ubuntu.com/job/qtmir-ci/576/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/272
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/272/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/154
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/309/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/309/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/154/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/576/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Run ubuntuuitoolkit autopilot3 test suite and there were no crashes (although 30 of them failed)

review: Approve

[Daniel d'Andrada (dandrader)]

same code, same verdict.

review: Approve

[lp:~gerboland/qtmir/frameSwapped-crash-fix]

413. By Gerry Boland on 2015-11-24

Manage frameSwapped signal/slot connection with MirSurface more strictly to avoid crash.

Direct Signal/slot connections across thread boundaries incur the same risks as any cross-thread calls. While connect/disconnect are thread safe methods, it is possible for a slot to be called while the slot owner is being deconstructed - and so not yet disconnected.

So watch for the Item's window change signal and disconnect signal immediately. Also move slot ownership to MirSurfaceItem to auto-disconnect more agressively.

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2015-11-24 16:01:06 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-11-24 16:01:06 +0000
@@ -84,6 +84,7 @@
 MirSurfaceItem::MirSurfaceItem(QQuickItem *parent)
     : MirSurfaceItemInterface(parent)
     , m_surface(nullptr)
+    , m_window(nullptr)
     , m_textureProvider(nullptr)
     , m_lastTouchEvent(nullptr)
     , m_lastFrameNumberRendered(nullptr)
@@ -107,6 +108,7 @@
 
     connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);
     connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility);
+    connect(this, &QQuickItem::windowChanged, this, &MirSurfaceItem::onWindowChanged);
 }
 
 MirSurfaceItem::~MirSurfaceItem()
@@ -614,11 +616,6 @@
         }
 
         m_surface->unregisterView((qintptr)this);
-
-        if (!m_surface->isBeingDisplayed() && window()) {
-            disconnect(window(), nullptr, m_surface, nullptr);
-        }
-
         unsetCursor();
     }
 
@@ -636,11 +633,6 @@
         connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
         connect(m_surface, &MirSurfaceInterface::cursorChanged, this, &MirSurfaceItem::setCursor);
 
-        if (window()) {
-            connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
-                (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
-        }
-
         Q_EMIT typeChanged(m_surface->type());
         Q_EMIT liveChanged(true);
         Q_EMIT surfaceStateChanged(m_surface->state());
@@ -674,6 +666,25 @@
     Q_EMIT surfaceChanged(m_surface);
 }
 
+void MirSurfaceItem::onCompositorSwappedBuffers()
+{
+    if (Q_LIKELY(m_surface)) {
+        m_surface->onCompositorSwappedBuffers();
+    }
+}
+
+void MirSurfaceItem::onWindowChanged(QQuickWindow *window)
+{
+    if (m_window) {
+        disconnect(m_window, nullptr, this, nullptr);
+    }
+    m_window = window;
+    if (m_window) {
+        connect(m_window, &QQuickWindow::frameSwapped, this, &MirSurfaceItem::onCompositorSwappedBuffers,
+            Qt::DirectConnection);
+    }
+}
+
 void MirSurfaceItem::releaseResources()
 {
     if (m_textureProvider) {

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h	2015-11-19 12:55:57 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h	2015-11-24 16:01:06 +0000
@@ -115,6 +115,9 @@
     void updateMirSurfaceVisibility();
 
     void onActualSurfaceSizeChanged(const QSize &size);
+    void onCompositorSwappedBuffers();
+
+    void onWindowChanged(QQuickWindow *window);
 
 private:
     void ensureTextureProvider();
@@ -131,6 +134,7 @@
             Qt::TouchPointStates touchPointStates);
 
     MirSurfaceInterface* m_surface;
+    QQuickWindow* m_window;
 
     QMutex m_mutex;
     MirTextureProvider *m_textureProvider;