Merge lp:~gerboland/qtubuntu/fix-test-crash-backingstore into lp:qtubuntu

Proposed by Gerry Boland on 2017-03-13
Status: Merged
Approved by: Daniel d'Andrada on 2017-03-24
Approved revision: 384
Merged at revision: 386
Proposed branch: lp:~gerboland/qtubuntu/fix-test-crash-backingstore
Merge into: lp:qtubuntu
Diff against target: 36 lines (+13/-3)
1 file modified
src/ubuntumirclient/qmirclientbackingstore.cpp (+13/-3)
To merge this branch: bzr merge lp:~gerboland/qtubuntu/fix-test-crash-backingstore
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) 2017-03-13 Approve on 2017-03-24
Unity8 CI Bot continuous-integration 2017-03-13 Approve on 2017-03-24
Review via email: mp+319695@code.launchpad.net

This proposal supersedes a proposal from 2017-03-13.

Commit message

BackingStore: is possible for context's QPlatformSurface to be deleted before backing store is.

On destruction, check if there is a current context, and if not, it is likely the QWindow's original QPlatformSurface has been deleted. To clean up properly, need to create a temporary QOffscreenSurface, which can then acquire the GL context and delete the texture safely.

Description of the change

To post a comment you must log in.
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

You may drop the prerequisite now.

381. By Gerry Boland on 2017-03-13

Add required header

382. By Gerry Boland on 2017-03-13

Shrink diff slightly

383. By Gerry Boland on 2017-03-13

Missing close quote

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:381
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/202/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4410
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4438
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4271/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4271/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4271/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4271/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4271/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4271
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4271/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/202/rebuild

review: Approve (continuous-integration)
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:383
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/203/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4413
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4441
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4274/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4274/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4274/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4274/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4274/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4274
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4274/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/203/rebuild

review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

This isn't easy to test really. I had to apply this patch to Qt upstream to fix QWindow unit tests crashing.

Daniel d'Andrada (dandrader) wrote :

I might be missing something, but is there a reason why the tempSurface declaration is outside the if(!context){} block?

review: Needs Information
Gerry Boland (gerboland) wrote :

You're right, it can be brought in, fixing

Gerry Boland (gerboland) wrote :

Delay because actually bringing the tempSurface inside the if block causes crashes again. Unsure why

384. By Gerry Boland on 2017-03-24

QOffScreenSurface on stack instead of heap

Gerry Boland (gerboland) wrote :

Nope, it is ok. Crash was due to something else.

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:384
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/213/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4685
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4713
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4536/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4536/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4536/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4536/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4536/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4536
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4536/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/213/rebuild

review: Approve (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

* Did CI run pass? If not, please explain why.
Yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ubuntumirclient/qmirclientbackingstore.cpp'
2--- src/ubuntumirclient/qmirclientbackingstore.cpp 2017-02-21 13:23:47 +0000
3+++ src/ubuntumirclient/qmirclientbackingstore.cpp 2017-03-24 15:27:00 +0000
4@@ -40,6 +40,7 @@
5
6 #include "qmirclientbackingstore.h"
7 #include "qmirclientlogging.h"
8+#include <QtGui/QOffscreenSurface>
9 #include <QtGui/QOpenGLContext>
10 #include <QtGui/QOpenGLTexture>
11 #include <QtGui/QMatrix4x4>
12@@ -61,12 +62,21 @@
13
14 QMirClientBackingStore::~QMirClientBackingStore()
15 {
16+ if (!mTexture->isCreated())
17+ return;
18+
19 // Paraphrasing QOpenGLCompositorBackingStore: "With render-to-texture-widgets QWidget makes
20 // sure the context is made current before destroying backingstores. That is however not the
21- // case for windows with regular widgets only.
22- if (!QOpenGLContext::currentContext()) {
23- mContext->makeCurrent(window());
24+ // case for windows with regular widgets only."
25+ auto context = QOpenGLContext::currentContext();
26+ if (!context) { // QWindow's backing QPlatformSurface probably gone, use temp one for cleanup
27+ QOffscreenSurface tempSurface;
28+ tempSurface.setFormat(mContext->format());
29+ tempSurface.create();
30+ mContext->makeCurrent(&tempSurface);
31 }
32+ // QOpenGLTexture will go out of scope, is then deleted. Then QOpenGLContext falls out of
33+ // scope, calls doneCurrent and is then deleted.
34 }
35
36 void QMirClientBackingStore::flush(QWindow* window, const QRegion& region, const QPoint& offset)

Subscribers

People subscribed via source and target branches