Refactoring: removing m_reply from GeoReply classes

This patch tries to simplify the code removing the contained m_reply
from all the Geo[Map,Route,Geocode,Places]Reply classes.
The need for m_reply was associated to the "abort" method, but this
can be solved by emitting a signal in the superclass abort() method,
and connecting that to QNetworkReply::abort() in the constructor.

Since QNetworkReplyHttpImpl always sends an OperationCanceledError
it should then be safe to call deleteLater() on the network reply
in the slot connected to QNetworkReply::error

This patch also prevents the series of
"QCoreApplication::postEvent: Unexpected null receiver" warnings
that are generated due to deletingLater already deleted objects
(abort() emits an error, the reply is destroyed inside the
onError slot, but also in the abort() method).

Finally, this patch removes the setFinished() call in
QGeoRouteReply::abort() since the documentation does not mention
this, and all the subclasses do not perform this operation and
emit the corresponding signal.
tst_qgeoroutereply has been adapted accordingly.

Change-Id: I226ee163e7bed784dd7f0da1522e651459543bca
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

9 files changed, 74 insertions, 160 deletions

diff --git a/src/plugins/geoservices/osm/qgeocodereplyosm.cpp b/src/plugins/geoservices/osm/qgeocodereplyosm.cpp
index 15b1724e..a30601d0 100644
--- a/src/plugins/geoservices/osm/qgeocodereplyosm.cpp
+++ b/src/plugins/geoservices/osm/qgeocodereplyosm.cpp
@@ -50,31 +50,23 @@
 QT_BEGIN_NAMESPACE
 
 QGeoCodeReplyOsm::QGeoCodeReplyOsm(QNetworkReply *reply, QObject *parent)
-:   QGeoCodeReply(parent), m_reply(reply)
+:   QGeoCodeReply(parent)
 {
-    connect(m_reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
-    connect(m_reply, SIGNAL(error(QNetworkReply::NetworkError)),
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
+        return;
+    }
+    connect(reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)),
             this, SLOT(networkReplyError(QNetworkReply::NetworkError)));
