summaryrefslogtreecommitdiffstats
path: root/src/qml
diff options
context:
space:
mode:
authorYulong Bai <yulong.bai@qt.io>2019-05-21 13:48:52 +0200
committerYulong Bai <yulong.bai@qt.io>2019-06-17 16:19:15 +0200
commite9520ec84c95e10a6826b2289e46552a2d446895 (patch)
tree74b0a97cb50c3608f82b4f3bb2cc00e115164b73 /src/qml
parentc1663865e68d96d4a51351d4d1d2bfa5f313dc18 (diff)
Fix crash caused by objects self-destructions during displacement animationsHEAD5.12
The root cause was that the QAbstractAnimationJob::finished() might delegate its destruction to change.listener->animationFinished(this), and the original author was aware of that and provided a RETURN_IF_DELETE macro to return early if itself got deleted. In the bug's case, change.listener->animationFinished(this) dispatched to QQuickItemViewPrivate::animationFinished() which called QQuickItemViewPrivate::release() and deleted the QAbstractAnimationJob object itself in the end. However, any objects derived from QAbstractAnimationJob, or holding a pointer to a QAbstractAnimationJob, may potentially fall into the code path calling QAbstractAnimationJob::finished(). Any QAnimationJobChangeListener that directly or indirectly deletes QAbstractAnimationJob should be very suspicious to this kind of "heap-use-after-free" bug. Should ensure that the QAbstractAnimationJob won't be referenced after deletion. In the bug's case, within the code path triggered by ListView displacement animation, the other affected classes by QAbstractAnimationJob are: QQuickItemViewFxItem, QQuickItemViewTransitionableItem, QQuickTransitionManager. To fix this, a new SelfDeletable class is factored out to simplify the self-deletion test logic. Any affected classes are made to have a public member m_selfDeletable. Any code paths that finally reach QAbstractAnimationJob::finished() are wrapped with related util macro. Change-Id: Idd33fc3f2d529fd7d8bb088c329101b1e70dd6c0 Task-number: QTBUG-44308 Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
Diffstat (limited to 'src/qml')
-rw-r--r--src/qml/animations/qabstractanimationjob.cpp4
-rw-r--r--src/qml/animations/qabstractanimationjob_p.h3
-rw-r--r--src/qml/animations/qanimationjobutil_p.h32
3 files changed, 28 insertions, 11 deletions
diff --git a/src/qml/animations/qabstractanimationjob.cpp b/src/qml/animations/qabstractanimationjob.cpp
index ece2f0d69..7a784a2b3 100644
--- a/src/qml/animations/qabstractanimationjob.cpp
+++ b/src/qml/animations/qabstractanimationjob.cpp
@@ -263,7 +263,6 @@ QAbstractAnimationJob::QAbstractAnimationJob()
, m_currentLoopStartTime(0)
, m_nextSibling(nullptr)
, m_previousSibling(nullptr)
- , m_wasDeleted(nullptr)
, m_hasRegisteredTimer(false)
, m_isPause(false)
, m_isGroup(false)
@@ -277,9 +276,6 @@ QAbstractAnimationJob::QAbstractAnimationJob()
QAbstractAnimationJob::~QAbstractAnimationJob()
{
- if (m_wasDeleted)
- *m_wasDeleted = true;
-
//we can't call stop here. Otherwise we get pure virtual calls
if (m_state != Stopped) {
State oldState = m_state;
diff --git a/src/qml/animations/qabstractanimationjob_p.h b/src/qml/animations/qabstractanimationjob_p.h
index 0be6ca96e..d046ce9de 100644
--- a/src/qml/animations/qabstractanimationjob_p.h
+++ b/src/qml/animations/qabstractanimationjob_p.h
@@ -52,6 +52,7 @@
//
#include <private/qtqmlglobal_p.h>
+#include <private/qanimationjobutil_p.h>
#include <QtCore/QObject>
#include <QtCore/private/qabstractanimation_p.h>
#include <vector>
@@ -130,6 +131,7 @@ public:
bool isRenderThreadJob() const { return m_isRenderThreadJob; }
bool isRenderThreadProxy() const { return m_isRenderThreadProxy; }
+ SelfDeletable m_selfDeletable;
protected:
virtual void updateCurrentTime(int) {}
virtual void updateState(QAbstractAnimationJob::State newState, QAbstractAnimationJob::State oldState);
@@ -174,7 +176,6 @@ protected:
QAbstractAnimationJob *m_previousSibling;
QQmlAnimationTimer *m_timer = nullptr;
- bool *m_wasDeleted;
bool m_hasRegisteredTimer:1;
bool m_isPause:1;
bool m_isGroup:1;
diff --git a/src/qml/animations/qanimationjobutil_p.h b/src/qml/animations/qanimationjobutil_p.h
index e3d6fe917..83cf3b246 100644
--- a/src/qml/animations/qanimationjobutil_p.h
+++ b/src/qml/animations/qanimationjobutil_p.h
@@ -51,20 +51,40 @@
// We mean it.
//
+#include <type_traits>
+
QT_REQUIRE_CONFIG(qml_animation);
-#define RETURN_IF_DELETED(func) \
+// SelfDeletable is used for self-destruction detection along with
+// ACTION_IF_DELETED and RETURN_IF_DELETED macros. While using, the objects
+// under test should have a member m_selfDeletable of type SelfDeletable
+struct SelfDeletable {
+ ~SelfDeletable() {
+ if (m_wasDeleted)
+ *m_wasDeleted = true;
+ }
+ bool *m_wasDeleted = nullptr;
+};
+
+// \param p pointer to object under test, which should have a member m_selfDeletable of type SelfDeletable
+// \param func statements or functions that to be executed under test.
+// \param action post process if p was deleted under test.
+#define ACTION_IF_DELETED(p, func, action) \
{ \
- bool *prevWasDeleted = m_wasDeleted; \
+ static_assert(std::is_same<decltype((p)->m_selfDeletable), SelfDeletable>::value, "m_selfDeletable must be SelfDeletable");\
+ bool *prevWasDeleted = (p)->m_selfDeletable.m_wasDeleted; \
bool wasDeleted = false; \
- m_wasDeleted = &wasDeleted; \
- func; \
+ (p)->m_selfDeletable.m_wasDeleted = &wasDeleted; \
+ {func;} \
if (wasDeleted) { \
if (prevWasDeleted) \
*prevWasDeleted = true; \
- return; \
+ {action;} \
} \
- m_wasDeleted = prevWasDeleted; \
+ (p)->m_selfDeletable.m_wasDeleted = prevWasDeleted; \
}
+#define RETURN_IF_DELETED(func) \
+ACTION_IF_DELETED(this, func, return)
+
#endif // QANIMATIONJOBUTIL_P_H