From dd264cc9c00e09eb995f1c6f9b1fd651f1d4b2f4 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Wed, 5 Jan 2022 08:41:06 +0100 Subject: [PATCH] QGraphicsView: remove (last remaining?) user of QEvent::op= Instead of aggregating a QMutableSinglePointEvent by value, which depends on the QEvent copy assignment operator, and casting it to a QMouseEvent that it isn't, introduce QEventStorage which is a bit like std::optional, but, by each event subclass befriending it, can store event copies by-value, unlike clone(), which is restricted to the heap. We could have befriended std::optional, too, but by adding our own type in _p.h, we can better control which code uses this dangerous construct. Added a guard to avoid clobbering lastMouseEvent with a copy of itself in storeMouseEvent(). Before, we'd self-assign to lastMouseEvent, which didn't invalidate the reference. Now, a store() is the equivalent of a dtor + copy constructor, so we need to be a bit more careful. Fixes UBSan reports when running tst_qgraphicsview: qgraphicsview/qgraphicsview.cpp:612:27: runtime error: downcast of address 0x61a000035e90 which does not point to an object of type 'QMouseEvent' 0x61a000035e90: note: object is of type 'QMutableSinglePointEvent' 00 00 00 00 30 47 ef 8b 99 7f 00 00 02 00 00 00 00 00 00 e0 d0 91 00 00 20 60 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'QMutableSinglePointEvent' #0 0x7f99a27c7a47 in QGraphicsViewPrivate::replayLastMouseEvent() qgraphicsview.cpp:612 qgraphicsview.cpp:653:39: runtime error: member call on address 0x61a0000fe290 which does not point to an object of type 'QMouseEvent' 0x61a0000fe290: note: object is of type 'QMutableSinglePointEvent' 00 00 00 00 30 47 ef 8b 99 7f 00 00 05 00 00 00 00 00 00 e0 d0 91 00 00 20 60 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'QMutableSinglePointEvent' #0 0x7f99a27c3609 in QGraphicsViewPrivate::mouseMoveEventHandler(QMouseEvent*) qgraphicsview.cpp:653 #1 0x7f99a27c7832 in QGraphicsViewPrivate::replayLastMouseEvent() qgraphicsview.cpp:612 qgraphicsview.cpp:654:37: runtime error: member call on address 0x61a0000fe290 which does not point to an object of type 'QMouseEvent' 0x61a0000fe290: note: object is of type 'QMutableSinglePointEvent' 00 00 00 00 30 47 ef 8b 99 7f 00 00 05 00 00 00 00 00 00 e0 d0 91 00 00 20 60 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'QMutableSinglePointEvent' #0 0x7f99a27c358b in QGraphicsViewPrivate::mouseMoveEventHandler(QMouseEvent*) qgraphicsview.cpp:654 #1 0x7f99a27c7832 in QGraphicsViewPrivate::replayLastMouseEvent() qgraphicsview.cpp:612 Pick-to: 6.9 Task-number: QTBUG-99563 Change-Id: Ib642d416b8aef98c7fd8b1fa164ec2449189992a Reviewed-by: Axel Spoerl Reviewed-by: Volker Hilsheimer Reviewed-by: Shawn Rutledge --- src/corelib/kernel/qcoreevent.h | 3 + src/gui/kernel/qevent_p.h | 94 ++++++++++++++++++++++ src/widgets/graphicsview/qgraphicsview.cpp | 24 +++--- src/widgets/graphicsview/qgraphicsview_p.h | 2 +- 4 files changed, 110 insertions(+), 13 deletions(-) diff --git a/src/corelib/kernel/qcoreevent.h b/src/corelib/kernel/qcoreevent.h index a6ecbb44527..50367efd515 100644 --- a/src/corelib/kernel/qcoreevent.h +++ b/src/corelib/kernel/qcoreevent.h @@ -11,6 +11,8 @@ QT_BEGIN_NAMESPACE +template class QEventStorage; + #define Q_EVENT_DISABLE_COPY(Class) \ protected: \ Class(const Class &) = default; \ @@ -19,6 +21,7 @@ protected: \ Class &operator=(Class &&) = delete #define Q_DECL_EVENT_COMMON(Class) \ + friend class QEventStorage; \ protected: \ Class(const Class &); \ Class(Class &&) = delete; \ diff --git a/src/gui/kernel/qevent_p.h b/src/gui/kernel/qevent_p.h index 3c185c6ba24..bd34ebc61bf 100644 --- a/src/gui/kernel/qevent_p.h +++ b/src/gui/kernel/qevent_p.h @@ -24,6 +24,100 @@ QT_BEGIN_NAMESPACE class QPointingDevice; +/*! + \internal + \since 6.9 + + This class provides a way to store copies of QEvents. QEvents can otherwise + only be copied by \l{QEvent::clone()}{cloning}, which allocates the copy on + the heap. By befriending concrete QEvent subclasses, QEventStorage gains + access to their protected copy constructors. + + It is similar to std::optional, but with a more targeted API. + + Note that by storing an event in QEventStorage, it is always sliced. +*/ +template +class QEventStorage +{ + Q_DISABLE_COPY_MOVE(QEventStorage) + static_assert(std::is_base_of_v); + union { + char m_eventNotSet; // could be std::monostate, but don't want to pay for include + Event m_event; + }; + bool m_engaged = false; + + void engage(const Event &e) + { + Q_ASSERT(!m_engaged); + new (&m_event) Event(e); + m_engaged = true; + } + + void disengage() + { + Q_ASSERT(m_engaged); + m_event.~Event(); + m_engaged = false; + } + +public: + QEventStorage() noexcept : m_eventNotSet{} {} + explicit QEventStorage(const Event &e) + { + engage(e); + } + + ~QEventStorage() + { + if (m_engaged) + disengage(); + } + + explicit operator bool() const noexcept { return m_engaged; } + bool operator!() const noexcept { return !m_engaged; } + + Event &store(const Event &e) + { + if (m_engaged) + disengage(); + engage(e); + return m_event; + } + + Event &storeUnlessAlias(const Event &e) + { + if (m_engaged && &e == &m_event) + return m_event; + return store(e); + } + + const Event &operator*() const + { + Q_PRE(m_engaged); + return m_event; + } + + Event &operator*() + { + Q_PRE(m_engaged); + return m_event; + } + + const Event *operator->() const + { + Q_PRE(m_engaged); + return &m_event; + } + + Event *operator->() + { + Q_PRE(m_engaged); + return &m_event; + } +}; + class Q_GUI_EXPORT QMutableTouchEvent : public QTouchEvent { public: diff --git a/src/widgets/graphicsview/qgraphicsview.cpp b/src/widgets/graphicsview/qgraphicsview.cpp index 9dae651284f..af113432ba2 100644 --- a/src/widgets/graphicsview/qgraphicsview.cpp +++ b/src/widgets/graphicsview/qgraphicsview.cpp @@ -608,8 +608,7 @@ void QGraphicsViewPrivate::replayLastMouseEvent() { if (!useLastMouseEvent || !scene) return; - QSinglePointEvent *spe = static_cast(&lastMouseEvent); - mouseMoveEventHandler(static_cast(spe)); + mouseMoveEventHandler(&*lastMouseEvent); } /*! @@ -618,7 +617,8 @@ void QGraphicsViewPrivate::replayLastMouseEvent() void QGraphicsViewPrivate::storeMouseEvent(QMouseEvent *event) { useLastMouseEvent = true; - lastMouseEvent = *event; + // *event may alias *lastMouseEvent + lastMouseEvent.storeUnlessAlias(*event); } void QGraphicsViewPrivate::mouseMoveEventHandler(QMouseEvent *event) @@ -630,7 +630,7 @@ void QGraphicsViewPrivate::mouseMoveEventHandler(QMouseEvent *event) #endif storeMouseEvent(event); - lastMouseEvent.setAccepted(false); + lastMouseEvent->setAccepted(false); if (!sceneInteractionAllowed) return; @@ -662,7 +662,7 @@ void QGraphicsViewPrivate::mouseMoveEventHandler(QMouseEvent *event) QCoreApplication::sendEvent(scene, &mouseEvent); // Remember whether the last event was accepted or not. - lastMouseEvent.setAccepted(mouseEvent.isAccepted()); + lastMouseEvent->setAccepted(mouseEvent.isAccepted()); if (mouseEvent.isAccepted() && mouseEvent.buttons() != 0) { // The event was delivered to a mouse grabber; the press is likely to @@ -819,7 +819,7 @@ void QGraphicsViewPrivate::_q_setViewportCursor(const QCursor &cursor) void QGraphicsViewPrivate::_q_unsetViewportCursor() { Q_Q(QGraphicsView); - const auto items = q->items(lastMouseEvent.position().toPoint()); + const auto items = q->items(lastMouseEvent->position().toPoint()); for (QGraphicsItem *item : items) { if (item->isEnabled() && item->hasCursor()) { _q_setViewportCursor(item->cursor()); @@ -3183,7 +3183,7 @@ void QGraphicsView::mouseDoubleClickEvent(QMouseEvent *event) event->setAccepted(isAccepted); // Update the last mouse event accepted state. - d->lastMouseEvent.setAccepted(isAccepted); + d->lastMouseEvent->setAccepted(isAccepted); } /*! @@ -3197,7 +3197,7 @@ void QGraphicsView::mousePressEvent(QMouseEvent *event) // scroll-dragging; even in non-interactive mode, scroll hand dragging is // allowed, so we store the event at the very top of this function. d->storeMouseEvent(event); - d->lastMouseEvent.setAccepted(false); + d->lastMouseEvent->setAccepted(false); if (d->sceneInteractionAllowed) { // Store some of the event's button-down data. @@ -3235,7 +3235,7 @@ void QGraphicsView::mousePressEvent(QMouseEvent *event) event->setAccepted(isAccepted); // Update the last mouse event accepted state. - d->lastMouseEvent.setAccepted(isAccepted); + d->lastMouseEvent->setAccepted(isAccepted); if (isAccepted) return; @@ -3284,7 +3284,7 @@ void QGraphicsView::mouseMoveEvent(QMouseEvent *event) if (d->handScrolling) { QScrollBar *hBar = horizontalScrollBar(); QScrollBar *vBar = verticalScrollBar(); - QPoint delta = event->position().toPoint() - d->lastMouseEvent.position().toPoint(); + QPoint delta = event->position().toPoint() - d->lastMouseEvent->position().toPoint(); hBar->setValue(hBar->value() + (isRightToLeft() ? delta.x() : -delta.x())); vBar->setValue(vBar->value() - delta.y()); @@ -3318,7 +3318,7 @@ void QGraphicsView::mouseReleaseEvent(QMouseEvent *event) #endif d->handScrolling = false; - if (d->scene && d->sceneInteractionAllowed && !d->lastMouseEvent.isAccepted() && d->handScrollMotions <= 6) { + if (d->scene && d->sceneInteractionAllowed && !d->lastMouseEvent->isAccepted() && d->handScrollMotions <= 6) { // If we've detected very little motion during the hand drag, and // no item accepted the last event, we'll interpret that as a // click to the scene, and reset the selection. @@ -3355,7 +3355,7 @@ void QGraphicsView::mouseReleaseEvent(QMouseEvent *event) QCoreApplication::sendEvent(d->scene, &mouseEvent); // Update the last and current mouse events' accepted state. - d->lastMouseEvent.setAccepted(mouseEvent.isAccepted()); + d->lastMouseEvent->setAccepted(mouseEvent.isAccepted()); event->setAccepted(mouseEvent.isAccepted()); #ifndef QT_NO_CURSOR diff --git a/src/widgets/graphicsview/qgraphicsview_p.h b/src/widgets/graphicsview/qgraphicsview_p.h index 0a4699c9ca5..83448c13298 100644 --- a/src/widgets/graphicsview/qgraphicsview_p.h +++ b/src/widgets/graphicsview/qgraphicsview_p.h @@ -89,7 +89,7 @@ public: qreal topIndent; // Replaying mouse events - QMutableSinglePointEvent lastMouseEvent; + QEventStorage lastMouseEvent; void replayLastMouseEvent(); void storeMouseEvent(QMouseEvent *event); void mouseMoveEventHandler(QMouseEvent *event);