Merge pull request #7195 from elpaso/bugfix-18981-export-qlr-crash-2

[bugfix] Crash when exporting (invalid) legend to qlr Fixes #18981
qgis · Jun 7, 2018 · 51ae8e9 · 51ae8e9
2 parents 22a98fb + b307f39
commit 51ae8e9
Show file tree

Hide file tree

Showing 6 changed files with 265 additions and 3 deletions.
diff --git a/src/core/layertree/qgslayertreegroup.cpp b/src/core/layertree/qgslayertreegroup.cpp
@@ -221,7 +221,7 @@ QList<QgsLayerTreeLayer *> QgsLayerTreeGroup::findLayers() const
   QList<QgsLayerTreeLayer *> list;
   Q_FOREACH ( QgsLayerTreeNode *child, mChildren )
   {
-    if ( QgsLayerTree::isLayer( child ) )
+    if ( QgsLayerTree::isLayer( child ) && QgsLayerTree::toLayer( child )->layer( ) )
       list << QgsLayerTree::toLayer( child );
     else if ( QgsLayerTree::isGroup( child ) )
       list << QgsLayerTree::toGroup( child )->findLayers();

diff --git a/src/core/qgslayerdefinition.cpp b/src/core/qgslayerdefinition.cpp
@@ -12,7 +12,6 @@
  *   (at your option) any later version.                                   *
  *                                                                         *
  ***************************************************************************/
-#include <QDomNode>
 #include <QFileInfo>
 #include <QFile>
 #include <QDir>
@@ -214,6 +213,11 @@ bool QgsLayerDefinition::exportLayerDefinition( QDomDocument doc, const QList<Qg
   QList<QgsLayerTreeLayer *> layers = root->findLayers();
   Q_FOREACH ( QgsLayerTreeLayer *layer, layers )
   {
+    if ( ! layer->layer() )
+    {
+      QgsDebugMsgLevel( QStringLiteral( "Not a valid map layer: skipping %1" ).arg( layer->name( ) ), 4 );
+      continue;
+    }
     QDomElement layerelm = doc.createElement( QStringLiteral( "maplayer" ) );
     layer->layer()->writeLayerXml( layerelm, doc, context );
     layerselm.appendChild( layerelm );

diff --git a/src/core/qgslayerdefinition.h b/src/core/qgslayerdefinition.h
@@ -21,8 +21,8 @@
 
 #include <QString>
 #include <QVector>
+#include <QDomNode>
 
-class QDomNode;
 class QDomDocument;
 
 class QgsLayerTreeGroup;

diff --git a/tests/src/core/CMakeLists.txt b/tests/src/core/CMakeLists.txt
@@ -191,6 +191,7 @@ SET(TESTS
  testziplayer.cpp
  testqgsmeshlayer.cpp
  testqgsmeshlayerrenderer.cpp
+ testqgslayerdefinition.cpp
    )
 
 IF(WITH_QTWEBKIT)

diff --git a/tests/src/core/testqgslayerdefinition.cpp b/tests/src/core/testqgslayerdefinition.cpp
@@ -0,0 +1,107 @@
+/***************************************************************************
+    testqgsfilefiledownloader.cpp
+     --------------------------------------
+    Date                 : 07.06.2018
+    Copyright            : (C) 2018 Alessandro Pasotti
+    Email                : elpaso at itopen dot it
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+
+#include "qgstest.h"
+#include <QObject>
+#include <QTemporaryFile>
+#include <QTemporaryDir>
+
+#include <qgsapplication.h>
+#include <qgsproject.h>
+#include <qgslayertree.h>
+#include <qgslayerdefinition.h>
+
+class TestQgsLayerDefinition: public QObject
+{
+    Q_OBJECT
+  public:
+    TestQgsLayerDefinition() = default;
+
+  private slots:
+    void initTestCase(); // will be called before the first testfunction is executed.
+    void cleanupTestCase(); // will be called after the last testfunction was executed.
+    void init(); // will be called before each testfunction is executed.
+    void cleanup(); // will be called after every testfunction.
+
+    /**
+     * test that findLayers() skips invalid layers
+     */
+    void testFindLayers();
+
+    /**
+     * test that export does not crash: regression #18981
+     * https://issues.qgis.org/issues/18981 - Save QLR crashes QGIS 3
+     */
+    void testExportDoesNotCrash();
+
+  private:
+    QTemporaryFile *mTempFile;
+};
+
+
+void TestQgsLayerDefinition::initTestCase()
+{
+  QgsApplication::init();
+  QgsApplication::initQgis();
+
+}
+
+void TestQgsLayerDefinition::cleanupTestCase()
+{
+  QgsApplication::exitQgis();
+}
+
+void TestQgsLayerDefinition::init()
+{
+  mTempFile = new QTemporaryFile();
+  QVERIFY( mTempFile->open() );
+  mTempFile->close();
+  QString errorMessage;
+  const QString path = QString( TEST_DATA_DIR + QStringLiteral( "/bug_18981_broken.qlr" ) );
+  QgsLayerDefinition::loadLayerDefinition( path, QgsProject::instance(), QgsProject::instance()->layerTreeRoot(), errorMessage );
+  QVERIFY( errorMessage.isEmpty() );
+}
+
+
+void TestQgsLayerDefinition::cleanup()
+{
+  delete mTempFile;
+}
+
+void TestQgsLayerDefinition::testFindLayers()
+{
+  QCOMPARE( QgsProject::instance()->layerTreeRoot()->findLayers().count(), 1 );
+  QCOMPARE( QgsProject::instance()->layerTreeRoot()->findLayers().at( 0 )->name(), QStringLiteral( "NewMemory" ) );
+}
+
+void TestQgsLayerDefinition::testExportDoesNotCrash()
+{
+  QString errorMessage;
+  QVERIFY( QgsLayerDefinition::exportLayerDefinition( mTempFile->fileName(), QgsProject::instance()->layerTreeRoot()->children(), errorMessage ) );
+  QVERIFY( errorMessage.isEmpty() );
+  // Reload
+  const QString path = QString( TEST_DATA_DIR + QStringLiteral( "/bug_18981_broken.qlr" ) );
+  QgsProject::instance()->removeAllMapLayers();
+  QgsLayerDefinition::loadLayerDefinition( path, QgsProject::instance(), QgsProject::instance()->layerTreeRoot(), errorMessage );
+  testFindLayers();
+}
+
+
+
+QGSTEST_MAIN( TestQgsLayerDefinition )
+#include "testqgslayerdefinition.moc"
+
+
diff --git a/tests/testdata/bug_18981_broken.qlr b/tests/testdata/bug_18981_broken.qlr
@@ -0,0 +1,150 @@
+<!DOCTYPE qgis-layer-definition>
+<qlr>
+  <layer-tree-group expanded="1" checked="Qt::Checked" name="">
+    <customproperties/>
+    <layer-tree-group expanded="1" checked="Qt::Checked" name="qgis_plugin">
+      <customproperties/>
+      <layer-tree-group expanded="0" checked="Qt::Checked" name="Baggrundskort">
+        <customproperties/>
+        <layer-tree-layer expanded="1" providerKey="wms" checked="Qt::Checked" id="DTK_D8502016120170201702017020170201702017201801241314424672312" source="IgnoreGetMapUrl=1&amp;contextualWMSLegend=0&amp;crs=EPSG:25832&amp;dpiMode=7&amp;featureCount=10&amp;format=image/png&amp;layers=dtk_d850&amp;styles=&amp;url=http://kortforsyningen.kms.dk/service?servicename%3Dtopo_basis%26client%3DQGIS%26version%3D1.1.1%26login%3Dj%26password%3Dj" name="DTK/D850">
+          <customproperties/>
+        </layer-tree-layer>
+        <layer-tree-layer providerKey="memory" checked="Qt::Checked" expanded="1" name="NewMemory" source="NoGeometry?crs=EPSG:4326&amp;uid={64c9bcbf-cabe-4bd9-9058-2b80ebf87a72}" id="NewMemory_ffa4d8a4_e5be_46a9_a0e0_fb6ee924f3cd">
+        <customproperties/>
+      </layer-tree-layer>
+      </layer-tree-group>
+    </layer-tree-group>
+  </layer-tree-group>
+  <maplayers>
+    <maplayer minimumScale="0" maximumScale="1e+08" type="raster" hasScaleBasedVisibilityFlag="0">
+      <extent>
+        <xmin>120000</xmin>
+        <ymin>5900000</ymin>
+        <xmax>1000000</xmax>
+        <ymax>6500000</ymax>
+      </extent>
+      <id>DTK_D8502016120170201702017020170201702017201801241314424672312</id>
+      <datasource>IgnoreGetMapUrl=1&amp;contextualWMSLegend=0&amp;crs=EPSG:25832&amp;dpiMode=7&amp;featureCount=10&amp;format=image/png&amp;layers=dtk_d850&amp;styles=&amp;url=http://kortforsyningen.kms.dk/service?servicename%3Dtopo_basis%26client%3DQGIS%26version%3D1.1.1%26login%3Dj%26password%3Dj</datasource>
+      <keywordList>
+        <value></value>
+      </keywordList>
+      <layername>DTK/D850</layername>
+      <srs>
+        <spatialrefsys>
+          <proj4>+proj=utm +zone=32 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no_defs</proj4>
+          <srsid>2105</srsid>
+          <srid>25832</srid>
+          <authid>EPSG:25832</authid>
+          <description>ETRS89 / UTM zone 32N</description>
+          <projectionacronym>utm</projectionacronym>
+          <ellipsoidacronym>GRS80</ellipsoidacronym>
+          <geographicflag>false</geographicflag>
+        </spatialrefsys>
+      </srs>
+      <customproperties>
+        <property key="identify/format" value="Undefined"/>
+      </customproperties>
+      <provider>wms</provider>
+      <noData>
+        <noDataList bandNo="1" useSrcNoData="0"/>
+      </noData>
+      <map-layer-style-manager current="">
+        <map-layer-style name=""/>
+      </map-layer-style-manager>
+      <pipe>
+        <rasterrenderer opacity="1" alphaBand="-1" band="1" type="singlebandcolordata">
+          <rasterTransparency/>
+        </rasterrenderer>
+        <brightnesscontrast brightness="0" contrast="0"/>
+        <huesaturation colorizeGreen="128" colorizeOn="0" colorizeRed="255" colorizeBlue="128" grayscaleMode="0" saturation="0" colorizeStrength="100"/>
+        <rasterresampler maxOversampling="2"/>
+      </pipe>
+      <blendMode>0</blendMode>
+    </maplayer>
+    <maplayer refreshOnNotifyEnabled="0" readOnly="0" minScale="1e+8" type="vector" autoRefreshTime="0" hasScaleBasedVisibilityFlag="0" geometry="No geometry" autoRefreshEnabled="0" refreshOnNotifyMessage="" maxScale="0">
+      <id>NewMemory_ffa4d8a4_e5be_46a9_a0e0_fb6ee924f3cd</id>
+      <datasource>memory?geometry=NoGeometry&amp;crs=EPSG:4326</datasource>
+      <keywordList>
+        <value></value>
+      </keywordList>
+      <layername>NewMemory</layername>
+      <srs>
+        <spatialrefsys>
+          <proj4>+proj=longlat +datum=WGS84 +no_defs</proj4>
+          <srsid>3452</srsid>
+          <srid>4326</srid>
+          <authid>EPSG:4326</authid>
+          <description>WGS 84</description>
+          <projectionacronym>longlat</projectionacronym>
+          <ellipsoidacronym>WGS84</ellipsoidacronym>
+          <geographicflag>true</geographicflag>
+        </spatialrefsys>
+      </srs>
+      <resourceMetadata>
+        <identifier></identifier>
+        <parentidentifier></parentidentifier>
+        <language></language>
+        <type></type>
+        <title></title>
+        <abstract></abstract>
+        <links/>
+        <fees></fees>
+        <encoding></encoding>
+        <crs>
+          <spatialrefsys>
+            <proj4></proj4>
+            <srsid>0</srsid>
+            <srid>0</srid>
+            <authid></authid>
+            <description></description>
+            <projectionacronym></projectionacronym>
+            <ellipsoidacronym></ellipsoidacronym>
+            <geographicflag>false</geographicflag>
+          </spatialrefsys>
+        </crs>
+        <extent/>
+      </resourceMetadata>
+      <customproperties/>
+      <provider encoding="UTF-8">memory</provider>
+      <vectorjoins/>
+      <layerDependencies/>
+      <dataDependencies/>
+      <legend type="default-vector"/>
+      <expressionfields/>
+      <map-layer-style-manager current="predefinito">
+        <map-layer-style name="predefinito"/>
+      </map-layer-style-manager>
+      <auxiliaryLayer/>
+      <fieldConfiguration/>
+      <aliases/>
+      <excludeAttributesWMS/>
+      <excludeAttributesWFS/>
+      <defaults/>
+      <constraints/>
+      <constraintExpressions/>
+      <attributeactions>
+        <defaultAction key="Canvas" value="{00000000-0000-0000-0000-000000000000}"/>
+      </attributeactions>
+      <attributetableconfig sortOrder="0" actionWidgetStyle="dropDown" sortExpression="">
+        <columns/>
+      </attributetableconfig>
+      <editform></editform>
+      <editforminit/>
+      <editforminitcodesource>0</editforminitcodesource>
+      <editforminitfilepath></editforminitfilepath>
+      <editforminitcode><![CDATA[]]></editforminitcode>
+      <featformsuppress>0</featformsuppress>
+      <editorlayout>generatedlayout</editorlayout>
+      <editable/>
+      <labelOnTop/>
+      <widgets/>
+      <conditionalstyles>
+        <rowstyles/>
+        <fieldstyles/>
+      </conditionalstyles>
+      <expressionfields/>
+      <previewExpression></previewExpression>
+      <mapTip></mapTip>
+    </maplayer>
+  </maplayers>
+</qlr>