valgrind: fixed a few memory leaks and errors

[pvalsecc]

The reply handling around QgsNetworkAccessManager is quite complex and
there was a few cases where the reply was not deleted.

In the WMS provider, the usage of QgsNetworkAccessManager was a bit weird.
It's not a good idea to create and delete it that often when there is a
singleton around.

[wonder-sk]

Hi Patrick

-1 for removing private NetworkAccessManager instances - they were introduced because QNAM class is not thread safe and so using that one singleton from multiple threads causes havoc.

[pvalsecc]

Hi @wonder-sk
The only places where a local QNAM is created are qgswcsprovider.cpp and qgswmsprovider.cpp. So you are telling me WFS, WCS, ... are buggy? To be sure, I've added this code in QgsWmsImageDownloadHandler::QgsWmsImageDownloadHandler:

  if ( QgsNetworkAccessManager::instance()->thread() != thread() ) {
      abort();
  }

QGIS crashed in flames. And, after looking I do have those messages in the logs:

src/core/qgsmessagelog.cpp: 45: (logMessage) [4ms] [thread:0x307f210] 2016-01-28T15:08:08 Qt[1] QObject: Cannot create children for a parent that is in a different thread.
(Parent is QgsNetworkAccessManager(0x27e3f80), parent's thread is QThread(0x26935e0), current thread is QThread(0x307f210)

So I'm reverting my changes about that. But what about the other providers that happily use the singleton?

Just for fun, I've changed the implementation of QgsNetworkAccessManager::instance like that:

QgsNetworkAccessManager* QgsNetworkAccessManager::instance()
{
  static QgsNetworkAccessManager* sInstance( new QgsNetworkAccessManager( QApplication::instance() ) );
  if ( sInstance->thread() != QThread::currentThread() ) {
    abort();
  }
  return sInstance;
}

QGIS master (82c9a5a) crashes in flames there:

#0  0x00007ffff43ee267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
#1  0x00007ffff43efeca in __GI_abort () at abort.c:89
#2  0x00007ffff63970a5 in QgsNetworkAccessManager::instance () at /home/pvalsecchi/src/QGIS/src/core/qgsnetworkaccessmanager.cpp:104
#3  0x00007fff6bba8c50 in QgsWmsCapabilitiesDownload::downloadCapabilities (this=0x3218810)
    at /home/pvalsecchi/src/QGIS/src/providers/wms/qgswmscapabilities.cpp:1936
#4  0x00007fff6bba881e in QgsWmsCapabilitiesDownload::downloadCapabilities (this=0x3218810, baseUrl=..., auth=...)
    at /home/pvalsecchi/src/QGIS/src/providers/wms/qgswmscapabilities.cpp:1907
#5  0x00007fff6bc0ca5f in QgsWMSConnectionItem::createChildren (this=0x3218c20)
    at /home/pvalsecchi/src/QGIS/src/providers/wms/qgswmsdataitems.cpp:69
#6  0x00007ffff624474c in QgsDataItem::runCreateChildren (item=0x3218c20) at /home/pvalsecchi/src/QGIS/src/core/qgsdataitem.cpp:385
#7  0x00007ffff6250026 in QtConcurrent::StoredFunctorCall1<QVector<QgsDataItem*>, QVector<QgsDataItem*> (*)(QgsDataItem*), QgsDataItem*>::runFunctor (this=0x3216eb0) at /usr/include/qt4/QtCore/qtconcurrentstoredfunctioncall.h:267
#8  0x00007ffff624fef5 in QtConcurrent::RunFunctionTask<QVector<QgsDataItem*> >::run (this=0x3216eb0)
    at /usr/include/qt4/QtCore/qtconcurrentrunbase.h:106
#9  0x00007ffff5a7adba in QThreadPoolThread::run (this=0x1203390) at concurrent/qthreadpool.cpp:108
#10 0x00007ffff5a87d1c in QThreadPrivate::start (arg=0x1203390) at thread/qthread_unix.cpp:349
#11 0x00007fffedf1a6aa in start_thread (arg=0x7fff5f5b7700) at pthread_create.c:333
#12 0x00007ffff44bfeed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

I think I'll create an issue. What is your opinion?

[wonder-sk]

The only places where a local QNAM is created are qgswcsprovider.cpp and qgswmsprovider.cpp. So you are telling me WFS, WCS, ... are buggy?

WMS and WCS should be handled correctly (at least the rendering part). WFS is buggy in various ways and a significant rewrite is planned: qgis/QGIS-Enhancement-Proposals#53

I think I'll create an issue. What is your opinion?

Sounds good. I can even see an easy way to demonstrate the issue: define few WMS servers, then expand them in the browser dock widget. Restart QGIS: on start, WMS server items will be populated (in threads) because they have expanded state and so they request capabilities of WMS servers - at that point the whole QGIS gets stuck while requesting capabilities.

[m-kuhn]

@pvalsecc

I would replace

if ( sInstance->thread() != QThread::currentThread() ) {
  abort();

with

Q_ASSERT( sInstance->thread() == QThread::currentThread() )

so this only kicks in on debug builds

[pvalsecc]

@m-kuhn

I have no intention of adding this test to my commit. It crashes all the time. I've create an issue for the problem: https://hub.qgis.org/issues/14192

[m-kuhn]

Ok, makes sense.

Would be great to put this assertion in there once the underlying issues are fixed to avoid future regressions.

[alexbruy]

Am I wrong, or this was fixed with 2eb8243?

[pvalsecc]

@alexbruy, partially, yes. There are other fixes. I'll go through them and I'll update this PR when I have some time.
Thanks.

[pvalsecc]

@m-kuhn, this PR has been revisited, please remove the Requires Changes tag.

[m-kuhn]

@wonder-sk does this look good to you?

[wonder-sk]

I do not see any harm here, but I fail to understand what does that change fix...?

[pvalsecc]

@wonder-sk:

• parsing errors in the settings were just ignored.
• I don't really remember... old PR. I guess to leave the snapMode to SnapCurrentLayer.

[pvalsecc]

Cleaning up my PR that have been there for too long.

[nyalldawson]

@pvalsecc is any of this still relevant?

[pvalsecc]

@nyalldawson. I don't know, I don't have crashes because of that and I've spent too much time rebasing this guy already. I'm just cutting my losses... ;-)