use signals/slots between QgsWmsCapabilitiesDownload and QgsNetworkAccessManager, fixes crash on start up in #13271

blazek · blazek · commit e95bf6d3e016 · 2015-09-01T16:34:35.000+02:00
diff --git a/src/core/qgsnetworkaccessmanager.cpp b/src/core/qgsnetworkaccessmanager.cpp
@@ -341,3 +341,20 @@ void QgsNetworkAccessManager::setupDefaultProxyAndCache()
     setCache( newcache );
 }
 
+void QgsNetworkAccessManager::sendGet( const QNetworkRequest & request )
+{
+  QgsDebugMsg( "Entered" );
+  QNetworkReply * reply = get( request );
+  emit requestSent( reply, QObject::sender() );
+}
+
+void QgsNetworkAccessManager::deleteReply( QNetworkReply * reply )
+{
+  QgsDebugMsg( "Entered" );
+  if ( !reply )
+  {
+    return;
+  }
+  reply->abort();
+  reply->deleteLater();
+}
diff --git a/src/core/qgsnetworkaccessmanager.h b/src/core/qgsnetworkaccessmanager.h
@@ -84,10 +84,29 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
 
     bool useSystemProxy() { return mUseSystemProxy; }
 
+  public slots:
+    /** Send GET request, calls get().
+     * Emits requestSent().
+     * @param request request to be sent
+     */
+    void sendGet( const QNetworkRequest & request );
+    /** Abort and delete reply. This slot may be used to abort reply created by instance of this class
+     * (and which was not moved to another thread) from a different thread. Such reply cannot
+     * be aborted directly from a different thread. The reply must be also deleted
+     * in this slot, otherwise it could happen that abort signal comes after the reply was deleted.
+     * @param reply reply to be aborted.
+     */
+    void deleteReply( QNetworkReply * reply );
+
   signals:
     void requestAboutToBeCreated( QNetworkAccessManager::Operation, const QNetworkRequest &, QIODevice * );
     void requestCreated( QNetworkReply * );
     void requestTimedOut( QNetworkReply * );
+    /** Emited when request was sent by request()
+     * @param reply request reply
+     * @param sender the object which called request() slot.
+     */
+    void requestSent( QNetworkReply * reply, QObject *sender );
 
   private slots:
     void abortRequest();
diff --git a/src/providers/wms/qgswmscapabilities.cpp b/src/providers/wms/qgswmscapabilities.cpp
@@ -1859,6 +1859,7 @@ QgsWmsCapabilitiesDownload::QgsWmsCapabilitiesDownload( QObject *parent )
     , mCapabilitiesReply( 0 )
     , mIsAborted( false )
 {
+  connectManager();
 }
 
 QgsWmsCapabilitiesDownload::QgsWmsCapabilitiesDownload( const QString& baseUrl, const QgsWmsAuthorization& auth, QObject *parent )
