Fix the double-free issues related to QGrpcOperation primitives

In the recent implementation the std::shared_ptr takes the real
ownership over the Qt GRPC channel primitives such as QGrpcOperation,
QGrpcStream, and QGrpcReply. Meanwhile these primitives had the QObject
parent-children relationships with the classes derived of the
QAbstractGrpcClient. This led to the floating double free issue when
Qt GC removed the primitives before the std::shared_ptr did.

Break the parent-child linking between the Qt GRPC channel primitives
and QAbstractGrpcClient. Cleanup the code parts that suppressed the
issue - the use of deleteLater instead of calling destructor directly.

Pick-to: 6.5
Change-Id: I9c6562335020e45ddb8e440311ec119fc90777f8
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

src/grpc/qgrpccallreply.cpp

@@ -50,9 +50,7 @@ using namespace Qt::StringLiterals;
     \endcode
 */
 
-QGrpcCallReply::QGrpcCallReply(QAbstractGrpcClient *parent) : QGrpcOperation(parent)
-{
-}
+QGrpcCallReply::QGrpcCallReply(QAbstractGrpcClient *client) : QGrpcOperation(client) { }
 
 QGrpcCallReply::~QGrpcCallReply() = default;
 

src/grpc/qgrpccallreply.h

@@ -16,7 +16,7 @@ class Q_GRPC_EXPORT QGrpcCallReply final : public QGrpcOperation
     Q_OBJECT
 
 public:
-    explicit QGrpcCallReply(QAbstractGrpcClient *parent);
+    explicit QGrpcCallReply(QAbstractGrpcClient *client);
     ~QGrpcCallReply() override;
 
     void abort() override;
@@ -40,7 +40,10 @@ public:
     }
 
 private:
+    QGrpcCallReply();
     Q_DISABLE_COPY_MOVE(QGrpcCallReply)
+
+    friend class QAbstractGrpcClient;
 };
 
 QT_END_NAMESPACE

src/grpc/qgrpcchannel.cpp

