diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp index d8255442e..60cf4c087 100644 --- a/kit/ChildSession.cpp +++ b/kit/ChildSession.cpp @@ -122,7 +122,6 @@ bool ChildSession::_handleInput(const char *buffer, int length) // Client is getting active again. // Send invalidation and other sync-up messages. std::unique_lock lock(Mutex); //TODO: Move to top of function? - std::unique_lock lockLokDoc(_docManager.getDocumentMutex()); getLOKitDocument()->setView(_viewId); @@ -133,8 +132,6 @@ bool ChildSession::_handleInput(const char *buffer, int length) // Notify all views about updated view info _docManager.notifyViewInfo(); - lockLokDoc.unlock(); - if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT) { sendTextFrame("curpart: part=" + std::to_string(curPart)); @@ -456,13 +453,9 @@ bool ChildSession::uploadSignedDocument(const char* buffer, int length, const st const Poco::Path filenameParam(filename); const std::string url = JAILED_DOCUMENT_ROOT + tmpDir + "/" + filenameParam.getFileName(); - { - std::unique_lock lock(_docManager.getDocumentMutex()); - - getLOKitDocument()->saveAs(url.c_str(), - filetype.empty() ? nullptr : filetype.c_str(), - nullptr); - } + getLOKitDocument()->saveAs(url.c_str(), + filetype.empty() ? nullptr : filetype.c_str(), + nullptr); Authorization authorization(Authorization::Type::Token, token); Poco::URI uriObject(wopiUrl + "/" + filename + "/contents"); @@ -573,8 +566,6 @@ bool ChildSession::loadDocument(const char * /*buffer*/, int /*length*/, const s LOG_INF("Created new view with viewid: [" << _viewId << "] for username: [" << getUserNameAnonym() << "] in session: [" << getId() << "]."); - std::unique_lock lockLokDoc(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); _docType = LOKitHelper::getDocumentTypeAsString(getLOKitDocument()->get()); @@ -638,13 +629,9 @@ bool ChildSession::sendFontRendering(const char* /*buffer*/, int /*length*/, con int width = 0, height = 0; unsigned char* ptrFont = nullptr; - { - std::unique_lock lock(_docManager.getDocumentMutex()); + getLOKitDocument()->setView(_viewId); - getLOKitDocument()->setView(_viewId); - - ptrFont = getLOKitDocument()->renderFont(decodedFont.c_str(), decodedChar.c_str(), &width, &height); - } + ptrFont = getLOKitDocument()->renderFont(decodedFont.c_str(), decodedChar.c_str(), &width, &height); LOG_TRC("renderFont [" << font << "] rendered in " << (timestamp.elapsed()/1000.) << "ms"); @@ -671,13 +658,10 @@ bool ChildSession::sendFontRendering(const char* /*buffer*/, int /*length*/, con bool ChildSession::getStatus(const char* /*buffer*/, int /*length*/) { std::string status; - { - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); + getLOKitDocument()->setView(_viewId); - status = LOKitHelper::documentStatus(getLOKitDocument()->get()); - } + status = LOKitHelper::documentStatus(getLOKitDocument()->get()); if (status.empty()) { @@ -731,8 +715,6 @@ bool ChildSession::getCommandValues(const char* /*buffer*/, int /*length*/, cons return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); if (command == ".uno:DocumentRepair") @@ -775,8 +757,6 @@ bool ChildSession::clientZoom(const char* /*buffer*/, int /*length*/, const std: return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setClientZoom(tilePixelWidth, tilePixelHeight, tileTwipWidth, tileTwipHeight); @@ -800,8 +780,6 @@ bool ChildSession::clientVisibleArea(const char* /*buffer*/, int /*length*/, con return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setClientVisibleArea(x, y, width, height); @@ -828,8 +806,6 @@ bool ChildSession::outlineState(const char* /*buffer*/, int /*length*/, const st bool column = type == "column"; bool hidden = state == "hidden"; - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setOutlineState(column, level, index, hidden); @@ -875,17 +851,13 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const std: const std::string nameAnonym = anonymizeUrl(name); const std::string urlAnonym = JAILED_DOCUMENT_ROOT + tmpDir + "/" + Poco::Path(nameAnonym).getFileName(); - { - std::unique_lock lock(_docManager.getDocumentMutex()); + LOG_DBG("Calling LOK's downloadAs with: url='" << urlAnonym << "', format='" << + (format.empty() ? "(nullptr)" : format.c_str()) << "', ' filterOptions=" << + (filterOptions.empty() ? "(nullptr)" : filterOptions.c_str()) << "'."); - LOG_DBG("Calling LOK's downloadAs with: url='" << urlAnonym << "', format='" << - (format.empty() ? "(nullptr)" : format.c_str()) << "', ' filterOptions=" << - (filterOptions.empty() ? "(nullptr)" : filterOptions.c_str()) << "'."); - - getLOKitDocument()->saveAs(url.c_str(), - format.empty() ? nullptr : format.c_str(), - filterOptions.empty() ? nullptr : filterOptions.c_str()); - } + getLOKitDocument()->saveAs(url.c_str(), + format.empty() ? nullptr : format.c_str(), + filterOptions.empty() ? nullptr : filterOptions.c_str()); sendTextFrame("downloadas: jail=" + _jailId + " dir=" + tmpDir + " name=" + name + " port=" + std::to_string(ClientPortNumber) + " id=" + id); @@ -901,13 +873,10 @@ bool ChildSession::getChildId() std::string ChildSession::getTextSelectionInternal(const std::string& mimeType) { char* textSelection = nullptr; - { - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); + getLOKitDocument()->setView(_viewId); - textSelection = getLOKitDocument()->getTextSelection(mimeType.c_str(), nullptr); - } + textSelection = getLOKitDocument()->getTextSelection(mimeType.c_str(), nullptr); std::string str(textSelection ? textSelection : ""); free(textSelection); @@ -944,8 +913,6 @@ bool ChildSession::paste(const char* buffer, int length, const std::vector 0) { - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->paste(mimeType.c_str(), data, size); @@ -1005,8 +972,6 @@ bool ChildSession::insertFile(const char* /*buffer*/, int /*length*/, const std: "\"value\":\"" + url + "\"" "}}"; - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); LOG_TRC("Inserting graphic: '" << arguments.c_str() << "', '"); @@ -1037,7 +1002,6 @@ bool ChildSession::extTextInputEvent(const char* /*buffer*/, int /*length*/, std::string decodedText; URI::decode(text, decodedText); - std::unique_lock lock(_docManager.getDocumentMutex()); getLOKitDocument()->setView(_viewId); getLOKitDocument()->postWindowExtTextInputEvent(id, type, decodedText.c_str()); @@ -1092,7 +1056,6 @@ bool ChildSession::keyEvent(const char* /*buffer*/, int /*length*/, return true; } - std::unique_lock lock(_docManager.getDocumentMutex()); getLOKitDocument()->setView(_viewId); if (target == LokEventTargetEnum::Document) getLOKitDocument()->postKeyEvent(type, charcode, keycode); @@ -1132,8 +1095,6 @@ bool ChildSession::gestureEvent(const char* /*buffer*/, int /*length*/, return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->postWindowGestureEvent(windowID, type.c_str(), x, y, offset); @@ -1194,8 +1155,6 @@ bool ChildSession::mouseEvent(const char* /*buffer*/, int /*length*/, return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); switch (target) { @@ -1226,8 +1185,6 @@ bool ChildSession::unoCommand(const char* /*buffer*/, int /*length*/, const std: tokens[1] == ".uno:Redo" || Util::startsWith(tokens[1], "vnd.sun.star.script:")); - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); if (tokens.size() == 2) @@ -1270,8 +1227,6 @@ bool ChildSession::selectText(const char* /*buffer*/, int /*length*/, const std: return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setTextSelection(type, x, y); @@ -1283,7 +1238,6 @@ bool ChildSession::renderWindow(const char* /*buffer*/, int /*length*/, const st { const unsigned winId = (tokens.size() > 1 ? std::stoul(tokens[1]) : 0); - std::unique_lock lock(_docManager.getDocumentMutex()); getLOKitDocument()->setView(_viewId); int startX = 0, startY = 0; @@ -1354,7 +1308,6 @@ bool ChildSession::sendWindowCommand(const char* /*buffer*/, int /*length*/, con { const unsigned winId = (tokens.size() > 1 ? std::stoul(tokens[1]) : 0); - std::unique_lock lock(_docManager.getDocumentMutex()); getLOKitDocument()->setView(_viewId); if (tokens.size() > 2 && tokens[2] == "close") @@ -1529,11 +1482,7 @@ bool ChildSession::exportSignAndUploadDocument(const char* buffer, int length, c const Poco::Path filenameParam(filename); const std::string aTempDocumentURL = JAILED_DOCUMENT_ROOT + aTempDir + "/" + filenameParam.getFileName(); - { - std::unique_lock lock(_docManager.getDocumentMutex()); - - getLOKitDocument()->saveAs(aTempDocumentURL.c_str(), filetype.c_str(), nullptr); - } + getLOKitDocument()->saveAs(aTempDocumentURL.c_str(), filetype.c_str(), nullptr); // sign document { @@ -1624,8 +1573,6 @@ bool ChildSession::exportSignAndUploadDocument(const char* buffer, int length, c bool ChildSession::askSignatureStatus(const char* buffer, int length, const std::vector& /*tokens*/) { - std::unique_lock lock(_docManager.getDocumentMutex()); - bool bResult = true; const std::string firstLine = getFirstLine(buffer, length); @@ -1676,8 +1623,6 @@ bool ChildSession::selectGraphic(const char* /*buffer*/, int /*length*/, const s return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setGraphicSelection(type, x, y); @@ -1693,8 +1638,6 @@ bool ChildSession::resetSelection(const char* /*buffer*/, int /*length*/, const return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->resetSelection(); @@ -1743,51 +1686,48 @@ bool ChildSession::saveAs(const char* /*buffer*/, int /*length*/, const std::vec } bool success = false; - { - std::unique_lock lock(_docManager.getDocumentMutex()); - if (filterOptions.empty() && format == "html") + if (filterOptions.empty() && format == "html") + { + // Opt-in to avoid linked images, those would not leave the chroot. + filterOptions = "EmbedImages"; + } + + // We don't have the FileId at this point, just a new filename to save-as. + // So here the filename will be obfuscated with some hashing, which later will + // get a proper FileId that we will use going forward. + LOG_DBG("Calling LOK's saveAs with: '" << anonymizeUrl(wopiFilename) << "', '" << + (format.size() == 0 ? "(nullptr)" : format.c_str()) << "', '" << + (filterOptions.size() == 0 ? "(nullptr)" : filterOptions.c_str()) << "'."); + + getLOKitDocument()->setView(_viewId); + + success = getLOKitDocument()->saveAs(url.c_str(), + format.empty() ? nullptr : format.c_str(), + filterOptions.empty() ? nullptr : filterOptions.c_str()); + + if (!success) + { + // a desperate try - add an extension hoping that it'll help + bool retry = true; + switch (getLOKitDocument()->getDocumentType()) { - // Opt-in to avoid linked images, those would not leave the chroot. - filterOptions = "EmbedImages"; + case LOK_DOCTYPE_TEXT: url += ".odt"; wopiFilename += ".odt"; break; + case LOK_DOCTYPE_SPREADSHEET: url += ".ods"; wopiFilename += ".ods"; break; + case LOK_DOCTYPE_PRESENTATION: url += ".odp"; wopiFilename += ".odp"; break; + case LOK_DOCTYPE_DRAWING: url += ".odg"; wopiFilename += ".odg"; break; + default: retry = false; break; } - // We don't have the FileId at this point, just a new filename to save-as. - // So here the filename will be obfuscated with some hashing, which later will - // get a proper FileId that we will use going forward. - LOG_DBG("Calling LOK's saveAs with: '" << anonymizeUrl(wopiFilename) << "', '" << - (format.size() == 0 ? "(nullptr)" : format.c_str()) << "', '" << - (filterOptions.size() == 0 ? "(nullptr)" : filterOptions.c_str()) << "'."); - - getLOKitDocument()->setView(_viewId); - - success = getLOKitDocument()->saveAs(url.c_str(), - format.empty() ? nullptr : format.c_str(), - filterOptions.empty() ? nullptr : filterOptions.c_str()); - - if (!success) + if (retry) { - // a desperate try - add an extension hoping that it'll help - bool retry = true; - switch (getLOKitDocument()->getDocumentType()) - { - case LOK_DOCTYPE_TEXT: url += ".odt"; wopiFilename += ".odt"; break; - case LOK_DOCTYPE_SPREADSHEET: url += ".ods"; wopiFilename += ".ods"; break; - case LOK_DOCTYPE_PRESENTATION: url += ".odp"; wopiFilename += ".odp"; break; - case LOK_DOCTYPE_DRAWING: url += ".odg"; wopiFilename += ".odg"; break; - default: retry = false; break; - } + LOG_DBG("Retry: calling LOK's saveAs with: '" << url.c_str() << "', '" << + (format.size() == 0 ? "(nullptr)" : format.c_str()) << "', '" << + (filterOptions.size() == 0 ? "(nullptr)" : filterOptions.c_str()) << "'."); - if (retry) - { - LOG_DBG("Retry: calling LOK's saveAs with: '" << url.c_str() << "', '" << - (format.size() == 0 ? "(nullptr)" : format.c_str()) << "', '" << - (filterOptions.size() == 0 ? "(nullptr)" : filterOptions.c_str()) << "'."); - - success = getLOKitDocument()->saveAs(url.c_str(), - format.size() == 0 ? nullptr :format.c_str(), - filterOptions.size() == 0 ? nullptr : filterOptions.c_str()); - } + success = getLOKitDocument()->saveAs(url.c_str(), + format.size() == 0 ? nullptr :format.c_str(), + filterOptions.size() == 0 ? nullptr : filterOptions.c_str()); } } @@ -1813,8 +1753,6 @@ bool ChildSession::setClientPart(const char* /*buffer*/, int /*length*/, const s return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT && part != getLOKitDocument()->getPart()) @@ -1835,8 +1773,6 @@ bool ChildSession::setPage(const char* /*buffer*/, int /*length*/, const std::ve return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); getLOKitDocument()->setPart(page); @@ -1854,8 +1790,6 @@ bool ChildSession::renderShapeSelection(const char* /*buffer*/, int /*length*/, return false; } - std::unique_lock lock(_docManager.getDocumentMutex()); - getLOKitDocument()->setView(_viewId); char* pOutput = nullptr; @@ -2079,7 +2013,6 @@ void ChildSession::loKitCallback(const int type, const std::string& payload) { //TODO: clenaup and merge. - std::unique_lock lock(_docManager.getDocumentMutex()); const int parts = getLOKitDocument()->getParts(); for (int i = 0; i < parts; ++i) { @@ -2091,8 +2024,6 @@ void ChildSession::loKitCallback(const int type, const std::string& payload) " height=" + std::to_string(INT_MAX)); } - lock.unlock(); - getStatus("", 0); } break; diff --git a/kit/ChildSession.hpp b/kit/ChildSession.hpp index 597002336..215d1ad4b 100644 --- a/kit/ChildSession.hpp +++ b/kit/ChildSession.hpp @@ -68,10 +68,6 @@ public: virtual std::map getViewInfo() = 0; virtual std::mutex& getMutex() = 0; - /// Mutex guarding the document - so that we can lock operations like - /// setting a view followed by a tile render, etc. - virtual std::mutex& getDocumentMutex() = 0; - virtual std::string getObfuscatedFileId() = 0; virtual std::shared_ptr& getTileQueue() = 0; diff --git a/kit/Kit.cpp b/kit/Kit.cpp index b0f3e32c2..2f0d63f21 100644 --- a/kit/Kit.cpp +++ b/kit/Kit.cpp @@ -1021,8 +1021,6 @@ public: " passwordProvided=" << _haveDocPassword << " password='" << _docPassword << "'"); - Util::assertIsLocked(_documentMutex); - if (_isDocPasswordProtected && _haveDocPassword) { // it means this is the second attempt with the wrong password; abort the load operation @@ -1098,7 +1096,6 @@ public: const size_t pixmapSize = 4 * pixmapWidth * pixmapHeight; std::vector pixmap(pixmapSize, 0); - std::unique_lock lock(_documentMutex); if (!_loKitDocument) { LOG_ERR("Tile rendering requested before loading document."); @@ -1516,7 +1513,6 @@ private: const int viewId = session.getViewId(); _tileQueue->removeCursorPosition(viewId); - std::unique_lock lockLokDoc(_documentMutex); if (_loKitDocument == nullptr) { LOG_ERR("Unloading session [" << sessionId << "] without loKitDocument."); @@ -1591,8 +1587,6 @@ private: /// Notify all views of viewId and their associated usernames void notifyViewInfo() override { - Util::assertIsLocked(_documentMutex); - // Get the list of view ids from the core const int viewCount = getLOKitDocument()->getViewsCount(); std::vector viewIds(viewCount); @@ -1693,8 +1687,6 @@ private: // Get the color value for all author names from the core std::map getViewColors() { - Util::assertIsLocked(_documentMutex); - char* values = _loKitDocument->getCommandValues(".uno:TrackedChangeAuthors"); const std::string colorValues = std::string(values == nullptr ? "" : values); std::free(values); @@ -1745,8 +1737,6 @@ private: if (!lang.empty()) options = "Language=" + lang; - std::unique_lock lock(_documentMutex); - if (!_loKitDocument) { // This is the first time we are loading the document @@ -2133,12 +2123,6 @@ private: return _loKitDocument; } - /// Return access to the lok::Document instance. - std::mutex& getDocumentMutex() override - { - return _documentMutex; - } - std::string getObfuscatedFileId() override { return _obfuscatedFileId; @@ -2178,10 +2162,6 @@ private: std::atomic _stop; mutable std::mutex _mutex; - /// Mutex guarding the lok::Document so that we can lock operations - /// like setting a view followed by a tile render, etc. - std::mutex _documentMutex; - ThreadPool _pngPool; std::condition_variable _cvLoading; diff --git a/test/WhiteBoxTests.cpp b/test/WhiteBoxTests.cpp index 21ca04bd1..38bb627d0 100644 --- a/test/WhiteBoxTests.cpp +++ b/test/WhiteBoxTests.cpp @@ -542,11 +542,6 @@ public: return _mutex; } - std::mutex& getDocumentMutex() override - { - return _mutex; - } - std::string getObfuscatedFileId() override { return std::string();