-
+    connect(this, &QGeoCodeReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
     setLimit(1);
     setOffset(0);
 }
 
 QGeoCodeReplyOsm::~QGeoCodeReplyOsm()
 {
-    if (m_reply)
-        m_reply->deleteLater();
-}
-
-void QGeoCodeReplyOsm::abort()
-{
-    if (!m_reply)
-        return;
-
-    m_reply->abort();
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 static QGeoAddress parseAddressObject(const QJsonObject &object)
@@ -108,14 +100,14 @@ static QGeoAddress parseAddressObject(const QJsonObject &object)
 
 void QGeoCodeReplyOsm::networkReplyFinished()
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (m_reply->error() != QNetworkReply::NoError)
+    if (reply->error() != QNetworkReply::NoError)
         return;
 
     QList<QGeoLocation> locations;
-    QJsonDocument document = QJsonDocument::fromJson(m_reply->readAll());
+    QJsonDocument document = QJsonDocument::fromJson(reply->readAll());
 
     if (document.isObject()) {
         QJsonObject object = document.object();
@@ -169,22 +161,14 @@ void QGeoCodeReplyOsm::networkReplyFinished()
 
     setLocations(locations);
     setFinished(true);
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoCodeReplyOsm::networkReplyError(QNetworkReply::NetworkError error)
 {
     Q_UNUSED(error)
-
-    if (!m_reply)
-        return;
-
-    setError(QGeoCodeReply::CommunicationError, m_reply->errorString());
-
-    m_reply->deleteLater();
-    m_reply = 0;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    setError(QGeoCodeReply::CommunicationError, reply->errorString());
 }
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/osm/qgeocodereplyosm.h b/src/plugins/geoservices/osm/qgeocodereplyosm.h
index 2772910e..0847f58c 100644
--- a/src/plugins/geoservices/osm/qgeocodereplyosm.h
+++ b/src/plugins/geoservices/osm/qgeocodereplyosm.h
@@ -53,14 +53,9 @@ public:
     explicit QGeoCodeReplyOsm(QNetworkReply *reply, QObject *parent = 0);
     ~QGeoCodeReplyOsm();
 
-    void abort();
-
 private Q_SLOTS:
     void networkReplyFinished();
     void networkReplyError(QNetworkReply::NetworkError error);
-
-private:
-    QNetworkReply *m_reply;
 };
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/osm/qgeomapreplyosm.cpp b/src/plugins/geoservices/osm/qgeomapreplyosm.cpp
index 052ed351..a06f91f3 100644
--- a/src/plugins/geoservices/osm/qgeomapreplyosm.cpp
+++ b/src/plugins/geoservices/osm/qgeomapreplyosm.cpp
@@ -45,64 +45,45 @@ QGeoMapReplyOsm::QGeoMapReplyOsm(QNetworkReply *reply,
                                  const QGeoTileSpec &spec,
                                  const QString &imageFormat,
                                  QObject *parent)
-:   QGeoTiledMapReply(spec, parent), m_reply(reply)
+:   QGeoTiledMapReply(spec, parent)
 {
-    connect(m_reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
-    connect(m_reply, SIGNAL(error(QNetworkReply::NetworkError)),
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
+        return;
+    }
+    connect(reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)),
             this, SLOT(networkReplyError(QNetworkReply::NetworkError)));
+    connect(this, &QGeoTiledMapReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
     setMapImageFormat(imageFormat);
 }
 
 QGeoMapReplyOsm::~QGeoMapReplyOsm()
 {
-    if (m_reply) {
-        m_reply->deleteLater();
-        m_reply = 0;
-    }
-}
-
-void QGeoMapReplyOsm::abort()
-{
-    if (!m_reply)
-        return;
-
-    m_reply->abort();
-}
-
-QNetworkReply *QGeoMapReplyOsm::networkReply() const
-{
-    return m_reply;
 }
 
 void QGeoMapReplyOsm::networkReplyFinished()
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (m_reply->error() != QNetworkReply::NoError) {
-        m_reply->deleteLater();
-        m_reply = 0;
+    if (reply->error() != QNetworkReply::NoError) // Already handled in networkReplyError
         return;
-    }
 
-    QByteArray a = m_reply->readAll();
+    QByteArray a = reply->readAll();
 
     setMapImageData(a);
     setFinished(true);
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoMapReplyOsm::networkReplyError(QNetworkReply::NetworkError error)
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    if (error == QNetworkReply::OperationCanceledError)
+        setFinished(true);
+    else
+        setError(QGeoTiledMapReply::CommunicationError, reply->errorString());
 
-    if (error != QNetworkReply::OperationCanceledError)
-        setError(QGeoTiledMapReply::CommunicationError, m_reply->errorString());
-
-    setFinished(true);
-    m_reply->deleteLater();
-    m_reply = 0;
 }
diff --git a/src/plugins/geoservices/osm/qgeomapreplyosm.h b/src/plugins/geoservices/osm/qgeomapreplyosm.h
index 804a0a24..ef0cbb15 100644
--- a/src/plugins/geoservices/osm/qgeomapreplyosm.h
+++ b/src/plugins/geoservices/osm/qgeomapreplyosm.h
@@ -40,12 +40,8 @@
 #ifndef QGEOMAPREPLYOSM_H
 #define QGEOMAPREPLYOSM_H
 
-#include "qgeotilefetcherosm.h"
-#include "qgeotileproviderosm.h"
-
 #include <QtNetwork/QNetworkReply>
 #include <QtLocation/private/qgeotiledmapreply_p.h>
-#include <QtCore/qpointer.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -57,16 +53,9 @@ public:
     QGeoMapReplyOsm(QNetworkReply *reply, const QGeoTileSpec &spec, const QString &imageFormat, QObject *parent = 0);
     ~QGeoMapReplyOsm();
 
-    void abort();
-
-    QNetworkReply *networkReply() const;
-
 private Q_SLOTS:
     void networkReplyFinished();
     void networkReplyError(QNetworkReply::NetworkError error);