@@ -77,8 +77,7 @@ static QByteArray buildRpcName(QLatin1StringView service, QLatin1StringView meth
 }
 
 QGrpcChannelStream::QGrpcChannelStream(grpc::Channel *channel, QLatin1StringView method,
-                                       QByteArrayView data, QObject *parent)
-    : QObject(parent)
+                                       QByteArrayView data)
 {
     grpc::ByteBuffer request = parseQByteArray(data);
 
@@ -132,8 +131,7 @@ void QGrpcChannelStream::cancel()
 }
 
 QGrpcChannelCall::QGrpcChannelCall(grpc::Channel *channel, QLatin1StringView method,
-                                   QByteArrayView data, QObject *parent)
-    : QObject(parent)
+                                   QByteArrayView data)
 {
     grpc::ByteBuffer request = parseQByteArray(data);
     thread = QThread::create([this, request, channel, method = toStdString(method)] {
@@ -218,12 +216,9 @@ std::shared_ptr<QGrpcCallReply> QGrpcChannelPrivate::call(QAbstractGrpcClient *c
                                                           QByteArrayView args)
 {
     const QByteArray rpcName = buildRpcName(service, method);
-    std::shared_ptr<QGrpcCallReply> reply(new QGrpcCallReply(client),
-                                          [](QGrpcCallReply *reply) { reply->deleteLater(); });
+    std::shared_ptr<QGrpcCallReply> reply(new QGrpcCallReply(client));
     QSharedPointer<QGrpcChannelCall> call(new QGrpcChannelCall(m_channel.get(),
-                                                               QLatin1StringView(rpcName), args,
-                                                               reply.get()),
-                                          &QGrpcChannelCall::deleteLater);
+                                                               QLatin1StringView(rpcName), args));
     auto connection = std::make_shared<QMetaObject::Connection>();
     auto abortConnection = std::make_shared<QMetaObject::Connection>();
 
@@ -271,10 +266,8 @@ void QGrpcChannelPrivate::stream(QGrpcStream *stream, QLatin1StringView service)
     Q_ASSERT(stream != nullptr);
     const QByteArray rpcName = buildRpcName(service, stream->method());
 
-    QSharedPointer<QGrpcChannelStream> sub(new QGrpcChannelStream(m_channel.get(),
-                                                                  QLatin1StringView(rpcName),
-                                                                  stream->arg(), stream),
-                                           &QGrpcChannelStream::deleteLater);
+    QSharedPointer<QGrpcChannelStream> sub(
+            new QGrpcChannelStream(m_channel.get(), QLatin1StringView(rpcName), stream->arg()));
 
     auto abortConnection = std::make_shared<QMetaObject::Connection>();
     auto readConnection = std::make_shared<QMetaObject::Connection>();

src/grpc/qgrpcchannel_p.h

@@ -41,7 +41,7 @@ class QGrpcChannelStream : public QObject
 
 public:
     explicit QGrpcChannelStream(grpc::Channel *channel, QLatin1StringView method,
-                                QByteArrayView data, QObject *parent = nullptr);
+                                QByteArrayView data);
     ~QGrpcChannelStream() override;
 
     void cancel();
@@ -65,8 +65,8 @@ class QGrpcChannelCall : public QObject
     Q_OBJECT
 
 public:
-    explicit QGrpcChannelCall(grpc::Channel *channel, QLatin1StringView method, QByteArrayView data,
-                              QObject *parent = nullptr);
+    explicit QGrpcChannelCall(grpc::Channel *channel, QLatin1StringView method,
+                              QByteArrayView data);
     ~QGrpcChannelCall() override;
 
     void cancel();

src/grpc/qgrpcoperation.cpp

@@ -4,6 +4,9 @@
 
 #include <qtgrpcglobal_p.h>
 
+#include <QtCore/qpointer.h>
+#include <QtCore/private/qobject_p.h>
+
 #include "qgrpcoperation.h"
 
 QT_BEGIN_NAMESPACE
@@ -42,7 +45,17 @@ QT_BEGIN_NAMESPACE
     or during serialization.
 */
 
-QGrpcOperation::QGrpcOperation(QAbstractGrpcClient *parent) : QObject(parent)
+class QGrpcOperationPrivate : public QObjectPrivate
+{
+    Q_DECLARE_PUBLIC(QGrpcOperation)
+public:
+    QGrpcOperationPrivate(QAbstractGrpcClient *_client) : client(_client) { }
+    QPointer<QAbstractGrpcClient> client;
+    QByteArray data;
+};
+
+QGrpcOperation::QGrpcOperation(QAbstractGrpcClient *client)
+    : QObject(*new QGrpcOperationPrivate(client))
 {
 }
 
@@ -56,7 +69,8 @@ QGrpcOperation::~QGrpcOperation() = default;
  */
 void QGrpcOperation::setData(const QByteArray &data)
 {
-    m_data = data;
+    Q_D(QGrpcOperation);
+    d->data = data;
 }
 
 /*!
@@ -67,7 +81,27 @@ void QGrpcOperation::setData(const QByteArray &data)
 */
 void QGrpcOperation::setData(QByteArray &&data)
 {
-    m_data = std::move(data);
+    Q_D(QGrpcOperation);
+    d->data = std::move(data);
+}
+
+/*!
+    \internal
+    Getter of the data received from the channel.
+*/
+QByteArray QGrpcOperation::data() const
+{
+    return d_func()->data;
+}
+
+/*!
+    \internal
+    Allows access to the client that is the formal owner of this
+    QGrpcOperation.
+*/
+QAbstractGrpcClient *QGrpcOperation::client() const
+{
+    return d_func()->client.get();
 }
 
 QT_END_NAMESPACE

src/grpc/qgrpcoperation.h

@@ -11,6 +11,7 @@
 
 QT_BEGIN_NAMESPACE
 
+class QGrpcOperationPrivate;
 class Q_GRPC_EXPORT QGrpcOperation : public QObject
 {
     Q_OBJECT
@@ -20,9 +21,8 @@ public:
     T read() const
     {
         T value;
-        auto client = qobject_cast<QAbstractGrpcClient *>(parent());
-        if (client)
-            client->tryDeserialize(&value, m_data);
+        if (client())
+            client()->tryDeserialize(&value, data());
         return value;
     }
 
@@ -36,13 +36,18 @@ Q_SIGNALS:
     void errorOccurred(const QGrpcStatus &status);
 
 protected:
-    explicit QGrpcOperation(QAbstractGrpcClient *parent);
+    explicit QGrpcOperation(QAbstractGrpcClient *client);
     ~QGrpcOperation() override;
 
 private:
     Q_DISABLE_COPY_MOVE(QGrpcOperation)
 
-    QByteArray m_data;
+    friend class QAbstractGrpcClient;
+
+    QByteArray data() const;
+    QAbstractGrpcClient *client() const;
+
+    Q_DECLARE_PRIVATE(QGrpcOperation)
 };
 
 QT_END_NAMESPACE

src/grpc/qgrpcstream.cpp

@@ -24,8 +24,8 @@ QT_BEGIN_NAMESPACE
 */
 
 QGrpcStream::QGrpcStream(QLatin1StringView method, QByteArrayView arg, const StreamHandler &handler,
-                         QAbstractGrpcClient *parent)
-    : QGrpcOperation(parent), m_method(method.data(), method.size()), m_arg(arg.toByteArray())
+                         QAbstractGrpcClient *client)
+    : QGrpcOperation(client), m_method(method.data(), method.size()), m_arg(arg.toByteArray())
 {
     if (handler)
         m_handlers.push_back(handler);

src/grpc/qgrpcstream.h

@@ -24,6 +24,8 @@ class Q_GRPC_EXPORT QGrpcStream final : public QGrpcOperation
     using StreamHandler = std::function<void(const QByteArray &)>;
 
 public:
+    ~QGrpcStream() override;
+
     void abort() override;
 
     QLatin1StringView method() const;
@@ -35,8 +37,7 @@ Q_SIGNALS:
 
 protected:
     explicit QGrpcStream(QLatin1StringView method, QByteArrayView arg, const StreamHandler &handler,
-                         QAbstractGrpcClient *parent);
-    ~QGrpcStream() override;
+                         QAbstractGrpcClient *client);
 
     void addHandler(const StreamHandler &handler);
 