@@ -1868,6 +1869,22 @@ QgsWmsCapabilitiesDownload::QgsWmsCapabilitiesDownload( const QString& baseUrl,
     , mCapabilitiesReply( 0 )
     , mIsAborted( false )
 {
+  connectManager();
+}
+
+void QgsWmsCapabilitiesDownload::connectManager()
+{
+  // The instance of this class may live on a thread different from QgsNetworkAccessManager instance's thread,
+  // so we cannot call QgsNetworkAccessManager::get() directly and we must send a signal instead.
+  connect( this, SIGNAL( sendRequest( const QNetworkRequest & ) ),
+           QgsNetworkAccessManager::instance(), SLOT( sendGet( const QNetworkRequest & ) ) );
+  connect( this, SIGNAL( deleteReply( QNetworkReply * ) ),
+           QgsNetworkAccessManager::instance(), SLOT( deleteReply( QNetworkReply * ) ) );
+}
+
+QgsWmsCapabilitiesDownload::~QgsWmsCapabilitiesDownload()
+{
+  abort();
 }
 
 bool QgsWmsCapabilitiesDownload::downloadCapabilities( const QString& baseUrl, const QgsWmsAuthorization& auth )
@@ -1880,7 +1897,9 @@ bool QgsWmsCapabilitiesDownload::downloadCapabilities( const QString& baseUrl, c
 bool QgsWmsCapabilitiesDownload::downloadCapabilities()
 {
   QgsDebugMsg( "entering." );
+  abort(); // cancel previous
   mIsAborted = false;
+
   QString url = mBaseUrl;
   QgsDebugMsg( "url = " + url );
   if ( !url.contains( "SERVICE=WMTS" ) &&
@@ -1896,17 +1915,9 @@ bool QgsWmsCapabilitiesDownload::downloadCapabilities()
   request.setAttribute( QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::PreferNetwork );
   request.setAttribute( QNetworkRequest::CacheSaveControlAttribute, true );
 
-  QgsDebugMsg( QString( "getcapabilities: %1" ).arg( url ) );
-  // This is causing Qt warning: "Cannot create children for a parent that is in a different thread."
-  // but it only means that the reply will have no parent
-  if ( mIsAborted )
-  {
-    return false;
-  }
-  mCapabilitiesReply = QgsNetworkAccessManager::instance()->get( request );
-
-  connect( mCapabilitiesReply, SIGNAL( finished() ), this, SLOT( capabilitiesReplyFinished() ), Qt::DirectConnection );
-  connect( mCapabilitiesReply, SIGNAL( downloadProgress( qint64, qint64 ) ), this, SLOT( capabilitiesReplyProgress( qint64, qint64 ) ), Qt::DirectConnection );
+  connect( QgsNetworkAccessManager::instance(), SIGNAL( requestSent( QNetworkReply *, QObject * ) ),
+           SLOT( requestSent( QNetworkReply *, QObject * ) ) );
+  emit sendRequest( request );
 
   QEventLoop loop;
   connect( this, SIGNAL( downloadFinished() ), &loop, SLOT( quit() ) );
@@ -1915,13 +1926,41 @@ bool QgsWmsCapabilitiesDownload::downloadCapabilities()
   return mError.isEmpty();
 }
 
+void QgsWmsCapabilitiesDownload::requestSent( QNetworkReply * reply, QObject *sender )
+{
+  QgsDebugMsg( "Entered" );
+  if ( sender != this ) // it is not our reply
+  {
+    return;
+  }
+  disconnect( QgsNetworkAccessManager::instance(), SIGNAL( requestSent( QNetworkReply *, QObject * ) ),
+              this, SLOT( requestSent( QNetworkReply *, QObject * ) ) );
+
+  if ( !reply )
+  {
+    emit downloadFinished();
+    return;
+  }
+  if ( mIsAborted )
+  {
+    emit deleteReply( reply );
+    emit downloadFinished();
+    return;
+  }
+  // Note: the reply was created on QgsNetworkAccessManager's thread
+  mCapabilitiesReply = reply;
+  connect( mCapabilitiesReply, SIGNAL( finished() ), this, SLOT( capabilitiesReplyFinished() ), Qt::DirectConnection );
+  connect( mCapabilitiesReply, SIGNAL( downloadProgress( qint64, qint64 ) ), this, SLOT( capabilitiesReplyProgress( qint64, qint64 ) ), Qt::DirectConnection );
+}
+
 void QgsWmsCapabilitiesDownload::abort()
 {
   QgsDebugMsg( "Entered" );
   mIsAborted = true;
   if ( mCapabilitiesReply )
   {
-    mCapabilitiesReply->abort();
+    emit deleteReply( mCapabilitiesReply );
+    mCapabilitiesReply = 0;
   }
 }
 
@@ -1935,7 +1974,7 @@ void QgsWmsCapabilitiesDownload::capabilitiesReplyProgress( qint64 bytesReceived
 void QgsWmsCapabilitiesDownload::capabilitiesReplyFinished()
 {
   QgsDebugMsg( "entering." );
-  if ( !mIsAborted )
+  if ( !mIsAborted && mCapabilitiesReply )
   {
     if ( mCapabilitiesReply->error() == QNetworkReply::NoError )
     {
@@ -1962,10 +2001,8 @@ void QgsWmsCapabilitiesDownload::capabilitiesReplyFinished()
 
           mCapabilitiesReply->deleteLater();
           QgsDebugMsg( QString( "redirected getcapabilities: %1" ).arg( redirect.toString() ) );
-          mCapabilitiesReply = QgsNetworkAccessManager::instance()->get( request );
-
-          connect( mCapabilitiesReply, SIGNAL( finished() ), this, SLOT( capabilitiesReplyFinished() ) );
-          connect( mCapabilitiesReply, SIGNAL( downloadProgress( qint64, qint64 ) ), this, SLOT( capabilitiesReplyProgress( qint64, qint64 ) ) );
+          //mCapabilitiesReply = QgsNetworkAccessManager::instance()->get( request );
+          emit sendRequest( request );
           return;
         }
       }
@@ -1987,8 +2024,11 @@ void QgsWmsCapabilitiesDownload::capabilitiesReplyFinished()
     }
   }
 
-  mCapabilitiesReply->deleteLater();
-  mCapabilitiesReply = 0;
+  if ( mCapabilitiesReply )
+  {
+    mCapabilitiesReply->deleteLater();
+    mCapabilitiesReply = 0;
+  }
 
   emit downloadFinished();
 }
diff --git a/src/providers/wms/qgswmscapabilities.h b/src/providers/wms/qgswmscapabilities.h
@@ -677,7 +677,13 @@ class QgsWmsCapabilities
 
 
 
-/** Class that handles download of capabilities */
+/** Class that handles download of capabilities.
+ * Methods of this class may only be called directly from the thread to which instance of the class has affinity.
+ * It is possible to connect to abort() slot from another thread however.
+ */
+/* The requirement to call methods only from the thread to which this class instance has affinity guarantees that
+ * abort() cannot be called in the middle of another method and makes it simple to check if the request was aborted.
+ */
 class QgsWmsCapabilitiesDownload : public QObject
 {
     Q_OBJECT
@@ -687,6 +693,8 @@ class QgsWmsCapabilitiesDownload : public QObject
 
     QgsWmsCapabilitiesDownload( const QString& baseUrl, const QgsWmsAuthorization& auth, QObject* parent = 0 );
 
+    virtual ~QgsWmsCapabilitiesDownload();
+
     bool downloadCapabilities();
 
     bool downloadCapabilities( const QString& baseUrl, const QgsWmsAuthorization& auth );
@@ -695,16 +703,25 @@ class QgsWmsCapabilitiesDownload : public QObject
 
     QByteArray response() const { return mHttpCapabilitiesResponse; }
 
+  public slots:
     /** Abort network request immediately */
     void abort();
+
   signals:
     /** \brief emit a signal to be caught by qgisapp and display a msg on status bar */
     void statusChanged( QString const &  theStatusQString );
 
     /** \brief emit a signal once the download is finished */
     void downloadFinished();
 
+    /** Send request via signal/slot to main another thread */
+    void sendRequest( const QNetworkRequest & request );
+
+    /** Abort request through QgsNetworkAccessManager */
+    void deleteReply( QNetworkReply * reply );
+
   protected slots:
+    void requestSent( QNetworkReply * reply, QObject *sender );
     void capabilitiesReplyFinished();
     void capabilitiesReplyProgress( qint64, qint64 );
 
@@ -727,6 +744,9 @@ class QgsWmsCapabilitiesDownload : public QObject
     QByteArray mHttpCapabilitiesResponse;
 
     bool mIsAborted;
+
+  private:
+    void connectManager();
 };
 
 