-
-private:
-    QPointer<QNetworkReply> m_reply;
 };
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/osm/qgeoroutereplyosm.cpp b/src/plugins/geoservices/osm/qgeoroutereplyosm.cpp
index da28317f..6924fda7 100644
--- a/src/plugins/geoservices/osm/qgeoroutereplyosm.cpp
+++ b/src/plugins/geoservices/osm/qgeoroutereplyosm.cpp
@@ -44,55 +44,37 @@ QT_BEGIN_NAMESPACE
 
 QGeoRouteReplyOsm::QGeoRouteReplyOsm(QNetworkReply *reply, const QGeoRouteRequest &request,
                                      QObject *parent)
-:   QGeoRouteReply(request, parent), m_reply(reply)
+:   QGeoRouteReply(request, parent)
 {
-    connect(m_reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
-    connect(m_reply, SIGNAL(error(QNetworkReply::NetworkError)),
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
+        return;
+    }
+    connect(reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)),
             this, SLOT(networkReplyError(QNetworkReply::NetworkError)));
+    connect(this, &QGeoRouteReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
 }
 
 QGeoRouteReplyOsm::~QGeoRouteReplyOsm()
 {
-    if (m_reply)
-        m_reply->deleteLater();
-}
-
-void QGeoRouteReplyOsm::abort()
-{
-    if (!m_reply)
-        return;
-
-    m_reply->abort();
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoRouteReplyOsm::networkReplyFinished()
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (m_reply->error() != QNetworkReply::NoError) {
-        setError(QGeoRouteReply::CommunicationError, m_reply->errorString());
-        m_reply->deleteLater();
-        m_reply = 0;
+    if (reply->error() != QNetworkReply::NoError)
         return;
-    }
-
-    if (m_reply->error() != QNetworkReply::NoError) {
-        setError(QGeoRouteReply::CommunicationError, m_reply->errorString());
-        m_reply->deleteLater();
-        m_reply = 0;
-        return;
-    }
 
     QGeoRoutingManagerEngineOsm *engine = qobject_cast<QGeoRoutingManagerEngineOsm *>(parent());
     const QGeoRouteParser *parser = engine->routeParser();
 
     QList<QGeoRoute> routes;
     QString errorString;
-    QGeoRouteReply::Error error = parser->parseReply(routes, errorString, m_reply->readAll());
+    QGeoRouteReply::Error error = parser->parseReply(routes, errorString, reply->readAll());
 
     if (error == QGeoRouteReply::NoError) {
         setRoutes(routes.mid(0,1)); // TODO QTBUG-56426
@@ -101,22 +83,14 @@ void QGeoRouteReplyOsm::networkReplyFinished()
     } else {
         setError(error, errorString);
     }
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoRouteReplyOsm::networkReplyError(QNetworkReply::NetworkError error)
 {
     Q_UNUSED(error)
-
-    if (!m_reply)
-        return;
-
-    setError(QGeoRouteReply::CommunicationError, m_reply->errorString());
-
-    m_reply->deleteLater();
-    m_reply = 0;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    setError(QGeoRouteReply::CommunicationError, reply->errorString());
 }
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/osm/qgeoroutereplyosm.h b/src/plugins/geoservices/osm/qgeoroutereplyosm.h
index 8733c154..feaae59c 100644
--- a/src/plugins/geoservices/osm/qgeoroutereplyosm.h
+++ b/src/plugins/geoservices/osm/qgeoroutereplyosm.h
@@ -54,14 +54,9 @@ public:
     QGeoRouteReplyOsm(QNetworkReply *reply, const QGeoRouteRequest &request, QObject *parent = 0);
     ~QGeoRouteReplyOsm();
 
-    void abort() Q_DECL_OVERRIDE;
-
 private Q_SLOTS:
     void networkReplyFinished();
     void networkReplyError(QNetworkReply::NetworkError error);
-
-private:
-    QNetworkReply *m_reply;
 };
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/osm/qplacemanagerengineosm.cpp b/src/plugins/geoservices/osm/qplacemanagerengineosm.cpp
index 719769f0..dcf02b13 100644
--- a/src/plugins/geoservices/osm/qplacemanagerengineosm.cpp
+++ b/src/plugins/geoservices/osm/qplacemanagerengineosm.cpp
@@ -260,9 +260,6 @@ void QPlaceManagerEngineOsm::setLocales(const QList<QLocale> &locales)
 void QPlaceManagerEngineOsm::categoryReplyFinished()
 {
     QNetworkReply *reply = qobject_cast<QNetworkReply *>(sender());
-    if (!reply)
-        return;
-
     reply->deleteLater();
 
     QXmlStreamReader parser(reply);
diff --git a/src/plugins/geoservices/osm/qplacesearchreplyosm.cpp b/src/plugins/geoservices/osm/qplacesearchreplyosm.cpp
index 3cb0ab60..35b05d4d 100644
--- a/src/plugins/geoservices/osm/qplacesearchreplyosm.cpp
+++ b/src/plugins/geoservices/osm/qplacesearchreplyosm.cpp
@@ -53,32 +53,28 @@ QT_BEGIN_NAMESPACE
 
 QPlaceSearchReplyOsm::QPlaceSearchReplyOsm(const QPlaceSearchRequest &request,
                                              QNetworkReply *reply, QPlaceManagerEngineOsm *parent)
-:   QPlaceSearchReply(parent), m_reply(reply)
+:   QPlaceSearchReply(parent)
 {
     Q_ASSERT(parent);
-
-    setRequest(request);
-
-    if (!m_reply)
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
         return;
+    }
+    setRequest(request);
 
-    m_reply->setParent(this);
-    connect(m_reply, SIGNAL(finished()), this, SLOT(replyFinished()));
+    connect(reply, SIGNAL(finished()), this, SLOT(replyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)), this, SLOT(networkError(QNetworkReply::NetworkError)));
+    connect(this, &QPlaceReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
 }
 
 QPlaceSearchReplyOsm::~QPlaceSearchReplyOsm()
 {
 }
 