tests/auto/grpc/client/tst_grpc_client.cpp

@@ -172,7 +172,6 @@ void QtGrpcClientTest::CallAndAsyncRecieveWithSubscribeStringTest()
     std::shared_ptr<QGrpcCallReply> reply = _client->testMethod(request);
     reply->subscribe(this, [reply, &result, &waiter] {
         result = reply->read<SimpleStringMessage>();
-        reply->deleteLater();
         waiter.quit();
     });
 
@@ -204,10 +203,8 @@ void QtGrpcClientTest::CallAndAsyncImmediateAbortStringTest()
     std::shared_ptr<QGrpcCallReply> reply = _client->testMethod(request);
 
     result.setTestFieldString("Result not changed by echo");
-    QObject::connect(reply.get(), &QGrpcCallReply::finished, this, [&result, reply] {
-        result = reply->read<SimpleStringMessage>();
-        reply->deleteLater();
-    });
+    QObject::connect(reply.get(), &QGrpcCallReply::finished, this,
+                     [&result, reply] { result = reply->read<SimpleStringMessage>(); });
 
     QSignalSpy replyErrorSpy(reply.get(), &QGrpcCallReply::errorOccurred);
     QSignalSpy clientErrorSpy(_client.get(), &TestService::Client::errorOccurred);