Merge pull request #3864 from elpaso/downloader-fixes-bad-network

[BUGFIX] Fixed a crash on bad network protocol

Author: elpaso

src/app/qgsidentifyresultsdialog.cpp

@@ -77,7 +77,6 @@ QgsIdentifyResultsWebView::QgsIdentifyResultsWebView( QWidget *parent ) : QgsWeb
 {
   setSizePolicy( QSizePolicy::MinimumExpanding, QSizePolicy::Minimum );
   page()->setNetworkAccessManager( QgsNetworkAccessManager::instance() );
-  // page()->setLinkDelegationPolicy( QWebPage::DelegateAllLinks );
 #ifdef WITH_QTWEBKIT
   page()->setForwardUnsupportedContent( true );
 #endif

src/gui/qgsfiledownloader.cpp

@@ -60,7 +60,6 @@ void QgsFileDownloader::startDownload()
   mReply = nam->get( request );
 
   connect( mReply, &QNetworkReply::readyRead, this, &QgsFileDownloader::onReadyRead );
-  connect( mReply, static_cast < void ( QNetworkReply::* )( QNetworkReply::NetworkError ) > ( &QNetworkReply::error ), this, &QgsFileDownloader::onNetworkError );
   connect( mReply, &QNetworkReply::finished, this, &QgsFileDownloader::onFinished );
   connect( mReply, &QNetworkReply::downloadProgress, this, &QgsFileDownloader::onDownloadProgress );
   connect( nam, &QgsNetworkAccessManager::requestTimedOut, this, &QgsFileDownloader::onRequestTimedOut );
@@ -108,7 +107,7 @@ void QgsFileDownloader::error( QStringList errorMessages )
 {
   for ( auto end = errorMessages.size(), i = 0; i != end; ++i )
   {
-    mErrors.append( errorMessages[i] );
+    mErrors << errorMessages[i];
   }
   // Show error
   if ( mGuiNotificationsEnabled )
@@ -161,7 +160,7 @@ void QgsFileDownloader::onFinished()
     if ( mReply->error() )
     {
       mFile.remove();
-      error( tr( "Download failed: %1." ).arg( mReply->errorString() ) );
+      error( tr( "Download failed: %1" ).arg( mReply->errorString() ) );
     }
     else if ( !redirectionTarget.isNull() )
     {
@@ -174,8 +173,10 @@ void QgsFileDownloader::onFinished()
       startDownload();
       return;
     }
-    // All done
-    emit downloadCompleted();
+    else
+    {
+      emit downloadCompleted();
+    }
   }
   emit downloadExited();
   this->deleteLater();

tests/src/gui/testqgsfiledownloader.cpp

@@ -78,8 +78,8 @@ class TestQgsFileDownloader: public QObject
 
     void testValidDownload();
     void testInValidDownload();
-    void testCanceledDownload();
     void testInvalidFile();
+    void testCanceledDownload();
     void testInvalidUrl();
     void testBlankUrl();
 #ifndef QT_NO_SSL
@@ -172,7 +172,7 @@ void TestQgsFileDownloader::testInValidDownload()
   QVERIFY( mError );
   QVERIFY( !mCanceled );
   QVERIFY( mTempFile->size() == 0 );
-  QCOMPARE( mErrorMessage, QString( "Network error 3: Host www.doesnotexistofthatimsure.qgis not found" ) );
+  QCOMPARE( mErrorMessage, QString( "Download failed: Host www.doesnotexistofthatimsure.qgis not found" ) );
 }
 
 void TestQgsFileDownloader::testCanceledDownload()
@@ -184,7 +184,7 @@ void TestQgsFileDownloader::testCanceledDownload()
   QVERIFY( !mError );
   QVERIFY( mProgress );
   QVERIFY( mCanceled );
-  QVERIFY( mTempFile->size() == 0 );
+  QVERIFY( !mTempFile->exists() );
 }
 
 void TestQgsFileDownloader::testInvalidFile()
@@ -205,7 +205,7 @@ void TestQgsFileDownloader::testInvalidUrl()
   QVERIFY( !mCompleted );
   QVERIFY( mError );
   QVERIFY( !mCanceled );
-  QCOMPARE( mErrorMessage, QString( "Network error 301: Protocol \"xyz\" is unknown" ) );
+  QCOMPARE( mErrorMessage, QString( "Download failed: Protocol \"xyz\" is unknown" ) );
 }
 
 void TestQgsFileDownloader::testBlankUrl()