-void QPlaceSearchReplyOsm::abort()
-{
-    if (m_reply)
-        m_reply->abort();
-}
-
 void QPlaceSearchReplyOsm::setError(QPlaceReply::Error errorCode, const QString &errorString)
 {
-    QPlaceReply::setError(errorCode, errorString);
+    setError(errorCode, errorString);
     emit error(errorCode, errorString);
     setFinished(true);
     emit finished();
@@ -99,14 +95,11 @@ static QGeoRectangle parseBoundingBox(const QJsonArray &coordinates)
 
 void QPlaceSearchReplyOsm::replyFinished()
 {
-    QNetworkReply *reply = m_reply;
-    m_reply->deleteLater();
-    m_reply = 0;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (reply->error() != QNetworkReply::NoError) {
-        setError(CommunicationError, tr("Communication error"));
+    if (reply->error() != QNetworkReply::NoError)
         return;
-    }
 
     QJsonDocument document = QJsonDocument::fromJson(reply->readAll());
     if (!document.isArray()) {
@@ -163,6 +156,14 @@ void QPlaceSearchReplyOsm::replyFinished()
     emit finished();
 }
 
+void QPlaceSearchReplyOsm::networkError(QNetworkReply::NetworkError error)
+{
+    Q_UNUSED(error)
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    setError(QPlaceReply::CommunicationError, reply->errorString());
+}
+
 QPlaceResult QPlaceSearchReplyOsm::parsePlaceResult(const QJsonObject &item) const
 {
     QPlace place;
diff --git a/src/plugins/geoservices/osm/qplacesearchreplyosm.h b/src/plugins/geoservices/osm/qplacesearchreplyosm.h
index e495d4fa..223e5f0c 100644
--- a/src/plugins/geoservices/osm/qplacesearchreplyosm.h
+++ b/src/plugins/geoservices/osm/qplacesearchreplyosm.h
@@ -41,6 +41,7 @@
 #define QPLACESEARCHREPLYOSM_H
 
 #include <QtLocation/QPlaceSearchReply>
+#include <QNetworkReply>
 
 QT_BEGIN_NAMESPACE
 
@@ -57,16 +58,13 @@ public:
                           QPlaceManagerEngineOsm *parent);
     ~QPlaceSearchReplyOsm();
 
-    void abort();
-
 private slots:
     void setError(QPlaceReply::Error errorCode, const QString &errorString);
     void replyFinished();
+    void networkError(QNetworkReply::NetworkError error);
 
 private:
     QPlaceResult parsePlaceResult(const QJsonObject &item) const;
-
-    QNetworkReply *m_reply;
 };
 
 QT_END_NAMESPACE