From 0431e462dff57bc6a9010649c0d7f153d91cab2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Arve=20S=C3=A6ther?= Date: Mon, 10 Sep 2018 16:30:26 +0200 Subject: Add some null pointer checks to avoid some rare crashes The crashes happened because somebody was calling processEvents() from a mouse/touch event handler that was running for a long time. If we called processEvents() from the onPressed handler, and we released the mouse while still not having returned from the onPressed handler, it meant that we were actually delivering the release event before the press event was fully delivered... This should normally not be a problem, but QQuickWindow is reusing the QQuickPointerEvent object for each incoming QEvent, which meant that when we were delivering the release event, it would reuse (and overwrite) the QQuickPointerEvent that the press event handler is still using.... This then caused some assumptions that the code made to be wrong. This only avoids the crashes, and doesn't really fix the "out-of-order" delivery and the state inconsistency that this can lead to (e.g. mouse button states might be still wrong). But on the other hand, it is not the recommended way of making a long-running handler not block the application. The proper way is to create a thread that will run in the background, so that there would be no need to call processEvents() in the event handler. Change-Id: I6fa5d4f5748ef30d082a210f03ef382922bd4b29 Task-number: QTBUG-65454 Reviewed-by: Shawn Rutledge --- src/quick/items/qquickevents_p_p.h | 2 +- src/quick/items/qquickwindow.cpp | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/quick/items/qquickevents_p_p.h b/src/quick/items/qquickevents_p_p.h index 3f70ad8c2d..4097845ec9 100644 --- a/src/quick/items/qquickevents_p_p.h +++ b/src/quick/items/qquickevents_p_p.h @@ -429,7 +429,7 @@ public: // helpers for C++ only (during event delivery) virtual bool allUpdatedPointsAccepted() const = 0; virtual bool allPointsGrabbed() const = 0; bool isAccepted() { return m_event->isAccepted(); } - void setAccepted(bool accepted) { m_event->setAccepted(accepted); } + void setAccepted(bool accepted) { if (m_event) m_event->setAccepted(accepted); } QVector unacceptedPressedPointScenePositions() const; virtual int pointCount() const = 0; diff --git a/src/quick/items/qquickwindow.cpp b/src/quick/items/qquickwindow.cpp index d9db8b56b4..5cd04decf7 100644 --- a/src/quick/items/qquickwindow.cpp +++ b/src/quick/items/qquickwindow.cpp @@ -690,8 +690,8 @@ bool QQuickWindowPrivate::deliverTouchAsMouse(QQuickItem *item, QQuickPointerEve touchMouseId = p.id(); if (!q->mouseGrabberItem()) item->grabMouse(); - auto pointerEventPoint = pointerEvent->pointById(p.id()); - pointerEventPoint->setGrabberItem(item); + if (auto pointerEventPoint = pointerEvent->pointById(p.id())) + pointerEventPoint->setGrabberItem(item); if (checkIfDoubleClicked(event->timestamp())) { QScopedPointer mouseDoubleClick(touchToMouseEvent(QEvent::MouseButtonDblClick, p, event.data(), item, false)); @@ -2607,19 +2607,22 @@ void QQuickWindowPrivate::deliverMatchingPointsToItem(QQuickItem *item, QQuickPo // update accepted new points. bool isPressOrRelease = pointerEvent->isPressEvent() || pointerEvent->isReleaseEvent(); for (auto point: qAsConst(touchEvent->touchPoints())) { - auto pointerEventPoint = ptEvent->pointById(point.id()); - pointerEventPoint->setAccepted(); - if (isPressOrRelease) - pointerEventPoint->setGrabberItem(item); + if (auto pointerEventPoint = ptEvent->pointById(point.id())) { + pointerEventPoint->setAccepted(); + if (isPressOrRelease) + pointerEventPoint->setGrabberItem(item); + } } } else { // But if the event was not accepted then we know this item // will not be interested in further updates for those touchpoint IDs either. for (auto point: qAsConst(touchEvent->touchPoints())) { if (point.state() == Qt::TouchPointPressed) { - if (ptEvent->pointById(point.id())->exclusiveGrabber() == item) { - qCDebug(DBG_TOUCH_TARGET) << "TP" << hex << point.id() << "disassociated"; - ptEvent->pointById(point.id())->setGrabberItem(nullptr); + if (auto *tp = ptEvent->pointById(point.id())) { + if (tp->exclusiveGrabber() == item) { + qCDebug(DBG_TOUCH_TARGET) << "TP" << hex << point.id() << "disassociated"; + tp->setGrabberItem(nullptr); + } } } } -- cgit v1.2.3 From 8fd398c9d2f5f54e446e0b402bc63a2edb50da6f Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Tue, 2 May 2017 16:25:37 +1000 Subject: Prevent PathView's parent stealing mouse grab during double-flick This commit fixes an issue where mouse events could be stolen by the parent of a PathView due to the PathView not correctly setting the keep-mouse-grab flag in handleMousePressEvent(). This commit ensures that the flag is correctly set, so that the second flick in a double flick is handled by the PathView rather than being stolen by the parent. Task-number: QTBUG-59620 Change-Id: Iccdfe16e7e80e6d1d31f95c3dba9c8839b20f30f Reviewed-by: Shawn Rutledge --- src/quick/items/qquickpathview.cpp | 2 + .../quick/qquickpathview/tst_qquickpathview.cpp | 55 ++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/src/quick/items/qquickpathview.cpp b/src/quick/items/qquickpathview.cpp index 879db6284e..e7e19b041e 100644 --- a/src/quick/items/qquickpathview.cpp +++ b/src/quick/items/qquickpathview.cpp @@ -1632,6 +1632,7 @@ void QQuickPathView::mousePressEvent(QMouseEvent *event) void QQuickPathViewPrivate::handleMousePressEvent(QMouseEvent *event) { + Q_Q(QQuickPathView); if (!interactive || !items.count() || !model || !modelCount) return; velocityBuffer.clear(); @@ -1657,6 +1658,7 @@ void QQuickPathViewPrivate::handleMousePressEvent(QMouseEvent *event) stealMouse = true; // If we've been flicked then steal the click. else stealMouse = false; + q->setKeepMouseGrab(stealMouse); timer.start(); lastPosTime = computeCurrentTime(event); diff --git a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp index e6c03d052c..badc6f44c8 100644 --- a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp +++ b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp @@ -50,6 +50,8 @@ #include +Q_LOGGING_CATEGORY(lcTests, "qt.quick.tests") + using namespace QQuickViewTestUtil; using namespace QQuickVisualTestUtil; @@ -2255,6 +2257,59 @@ void tst_QQuickPathView::nestedinFlickable() QCOMPARE(fflickingSpy.count(), 0); QCOMPARE(fflickStartedSpy.count(), 0); QCOMPARE(fflickEndedSpy.count(), 0); + + // now test that two quick flicks are both handled by the pathview + movingSpy.clear(); + moveStartedSpy.clear(); + moveEndedSpy.clear(); + fflickingSpy.clear(); + fflickStartedSpy.clear(); + fflickEndedSpy.clear(); + int shortInterval = 2; + QTest::mousePress(window.data(), Qt::LeftButton, 0, QPoint(23,216)); + QTest::mouseMove(window.data(), QPoint(48,216), shortInterval); + QTest::mouseRelease(window.data(), Qt::LeftButton, 0, QPoint(73,217)); + QVERIFY(pathview->isMoving()); + QTest::mousePress(window.data(), Qt::LeftButton, 0, QPoint(21,216)); + QTest::mouseMove(window.data(), QPoint(46,216), shortInterval); + QTest::mouseRelease(window.data(), Qt::LeftButton, 0, QPoint(71,217)); + QVERIFY(pathview->isMoving()); + // moveEndedSpy.count() and moveStartedSpy.count() should be exactly 1 + // but in CI we sometimes see a scheduling issue being hit which + // causes the main thread to be stalled while the animation thread + // continues, allowing the animation timer to finish after the first + // call to QVERIFY(pathview->isMoving()) in the code above, prior to + // the detected beginning of the second flick, which can cause both of + // those signal counts to be 2 rather than 1. + // Note that this is not a true problem (this scheduling quirk just + // means that the unit test is not testing the enforced behavior + // as strictly as it would otherwise); it is only a bug if it results + // in the Flickable handling one or more of the flicks, and that + // is unconditionally tested below. + // To avoid false positive test failure in the scheduling quirk case + // we allow the multiple signal count case, rather than simply: + // QTRY_COMPARE(moveEndedSpy.count(), 1); + // QCOMPARE(moveStartedSpy.count(), 1); + QTRY_VERIFY(moveEndedSpy.count() > 0); + qCDebug(lcTests) << "After receiving moveEnded signal:" + << "moveEndedSpy.count():" << moveEndedSpy.count() + << "moveStartedSpy.count():" << moveStartedSpy.count() + << "fflickingSpy.count():" << fflickingSpy.count() + << "fflickStartedSpy.count():" << fflickStartedSpy.count() + << "fflickEndedSpy.count():" << fflickEndedSpy.count(); + QTRY_COMPARE(moveStartedSpy.count(), moveEndedSpy.count()); + qCDebug(lcTests) << "After receiving matched moveEnded signal(s):" + << "moveEndedSpy.count():" << moveEndedSpy.count() + << "moveStartedSpy.count():" << moveStartedSpy.count() + << "fflickingSpy.count():" << fflickingSpy.count() + << "fflickStartedSpy.count():" << fflickStartedSpy.count() + << "fflickEndedSpy.count():" << fflickEndedSpy.count(); + QVERIFY(moveStartedSpy.count() <= 2); + // Flickable should not handle this + QCOMPARE(fflickingSpy.count(), 0); + QCOMPARE(fflickStartedSpy.count(), 0); + QCOMPARE(fflickEndedSpy.count(), 0); + } void tst_QQuickPathView::flickableDelegate() -- cgit v1.2.3 From 4441cefd76446a9e464665bd7fb0666f0e2495c3 Mon Sep 17 00:00:00 2001 From: Kai Koehne Date: Thu, 27 Sep 2018 10:04:42 +0200 Subject: Doc: Remove wrong ';' from Q_PROPERTY example This won't compile. Change-Id: I823435673ebe47900dd8ba2a2a9f6f49e4a31539 Reviewed-by: Leena Miettinen --- src/qml/qml/qqmllist.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qml/qml/qqmllist.cpp b/src/qml/qml/qqmllist.cpp index ac6e3695fe..918b181195 100644 --- a/src/qml/qml/qqmllist.cpp +++ b/src/qml/qml/qqmllist.cpp @@ -361,7 +361,7 @@ List properties should have no setter. In the example above, the Q_PROPERTY() declarative will look like this: \code -Q_PROPERTY(QQmlListProperty fruit READ fruit); +Q_PROPERTY(QQmlListProperty fruit READ fruit) \endcode QML list properties are type-safe - in this case \c {Fruit} is a QObject type that -- cgit v1.2.3 From 039a28468a1812b8f0662aba62c173e572899841 Mon Sep 17 00:00:00 2001 From: Janne Koskinen Date: Fri, 28 Sep 2018 12:15:58 +0200 Subject: Fix Integrity OS allocator memory attributes Correctly set the attributes when allocation is extending to more than one page. Code spanning multiple pages can now be executed. Task-number: QTBUG-70350 Change-Id: I02af1add274f80befda5662d9670bfd2052c3c52 Reviewed-by: Lars Knoll --- src/3rdparty/masm/stubs/ExecutableAllocator.h | 4 ++-- src/3rdparty/masm/wtf/OSAllocator.h | 2 +- src/3rdparty/masm/wtf/OSAllocatorIntegrity.cpp | 12 ++++++++---- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/3rdparty/masm/stubs/ExecutableAllocator.h b/src/3rdparty/masm/stubs/ExecutableAllocator.h index 1ab28588fb..16b17bd3cd 100644 --- a/src/3rdparty/masm/stubs/ExecutableAllocator.h +++ b/src/3rdparty/masm/stubs/ExecutableAllocator.h @@ -123,7 +123,7 @@ struct ExecutableAllocator { } # endif # elif OS(INTEGRITY) - OSAllocator::setMemoryAttributes(addr, /*writable*/ true, /*executable*/ false); + OSAllocator::setMemoryAttributes(addr, size, /*writable*/ true, /*executable*/ false); # else int mode = PROT_READ | PROT_WRITE; if (mprotect(addr, size, mode) != 0) { @@ -159,7 +159,7 @@ struct ExecutableAllocator { } # endif # elif OS(INTEGRITY) - OSAllocator::setMemoryAttributes(addr, /*writable*/ false, /*executable*/ true); + OSAllocator::setMemoryAttributes(addr, size, /*writable*/ false, /*executable*/ true); # else int mode = PROT_READ | PROT_EXEC; if (mprotect(addr, size, mode) != 0) { diff --git a/src/3rdparty/masm/wtf/OSAllocator.h b/src/3rdparty/masm/wtf/OSAllocator.h index 366dd73993..9648a4e08f 100644 --- a/src/3rdparty/masm/wtf/OSAllocator.h +++ b/src/3rdparty/masm/wtf/OSAllocator.h @@ -75,7 +75,7 @@ public: static bool canAllocateExecutableMemory(); #if defined(Q_OS_INTEGRITY) - static void setMemoryAttributes(void* addr, bool writable, bool executable); + static void setMemoryAttributes(void* addr, size_t size, bool writable, bool executable); #endif }; diff --git a/src/3rdparty/masm/wtf/OSAllocatorIntegrity.cpp b/src/3rdparty/masm/wtf/OSAllocatorIntegrity.cpp index 7addf9e5c2..27f72073c4 100644 --- a/src/3rdparty/masm/wtf/OSAllocatorIntegrity.cpp +++ b/src/3rdparty/masm/wtf/OSAllocatorIntegrity.cpp @@ -123,10 +123,14 @@ Error setAttributes(MemoryRegion mr, bool writable, bool executable) return SetMemoryRegionAttributes(mr, attributes); } -void OSAllocator::setMemoryAttributes(void* addr, bool writable, bool executable) +void OSAllocator::setMemoryAttributes(void* addr, size_t size, bool writable, bool executable) { - const MRPair* pair = memoryRegionsContainer.getMRPair((Address)addr); - CheckSuccess(setAttributes(pair->vmr, writable, executable)); + Address addressIterator = Address(addr); + for(int i=0; i<(size + ASP_PAGESIZE -1)/ASP_PAGESIZE; i++) { + const MRPair* pair = memoryRegionsContainer.getMRPair(addressIterator); + CheckSuccess(setAttributes(pair->vmr, writable, executable)); + addressIterator += ASP_PAGESIZE; + } } void* OSAllocator::reserveUncommitted(size_t bytes, Usage usage, bool writable, bool executable) @@ -140,9 +144,9 @@ void* OSAllocator::reserveUncommitted(size_t bytes, Usage usage, bool writable, Address addressIterator = virtualStart; for(int i=0; i<(bytes + ASP_PAGESIZE -1)/ASP_PAGESIZE; i++) { MRPair pair; + pair.start = addressIterator; CheckSuccess(SplitMemoryRegion(VMR, ASP_PAGESIZE, &pair.vmr)); CheckSuccess(setAttributes(pair.vmr, writable, executable)); - pair.start = addressIterator; memoryRegionsContainer.insertMRPair(&pair); addressIterator += ASP_PAGESIZE; -- cgit v1.2.3 From 777cd6e9b10c83f5826fde67ad00a85c978218c0 Mon Sep 17 00:00:00 2001 From: Liang Qi Date: Sun, 30 Sep 2018 20:40:42 +0200 Subject: Add QT_CONFIG(filesystemwatcher) guard for include This is needed by 555a6b5d in qtbase. Change-Id: I4c8379c2db704f7d7f2f9a933b062285dd9d8e26 Reviewed-by: Friedemann Kleint Reviewed-by: Edward Welbourne --- src/imports/folderlistmodel/fileinfothread_p.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/imports/folderlistmodel/fileinfothread_p.h b/src/imports/folderlistmodel/fileinfothread_p.h index 67d2a1f5f7..438dea6faa 100644 --- a/src/imports/folderlistmodel/fileinfothread_p.h +++ b/src/imports/folderlistmodel/fileinfothread_p.h @@ -54,7 +54,9 @@ #include #include #include +#if QT_CONFIG(filesystemwatcher) #include +#endif #include #include -- cgit v1.2.3