@@ -216,7 +216,7 @@ void TestQgsFileDownloader::testBlankUrl()
   QVERIFY( !mCompleted );
   QVERIFY( mError );
   QVERIFY( !mCanceled );
-  QCOMPARE( mErrorMessage, QString( "Network error 301: Protocol \"\" is unknown" ) );
+  QCOMPARE( mErrorMessage, QString( "Download failed: Protocol \"\" is unknown" ) );
 }
 
 #ifndef QT_NO_SSL
@@ -226,11 +226,11 @@ void TestQgsFileDownloader::testSslError_data()
   QTest::addColumn<QString>( "result" );
 
   QTest::newRow( "expired" ) << "https://expired.badssl.com/"
-  << "Network error 6: SSL handshake failed;SSL Errors: ;The certificate has expired";
+  << "SSL Errors: ;The certificate has expired";
   QTest::newRow( "self-signed" ) << "https://self-signed.badssl.com/"
-  << "Network error 6: SSL handshake failed;SSL Errors: ;The certificate is self-signed, and untrusted";
+  << "SSL Errors: ;The certificate is self-signed, and untrusted";
   QTest::newRow( "untrusted-root" ) << "https://untrusted-root.badssl.com/"
-  << "Network error 6: SSL handshake failed;No certificates could be verified;SSL Errors: ;The issuer certificate of a locally looked up certificate could not be found";
+  << "No certificates could be verified;SSL Errors: ;The issuer certificate of a locally looked up certificate could not be found";
 }
 
 void TestQgsFileDownloader::testSslError()

tests/src/python/test_qgsfiledownloader.py

@@ -78,7 +78,7 @@ def test_inValidDownload(self):
         self.assertTrue(self.progress_was_called)
         self.assertFalse(self.canceled_was_called)
         self.assertTrue(self.error_was_called)
-        self.assertEqual(self.error_args[1], [u'Network error 3: Host www.doesnotexistofthatimsure.qgis not found'])
+        self.assertEqual(self.error_args[1], [u'Download failed: Host www.doesnotexistofthatimsure.qgis not found'])
         self.assertFalse(os.path.isfile(destination))
 
     def test_dowloadCanceled(self):
@@ -99,7 +99,7 @@ def test_InvalidUrl(self):
         self.assertFalse(self.canceled_was_called)
         self.assertTrue(self.error_was_called)
         self.assertFalse(os.path.isfile(destination))
-        self.assertEqual(self.error_args[1], [u"Network error 301: Protocol \"xyz\" is unknown"])
+        self.assertEqual(self.error_args[1], [u"Download failed: Protocol \"xyz\" is unknown"])
 
     def test_InvalidFile(self):
         self._make_download('https://github.com/qgis/QGIS/archive/master.zip', "")
@@ -117,7 +117,7 @@ def test_BlankUrl(self):
         self.assertFalse(self.canceled_was_called)
         self.assertTrue(self.error_was_called)
         self.assertFalse(os.path.isfile(destination))
-        self.assertEqual(self.error_args[1], [u"Network error 301: Protocol \"\" is unknown"])
+        self.assertEqual(self.error_args[1], [u"Download failed: Protocol \"\" is unknown"])
 
     def ssl_compare(self, name, url, error):
         destination = tempfile.mktemp()
@@ -133,9 +133,9 @@ def ssl_compare(self, name, url, error):
         self.assertTrue(result.startswith(error), msg + "expected:\n%s\nactual:\n%s\n" % (result, error))
 
     def test_sslExpired(self):
-        self.ssl_compare("expired", "https://expired.badssl.com/", "Network error 6: SSL handshake failed;SSL Errors: ;The certificate has expired")
-        self.ssl_compare("self-signed", "https://self-signed.badssl.com/", "Network error 6: SSL handshake failed;SSL Errors: ;The certificate is self-signed, and untrusted")
-        self.ssl_compare("untrusted-root", "https://untrusted-root.badssl.com/", "Network error 6: SSL handshake failed;No certificates could be verified;")
+        self.ssl_compare("expired", "https://expired.badssl.com/", "SSL Errors: ;The certificate has expired")
+        self.ssl_compare("self-signed", "https://self-signed.badssl.com/", "SSL Errors: ;The certificate is self-signed, and untrusted")
+        self.ssl_compare("untrusted-root", "https://untrusted-root.badssl.com/", "No certificates could be verified;SSL Errors: ;The issuer certificate of a locally looked up certificate could not be found")
 
     def _set_slot(self, *args, **kwargs):
         #print('_set_slot(%s) called' % args[0])