From cc60d70699019030a8cba777de5c46b4f4b8b31a Mon Sep 17 00:00:00 2001 From: Mikolaj Boc Date: Mon, 19 Dec 2022 16:41:46 +0100 Subject: [PATCH] Avoid mutual ownership in qstdweb's File::stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mutual ownership of chunkCompleted<->fileReader caused both not to be freed, which resulted in a memory leak. Resolve this by introducing the ChunkedFileReader class which owns itself until file read is finished. Also, resolve a similar issue in qwasmlocalfileaccess. Fixes: QTBUG-109436 Pick-to: 6.5 Change-Id: Ieec4cde15a893fa6a2e21a62d3bb6637374c5364 Reviewed-by: Morten Johan Sørvig --- src/corelib/platform/wasm/qstdweb.cpp | 87 +++++++++++-------- src/corelib/platform/wasm/qstdweb_p.h | 5 +- .../platform/wasm/qwasmlocalfileaccess.cpp | 2 +- src/plugins/platforms/wasm/qwasmdrag.cpp | 6 +- 4 files changed, 59 insertions(+), 41 deletions(-) diff --git a/src/corelib/platform/wasm/qstdweb.cpp b/src/corelib/platform/wasm/qstdweb.cpp index 3d72481a95..720ba6b7c9 100644 --- a/src/corelib/platform/wasm/qstdweb.cpp +++ b/src/corelib/platform/wasm/qstdweb.cpp @@ -34,6 +34,53 @@ static void usePotentialyUnusedSymbols() Q_CONSTRUCTOR_FUNCTION(usePotentialyUnusedSymbols) typedef double uint53_t; // see Number.MAX_SAFE_INTEGER namespace { +// Reads file in chunks in order to avoid holding two copies in memory at the same time +struct ChunkedFileReader +{ +public: + static void read(File file, char *buffer, uint32_t offset, uint32_t end, + std::function onCompleted) + { + (new ChunkedFileReader(end, std::move(onCompleted), std::move(file))) + ->readNextChunk(offset, buffer); + } + +private: + ChunkedFileReader(uint32_t end, std::function onCompleted, File file) + : end(end), onCompleted(std::move(onCompleted)), file(std::move(file)) + { + } + + void readNextChunk(uint32_t chunkBegin, char *chunkBuffer) + { + // Copy current chunk from JS memory to Wasm memory + qstdweb::ArrayBuffer result = fileReader.result(); + qstdweb::Uint8Array(result).copyTo(chunkBuffer); + + // Read next chunk if not at buffer end + const uint32_t nextChunkBegin = std::min(chunkBegin + result.byteLength(), end); + if (nextChunkBegin == end) { + onCompleted(); + delete this; + return; + } + char *nextChunkBuffer = chunkBuffer + result.byteLength(); + fileReader.onLoad([this, nextChunkBegin, nextChunkBuffer](emscripten::val) { + readNextChunk(nextChunkBegin, nextChunkBuffer); + }); + const uint32_t nextChunkEnd = std::min(nextChunkBegin + chunkSize, end); + qstdweb::Blob blob = file.slice(nextChunkBegin, nextChunkEnd); + fileReader.readAsArrayBuffer(blob); + } + + static constexpr uint32_t chunkSize = 256 * 1024; + + qstdweb::FileReader fileReader; + uint32_t end; + std::function onCompleted; + File file; +}; + enum class CallbackType { Then, Catch, @@ -403,47 +450,17 @@ std::string Blob::type() const // Streams partial file content into the given buffer asynchronously. The completed // callback is called on completion. -void File::stream(uint32_t offset, uint32_t length, char *buffer, const std::function &completed) const +void File::stream(uint32_t offset, uint32_t length, char *buffer, + std::function completed) const { - // Read file in chunks in order to avoid holding two copies in memory at the same time - const uint32_t chunkSize = 256 * 1024; - const uint32_t end = offset + length; - // assert end < file.size - auto fileReader = std::make_shared(); - - // "this" is valid now, but may not be by the time the chunkCompleted callback - // below is made. Make a copy of the file handle. - const File fileHandle = *this; - auto chunkCompleted = std::make_shared>(); - *chunkCompleted = [=](uint32_t chunkBegin, char *chunkBuffer) mutable { - - // Copy current chunk from JS memory to Wasm memory - qstdweb::ArrayBuffer result = fileReader->result(); - qstdweb::Uint8Array(result).copyTo(chunkBuffer); - - // Read next chunk if not at buffer end - uint32_t nextChunkBegin = std::min(chunkBegin + result.byteLength(), end); - uint32_t nextChunkEnd = std::min(nextChunkBegin + chunkSize, end); - if (nextChunkBegin == end) { - completed(); - chunkCompleted.reset(); - return; - } - char *nextChunkBuffer = chunkBuffer + result.byteLength(); - fileReader->onLoad([=](emscripten::val) { (*chunkCompleted)(nextChunkBegin, nextChunkBuffer); }); - qstdweb::Blob blob = fileHandle.slice(nextChunkBegin, nextChunkEnd); - fileReader->readAsArrayBuffer(blob); - }; - - // Read first chunk. First iteration is a dummy iteration with no available data. - (*chunkCompleted)(offset, buffer); + ChunkedFileReader::read(*this, buffer, offset, offset + length, std::move(completed)); } // Streams file content into the given buffer asynchronously. The completed // callback is called on completion. -void File::stream(char *buffer, const std::function &completed) const +void File::stream(char *buffer, std::function completed) const { - stream(0, size(), buffer, completed); + stream(0, size(), buffer, std::move(completed)); } std::string File::type() const diff --git a/src/corelib/platform/wasm/qstdweb_p.h b/src/corelib/platform/wasm/qstdweb_p.h index b3fc8a95f7..d8c0db191b 100644 --- a/src/corelib/platform/wasm/qstdweb_p.h +++ b/src/corelib/platform/wasm/qstdweb_p.h @@ -75,8 +75,9 @@ namespace qstdweb { std::string name() const; uint64_t size() const; std::string type() const; - void stream(uint32_t offset, uint32_t length, char *buffer, const std::function &completed) const; - void stream(char *buffer, const std::function &completed) const; + void stream(uint32_t offset, uint32_t length, char *buffer, + std::function completed) const; + void stream(char *buffer, std::function completed) const; emscripten::val val(); private: diff --git a/src/gui/platform/wasm/qwasmlocalfileaccess.cpp b/src/gui/platform/wasm/qwasmlocalfileaccess.cpp index 1b797be9fe..47deb0d553 100644 --- a/src/gui/platform/wasm/qwasmlocalfileaccess.cpp +++ b/src/gui/platform/wasm/qwasmlocalfileaccess.cpp @@ -124,7 +124,7 @@ void readFiles(const qstdweb::FileList &fileList, } // Read file data into caller-provided buffer - file.stream(buffer, [=]() { + file.stream(buffer, [readFile = readFile.get(), fileIndex, fileDataReady]() { fileDataReady(); (*readFile)(fileIndex + 1); }); diff --git a/src/plugins/platforms/wasm/qwasmdrag.cpp b/src/plugins/platforms/wasm/qwasmdrag.cpp index 52c2ddb14e..0f3aa9c751 100644 --- a/src/plugins/platforms/wasm/qwasmdrag.cpp +++ b/src/plugins/platforms/wasm/qwasmdrag.cpp @@ -109,9 +109,9 @@ static void dropEvent(val event) QByteArray fileContent; fileContent.resize(file.size()); - file.stream(fileContent.data(), [=]() { + file.stream(fileContent.data(), [mimeFormat, fileContent]() { if (!fileContent.isEmpty()) { - + QWasmDrag *wasmDrag = static_cast(QWasmIntegration::get()->drag()); if (mimeFormat.contains("image")) { QImage image; image.loadFromData(fileContent, nullptr); @@ -119,7 +119,7 @@ static void dropEvent(val event) } else { wasmDrag->m_mimeData->setData(mimeFormat, fileContent.data()); } - wasmDrag->qWasmDrop(); + wasmDrag->qWasmDrop(); } });