Invalidate old QImage data if load()/loadFromData() has failed

This guarantees one will never get `!img.isNull()` after
load()/loadFromData() has failed, even if the image was
not null before.
Apply the same fix to QPixmap and QPicture.

Change-Id: Ida1ad6a6f0fc830df8e75ada0c163fc2d3360dea
Reviewed-by: Samuel Rødal <samuel.rodal@digia.com>

dist/changes-5.0.0

@@ -494,6 +494,11 @@ QtGui
   For consistency with RGB32 and other 32-bit formats, function now expects
   image data in RGB layout as opposed to BGR layout.
 
+* Behavioral change in QImage and QPixmap load()/loadFromData() on a non-null image:
+  If load() or loadFromData() fails to load the image (returns false) then
+  the existent image data will be invalidated, so that isNull() is guaranteed
+  to return true in this case.
+
 QtWidgets
 ---------
 * QInputContext removed as well as related getters and setters on QWidget and QApplication.

src/gui/image/qimage.cpp

@@ -4346,7 +4346,8 @@ QImage QImage::rgbSwapped() const
 
 /*!
     Loads an image from the file with the given \a fileName. Returns true if
-    the image was successfully loaded; otherwise returns false.
+    the image was successfully loaded; otherwise invalidates the image
+    and returns false.
 
     The loader attempts to read the image using the specified \a format, e.g.,
     PNG or JPG. If \a format is not specified (which is the default), the
@@ -4363,15 +4364,9 @@ QImage QImage::rgbSwapped() const
 
 bool QImage::load(const QString &fileName, const char* format)
 {
-    if (fileName.isEmpty())
-        return false;
-
     QImage image = QImageReader(fileName, format).read();
-    if (!image.isNull()) {
     operator=(image);
-        return true;
+    return !isNull();
-    }
-    return false;
 }
 
 /*!
@@ -4384,11 +4379,8 @@ bool QImage::load(const QString &fileName, const char* format)
 bool QImage::load(QIODevice* device, const char* format)
 {
     QImage image = QImageReader(device, format).read();
-    if(!image.isNull()) {
     operator=(image);
-        return true;
+    return !isNull();
-    }
-    return false;
 }
 
 /*!
@@ -4396,7 +4388,7 @@ bool QImage::load(QIODevice* device, const char* format)
 
     Loads an image from the first \a len bytes of the given binary \a
     data. Returns true if the image was successfully loaded; otherwise
-    returns false.
+    invalidates the image and returns false.
 
     The loader attempts to read the image using the specified \a format, e.g.,
     PNG or JPG. If \a format is not specified (which is the default), the
@@ -4408,11 +4400,8 @@ bool QImage::load(QIODevice* device, const char* format)
 bool QImage::loadFromData(const uchar *data, int len, const char *format)
 {
     QImage image = fromData(data, len, format);
-    if (!image.isNull()) {
     operator=(image);
-        return true;
+    return !isNull();
-    }
-    return false;
 }
 
 /*!

src/gui/image/qpicture.cpp

@@ -246,7 +246,7 @@ void QPicture::setData(const char* data, uint size)
 
 /*!
     Loads a picture from the file specified by \a fileName and returns
-    true if successful; otherwise returns false.
+    true if successful; otherwise invalidates the picture and returns false.
 
     Please note that the \a format parameter has been deprecated and
     will have no effect.
@@ -257,8 +257,10 @@ void QPicture::setData(const char* data, uint size)
 bool QPicture::load(const QString &fileName, const char *format)
 {
     QFile f(fileName);
-    if (!f.open(QIODevice::ReadOnly))
+    if (!f.open(QIODevice::ReadOnly)) {
+        operator=(QPicture());
         return false;
+    }
     return load(&f, format);
 }
 
@@ -273,18 +275,14 @@ bool QPicture::load(QIODevice *dev, const char *format)
     if(format) {
 #ifndef QT_NO_PICTUREIO
         QPictureIO io(dev, format);
-        bool result = io.read();
+        if (io.read()) {
-        if (result) {
             operator=(io.picture());
-
+            return true;
-        } else if (format)
-#else
-            bool result = false;
-#endif
-        {
-            qWarning("QPicture::load: No such picture format: %s", format);
         }
-        return result;
+#endif
+        qWarning("QPicture::load: No such picture format: %s", format);
+        operator=(QPicture());
+        return false;
     }
 
     detach();

src/gui/image/qpixmap.cpp

@@ -684,8 +684,8 @@ QBitmap QPixmap::createMaskFromColor(const QColor &maskColor, Qt::MaskMode mode)
 
 /*!
     Loads a pixmap from the file with the given \a fileName. Returns
-    true if the pixmap was successfully loaded; otherwise returns
+    true if the pixmap was successfully loaded; otherwise invalidates
-    false.
+    the pixmap and returns false.
 
     The loader attempts to read the pixmap using the specified \a
     format. If the \a format is not specified (which is the default),
@@ -711,8 +711,10 @@ QBitmap QPixmap::createMaskFromColor(const QColor &maskColor, Qt::MaskMode mode)
 
 bool QPixmap::load(const QString &fileName, const char *format, Qt::ImageConversionFlags flags)
 {
-    if (fileName.isEmpty())
+    if (fileName.isEmpty()) {
+        data.reset();
         return false;
+    }
 
     QFileInfo info(fileName);
     QString key = QLatin1String("qt_pixmap")
@@ -723,19 +725,23 @@ bool QPixmap::load(const QString &fileName, const char *format, Qt::ImageConvers
 
     // Note: If no extension is provided, we try to match the
     // file against known plugin extensions
-    if (!info.completeSuffix().isEmpty() && !info.exists())
+    if (!info.completeSuffix().isEmpty() && !info.exists()) {
+        data.reset();
         return false;
+    }
 
-    if (QPixmapCache::find(key, *this))
+    if (QPixmapCache::find(key, this))
         return true;
 
-    QScopedPointer<QPlatformPixmap> tmp(QPlatformPixmap::create(0, 0, data ? data->type : QPlatformPixmap::PixmapType));
+    if (!data)
-    if (tmp->fromFile(fileName, format, flags)) {
+        data = QPlatformPixmap::create(0, 0, QPlatformPixmap::PixmapType);
-        data = tmp.take();
+
+    if (data->fromFile(fileName, format, flags)) {
         QPixmapCache::insert(key, *this);
         return true;
     }
 
+    data.reset();
     return false;
 }
 
@@ -744,7 +750,7 @@ bool QPixmap::load(const QString &fileName, const char *format, Qt::ImageConvers
 
     Loads a pixmap from the \a len first bytes of the given binary \a
     data.  Returns true if the pixmap was loaded successfully;
-    otherwise returns false.
+    otherwise invalidates the pixmap and returns false.
 
     The loader attempts to read the pixmap using the specified \a
     format. If the \a format is not specified (which is the default),
@@ -760,13 +766,19 @@ bool QPixmap::load(const QString &fileName, const char *format, Qt::ImageConvers
 
 bool QPixmap::loadFromData(const uchar *buf, uint len, const char *format, Qt::ImageConversionFlags flags)
 {
-    if (len == 0 || buf == 0)
+    if (len == 0 || buf == 0) {
+        data.reset();
         return false;
+    }
 
     if (!data)
         data = QPlatformPixmap::create(0, 0, QPlatformPixmap::PixmapType);
 
-    return data->fromData(buf, len, format, flags);
+    if (data->fromData(buf, len, format, flags))
+        return true;
+
+    data.reset();
+    return false;
 }
 
 /*!

tests/auto/gui/image/qimage/tst_qimage.cpp

@@ -95,6 +95,12 @@ private slots:
 
     void copy();
 
+    void load();
+    void loadFromData();
+#if !defined(QT_NO_DATASTREAM)
+    void loadFromDataStream();
+#endif
+
     void setPixel_data();
     void setPixel();
 
@@ -1006,6 +1012,85 @@ void tst_QImage::copy()
     }
 }
 
+void tst_QImage::load()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("image.jpg");
+
+    QImage dest(filePath);
+    QVERIFY(!dest.isNull());
+    QVERIFY(!dest.load("image_that_does_not_exist.png"));
+    QVERIFY(dest.isNull());
+    QVERIFY(dest.load(filePath));
+    QVERIFY(!dest.isNull());
+}
+
+void tst_QImage::loadFromData()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("image.jpg");
+
+    QImage original(filePath);
+    QVERIFY(!original.isNull());
+
+    QByteArray ba;
+    {
+        QBuffer buf(&ba);
+        QVERIFY(buf.open(QIODevice::WriteOnly));
+        QVERIFY(original.save(&buf, "BMP"));
+    }
+    QVERIFY(!ba.isEmpty());
+
+    QImage dest;
+    QVERIFY(dest.loadFromData(ba, "BMP"));
+    QVERIFY(!dest.isNull());
+
+    QCOMPARE(original, dest);
+
+    QVERIFY(!dest.loadFromData(QByteArray()));
+    QVERIFY(dest.isNull());
+}
+
+#if !defined(QT_NO_DATASTREAM)
+void tst_QImage::loadFromDataStream()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("image.jpg");
+
+    QImage original(filePath);
+    QVERIFY(!original.isNull());
+
+    QByteArray ba;
+    {
+        QDataStream s(&ba, QIODevice::WriteOnly);
+        s << original;
+    }
+    QVERIFY(!ba.isEmpty());
+
+    QImage dest;
+    {
+        QDataStream s(&ba, QIODevice::ReadOnly);
+        s >> dest;
+    }
+    QVERIFY(!dest.isNull());
+
+    QCOMPARE(original, dest);
+
+    {
+        ba.clear();
+        QDataStream s(&ba, QIODevice::ReadOnly);
+        s >> dest;
+    }
+    QVERIFY(dest.isNull());
+}
+#endif // QT_NO_DATASTREAM
+
 void tst_QImage::setPixel_data()
 {
     QTest::addColumn<int>("format");

tests/auto/gui/image/qpixmap/tst_qpixmap.cpp

@@ -139,6 +139,12 @@ private slots:
 
     void fromImage_crash();
 
+    void load();
+    void loadFromData();
+#if !defined(QT_NO_DATASTREAM)
+    void loadFromDataStream();
+#endif
+
     void fromData();
     void loadFromDataNullValues();
 
@@ -1191,6 +1197,85 @@ void tst_QPixmap::transformed2()
     QVERIFY(lenientCompare(actual, expected));
 }
 
+void tst_QPixmap::load()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("designer.png");
+
+    QPixmap dest(filePath);
+    QVERIFY(!dest.isNull());
+    QVERIFY(!dest.load("image_that_does_not_exist.png"));
+    QVERIFY(dest.isNull());
+    QVERIFY(dest.load(filePath));
+    QVERIFY(!dest.isNull());
+}
+
+void tst_QPixmap::loadFromData()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("designer.png");
+
+    QPixmap original(filePath);
+    QVERIFY(!original.isNull());
+
+    QByteArray ba;
+    {
+        QBuffer buf(&ba);
+        QVERIFY(buf.open(QIODevice::WriteOnly));
+        QVERIFY(original.save(&buf, "BMP"));
+    }
+    QVERIFY(!ba.isEmpty());
+
+    QPixmap dest;
+    QVERIFY(dest.loadFromData(ba, "BMP"));
+    QVERIFY(!dest.isNull());
+
+    QCOMPARE(original, dest);
+
+    QVERIFY(!dest.loadFromData(QByteArray()));
+    QVERIFY(dest.isNull());
+}
+
+#if !defined(QT_NO_DATASTREAM)
+void tst_QPixmap::loadFromDataStream()
+{
+    const QString prefix = QFINDTESTDATA("images/");
+    if (prefix.isEmpty())
+        QFAIL("can not find images directory!");
+    const QString filePath = prefix + QLatin1String("designer.png");
+
+    QPixmap original(filePath);
+    QVERIFY(!original.isNull());
+
+    QByteArray ba;
+    {
+        QDataStream s(&ba, QIODevice::WriteOnly);
+        s << original;
+    }
+    QVERIFY(!ba.isEmpty());
+
+    QPixmap dest;
+    {
+        QDataStream s(&ba, QIODevice::ReadOnly);
+        s >> dest;
+    }
+    QVERIFY(!dest.isNull());
+
+    QCOMPARE(original, dest);
+
+    {
+        ba.clear();
+        QDataStream s(&ba, QIODevice::ReadOnly);
+        s >> dest;
+    }
+    QVERIFY(dest.isNull());
+}
+#endif // QT_NO_DATASTREAM
+
 void tst_QPixmap::fromImage_crash()
 {
     QImage *img = new QImage(64, 64, QImage::Format_ARGB32_Premultiplied);