Revise saving .omap files

[dl3sdo]

In the past 18 months two corrupted .omap files were reported where the analysis showed that the corruption happened while writing object coordinates.

Although Mapper uses the QSaveFile class to ensure (by using a temporary file) that an .omap file is only overwritten when saving was successful, there is a weakness when saving object coordinates.

void XmlElementWriter::write(const MapCoordVector& coords) in xml_stream_util.cpp
bypasses the safe writeData mechanism by directly accessing QIODevice

else if (auto* device = xml.device())
{
	// Default: efficient plain text format
	// Direct UTF-8 writing without unnecessary allocations, escaping or
	// conversions, but also without handling of device errors.
	xml.writeCharacters({});  // Finish the start element
	MapCoord::StringBuffer<char> buffer;
	for (auto& coord : coords)
		device->write(coord.toUtf8(buffer));
}

Although the return value of exportImplementation() below is checked, the actual implementation in bool XMLFileExporter::exportImplementation() always returns true.

auto success = true;
// Save the map
try
{
	if (!exportImplementation())
	{
		Q_ASSERT(!warnings().empty());
		success = false;
	}
	if (success && managed_file && !managed_file->commit())
	{
		addWarning(managed_file->errorString());
		success = false;
	}

managed_file->commit() will not be aware of any errors that occur when not using QSaveFile::writeData(const char *data, qint64 len)
(see qsavefile.cpp).

[dl3sdo]

Note that the description of QSaveFile states that

Data is usually read and written using QDataStream or QTextStream, but you can also call the QIODevice-inherited functions read(), readLine(), readAll(), write().

which would allow to use device->write(coord.toUtf8(buffer))
IMO referring to file reading in a description of QSaveFile sounds weird to me.

However:

By subclassing QIODevice, you can provide the same interface to
your own I/O devices. Subclasses of QIODevice are only required to
implement the protected readData() and writeData() functions.
QIODevice uses these functions to implement all its convenience
functions, such as getChar(), readLine() and write().

Thus using QIODevice::write() will result in finally calling QSaveFile::writeData().

[dg0yt]

writeData is just an internal implementation of the io device classes (protected).

But the interaction of XML classes and direct writes is a sensitive point.
I wonder if these rare problems occured with particular version or distribution of Qt.

[dl3sdo]

Is there a reason (other than performance) for not always using the XML class access in the else path?

else
{
	// Default: efficient plain text format
	MapCoord::StringBuffer<QChar> buffer;
	for (auto& coord : coords)
		xml.writeCharacters(coord.toString(buffer));
}

[dg0yt]

Is there a reason (other than performance) for not always using the XML class access in the else path?

You want avoid additional memory pressure when saving under memory pressure.
(And do we already auto-save when Android decides to close the app? That's when time is limited IIRC.)

[dl3sdo]

Three examples for data corruption:

• 497113 2484443 1;497410 2484286;497613 2482988<0xA6><0xF6>Bfv<0xA1><0x8A>.....
• -18028 -85514 1;-19187 -83005;-17642 -8237ffffffffffffffffffffffff....
• 203642 35210 1;204271 34823;204924 35585;205206 34585 1;<0x00><0x00><0x00><0x00><0x00>.....