Imagine: fix crash when switching between 9-patch and regular image

Consider the following changes in source:

    normal.png => press.9.png => normal.png => focus.png

If the last two events happen quickly, pixmapChange() can be called
twice with no call to updatePaintNode() inbetween. On the first call,
resetNode will be true (because ninePatch is not null since it is still
in the process of going from a 9-patch image to a regular image),
and on the second call, resetNode would be false if we didn't have this
check. This results in the oldNode never being deleted, and QQuickImage
tries to static_cast a QQuickNinePatchImage to a QSGInternalImageNode.

Only change resetNode when it's false; i.e. when no reset is pending.
updatePaintNode() will take care of setting it to false if it's true.

Change-Id: I614c172c3e24fda2506f081f8fcdb6acd1c65fb8
Fixes: QTBUG-78790
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

5 files changed, 69 insertions, 2 deletions

diff --git a/src/imports/controls/imagine/qquickninepatchimage.cpp b/src/imports/controls/imagine/qquickninepatchimage.cpp
index c840c6f8..7d5e4f71 100644
--- a/src/imports/controls/imagine/qquickninepatchimage.cpp
+++ b/src/imports/controls/imagine/qquickninepatchimage.cpp
@@ -397,7 +397,24 @@ void QQuickNinePatchImage::pixmapChange()
 
         d->updatePatches();
     } else {
-        d->resetNode = !d->ninePatch.isNull();
+        /*
+            Only change resetNode when it's false; i.e. when no reset is pending.
+            updatePaintNode() will take care of setting it to false if it's true.
+
+            Consider the following changes in source:
+
+                normal.png => press.9.png => normal.png => focus.png
+
+            If the last two events happen quickly, pixmapChange() can be called
+            twice with no call to updatePaintNode() inbetween. On the first call,
+            resetNode will be true (because ninePatch is not null since it is still
+            in the process of going from a 9-patch image to a regular image),
+            and on the second call, resetNode would be false if we didn't have this check.
+            This results in the oldNode never being deleted, and QQuickImage
+            tries to static_cast a QQuickNinePatchImage to a QSGInternalImageNode.
+        */
+        if (!d->resetNode)
+            d->resetNode = !d->ninePatch.isNull();
         d->ninePatch = QImage();
     }
     QQuickImage::pixmapChange();
diff --git a/tests/auto/qquickimaginestyle/data/tst_imagine.qml b/tests/auto/qquickimaginestyle/data/tst_imagine.qml
index 03bb9602..b9078d78 100644
--- a/tests/auto/qquickimaginestyle/data/tst_imagine.qml
+++ b/tests/auto/qquickimaginestyle/data/tst_imagine.qml
@@ -54,6 +54,7 @@ import QtTest 1.1
 import QtQuick.Templates 2.12 as T
 import QtQuick.Controls 2.12
 import QtQuick.Controls.Imagine 2.12
+import QtQuick.Controls.Imagine.impl 2.12
 
 TestCase {
     id: testCase
@@ -105,4 +106,51 @@ TestCase {
         verify(control)
         compare(control.font.pixelSize, 80)
     }
+
+    Component {
+        id: ninePatchImageComponent
+
+        NinePatchImage {
+            property alias mouseArea: mouseArea
+
+            MouseArea {
+                id: mouseArea
+                anchors.fill: parent
+                // The name of the images isn't important; we just want to check that
+                // going from regular to 9-patch to regular to regular works without crashing.
+                onPressed: parent.source = "qrc:/control-assets/button-background.9.png"
+                onReleased: parent.source = "qrc:/test-assets/button-background-1.png"
+                onClicked: parent.source = "qrc:/test-assets/button-background-2.png"
+            }
+        }
+    }
+
+    Component {
+        id: signalSpyComponent
+
+        SignalSpy {}
+    }
+
+    // QTBUG-78790
+    function test_switchBetween9PatchAndRegular() {
+        var ninePatchImage = createTemporaryObject(ninePatchImageComponent, testCase,
+            { source: "qrc:/test-assets/button-background-1.png" })
+        verify(ninePatchImage)
+
+        var clickSpy = signalSpyComponent.createObject(ninePatchImage,
+            { target: ninePatchImage.mouseArea, signalName: "clicked" })
+        verify(clickSpy.valid)
+
+        var afterRenderingSpy = signalSpyComponent.createObject(ninePatchImage,
+            { target: testCase.Window.window, signalName: "afterRendering" })
+        verify(afterRenderingSpy.valid)
+
+        mousePress(ninePatchImage)
+        // Wait max 1 second - in reality it should take a handful of milliseconds.
+        afterRenderingSpy.wait(1000)
+        mouseRelease(ninePatchImage)
+        compare(clickSpy.count, 1)
+        // Shouldn't result in a crash.
+        afterRenderingSpy.wait(1000)
+    }
 }
diff --git a/tests/auto/qquickimaginestyle/qquickimaginestyle.pro b/tests/auto/qquickimaginestyle/qquickimaginestyle.pro
index c421f2dc..4b1a309a 100644
--- a/tests/auto/qquickimaginestyle/qquickimaginestyle.pro
+++ b/tests/auto/qquickimaginestyle/qquickimaginestyle.pro
@@ -7,7 +7,9 @@ SOURCES += \
 
 RESOURCES += \
     $$PWD/qtquickcontrols2.conf \
-    $$PWD/control-assets/button-background.9.png
+    $$PWD/control-assets/button-background.9.png \
+    $$PWD/test-assets/button-background-1.png \
+    $$PWD/test-assets/button-background-2.png
 
 OTHER_FILES += \
     $$PWD/data/*.qml
diff --git a/tests/auto/qquickimaginestyle/test-assets/button-background-1.png b/tests/auto/qquickimaginestyle/test-assets/button-background-1.png
new file mode 100644
index 00000000..244b707b
--- /dev/null
+++ b/tests/auto/qquickimaginestyle/test-assets/button-background-1.png
diff --git a/tests/auto/qquickimaginestyle/test-assets/button-background-2.png b/tests/auto/qquickimaginestyle/test-assets/button-background-2.png
new file mode 100644
index 00000000..54f5ecd8
--- /dev/null
+++ b/tests/auto/qquickimaginestyle/test-assets/button-background-2.png