Debounce QDeferredDeleteEvents in QObject::deleteLater()

We used to look through the event queue in QCoreApplication::postEvent,
and if we found an existing DeferredDelete event for the receiver we
would compress the two events into one.

This was changed in 99b89d30fa5484c5d1f3cbda828648c28af4fb7d, as the
logic was causing O(n^2) for deleteLater, by using one of the bits
in QObjectData to track whether the object had already been deleted.

But it kept the logic for tracking this in QCoreApplication::postEvent,
and QCoreApplication::compressEvent would still do the work of deleting
the additional QDeferredDeleteEvents.

To avoid the unnecessary heap allocation of the QDeferredDeleteEvents
we can move the debouncing/compression to QObject::deleteLater().
We use the same mutex as in QCoreApplication::postEvent to guard
concurrent access to deleteLaterCalled.

A note has been added about the (preexisting) issue that the mutex
is not sufficient to prevent data races, as the deleteLaterCalled
flag is part of a bit-field, and we're not guarding any of our
other accesses to other bits.

As QDeferredDeleteEvents is private API, we can rely on no-one else
posting it than QObject::deleteLater(), which should be the case now
that tst_QApplication::sendPostedEvents() was fixed.

The documentation has been clarified as well. It's safe to call
deleteLater() more than once, but that's not _because_ other
pending events for the object are cleared. The latter behavior
is normal ~QObject() behavior. The documentation was probably
written at a point we didn't do any event compression at all
for QDeferredDeleteEvents.

Task-number: QTBUG-120124
Task-number: QTBUG-119918
Change-Id: I2a733095b7cb066ba494b1335aa40200c749cb0c
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

3 files changed, 19 insertions, 16 deletions

diff --git a/src/corelib/kernel/qcoreapplication.cpp b/src/corelib/kernel/qcoreapplication.cpp
index 2752697dba..b53f8eafba 100644
--- a/src/corelib/kernel/qcoreapplication.cpp
+++ b/src/corelib/kernel/qcoreapplication.cpp
@@ -1677,9 +1677,6 @@ void QCoreApplication::postEvent(QObject *receiver, QEvent *event, int priority)
         return;
     }
 
-    if (event->type() == QEvent::DeferredDelete)
-        receiver->d_ptr->deleteLaterCalled = true;
-
     if (event->type() == QEvent::DeferredDelete && data == QThreadData::current()) {
         // remember the current running eventloop for DeferredDelete
         // events posted in the receiver's thread.
@@ -1750,17 +1747,6 @@ bool QCoreApplication::compressEvent(QEvent *event, QObject *receiver, QPostEven
         return false;
     }
 
-    if (event->type() == QEvent::DeferredDelete) {
-        if (receiver->d_ptr->deleteLaterCalled) {
-            // there was a previous DeferredDelete event, so we can drop the new one
-            delete event;
-            return true;
-        }
-        // deleteLaterCalled is set to true in postedEvents when queueing the very first
-        // deferred deletion event.
-        return false;
-    }
-
     if (event->type() == QEvent::Quit && receiverPostedEvents > 0) {
         for (const QPostEvent &cur : std::as_const(*postedEvents)) {
             if (cur.receiver != receiver
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index d8183a9d1c..0cee785f3b 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -2425,6 +2425,24 @@ void QObject::deleteLater()
     if (qApp == this)
         qWarning("You are deferring the delete of QCoreApplication, this may not work as expected.");
 #endif
+
+    {
+        // De-bounce QDeferredDeleteEvents. Use the post event list mutex
+        // to guard access to deleteLaterCalled, so we don't need a separate
+        // mutex in QObjectData.
+        auto locker = QCoreApplicationPrivate::lockThreadPostEventList(this);
+
+        // FIXME: The deleteLaterCalled flag is part of a bit field,
+        // so we likely have data races here, even with the mutex above,
+        // as long as we're not guarding every access to the bit field.
+
+        Q_D(QObject);
+        if (d->deleteLaterCalled)
+            return;
+
+        d->deleteLaterCalled = true;
+    }
+
     QCoreApplication::postEvent(this, new QDeferredDeleteEvent());
 }
 
diff --git a/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp b/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp
index 914d196896..e26152439a 100644
--- a/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp
+++ b/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp
@@ -19,7 +19,6 @@
 #if QT_CONFIG(process)
 # include <QtCore/QProcess>
 #endif
-#include <QtCore/private/qcoreevent_p.h>
 #include <QtCore/private/qeventloop_p.h>
 
 #include <QtGui/QFontDatabase>
@@ -1166,7 +1165,7 @@ void SendPostedEventsTester::doTest()
     QPointer<SendPostedEventsTester> p = this;
     QApplication::postEvent(this, new QEvent(QEvent::User));
     // DeferredDelete should not be delivered until returning from this function
-    QApplication::postEvent(this, new QDeferredDeleteEvent());
+    deleteLater();
 
     QEventLoop eventLoop;
     QMetaObject::invokeMethod(&eventLoop, "quit", Qt::QueuedConnection);