kit/ChildSession.cpp | 20 ++++----------------
kit/ChildSession.hpp | 5 +++--
kit/Kit.cpp | 37 +++++++++++++++++--------------------
test/WhiteBoxTests.cpp | 2 +-
4 files changed, 25 insertions(+), 39 deletions(-)
New commits:
commit 5779e468ade9409e46c736b929d374485138932a
Author: Ashod Nakashian <ashod.nakash...@collabora.co.uk>
Date: Sun May 28 23:59:12 2017 -0400
wsd: avoid race during viewinfo notification
Previously the call to notifyViewInfo is done with
the lock taken. Previously, the call was done outside
lock, which allowed for a change to the users list
before sending the previous one.
Change-Id: If2d8adc67337a5529cb6898808a84727ff1df38e
Reviewed-on: https://gerrit.libreoffice.org/38123
Reviewed-by: Ashod Nakashian <ashnak...@gmail.com>
Tested-by: Ashod Nakashian <ashnak...@gmail.com>
(cherry picked from commit 8a2c60e5b6f4f0ad8cac44ba1dd39c7594c7bbde)
Reviewed-on: https://gerrit.libreoffice.org/38141
Reviewed-by: Jan Holesovsky <ke...@collabora.com>
Tested-by: Jan Holesovsky <ke...@collabora.com>
diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp
index 1fbc1944..d68bcf59 100644
--- a/kit/ChildSession.cpp
+++ b/kit/ChildSession.cpp
@@ -97,19 +97,14 @@ bool ChildSession::_handleInput(const char *buffer, int
length)
getLOKitDocument()->setView(_viewId);
- // Get the list of view ids from the core
- const int viewCount = getLOKitDocument()->getViewsCount();
- std::vector<int> viewIds(viewCount);
- getLOKitDocument()->getViewIds(viewIds.data(), viewCount);
-
int curPart = 0;
if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT)
curPart = getLOKitDocument()->getPart();
- lockLokDoc.unlock();
-
// Notify all views about updated view info
- _docManager.notifyViewInfo(viewIds);
+ _docManager.notifyViewInfo();
+
+ lockLokDoc.unlock();
if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT)
{
@@ -370,15 +365,8 @@ bool ChildSession::loadDocument(const char * /*buffer*/,
int /*length*/, const s
return false;
}
- // Get the list of view ids from the core
- const int viewCount = getLOKitDocument()->getViewsCount();
- std::vector<int> viewIds(viewCount);
- getLOKitDocument()->getViewIds(viewIds.data(), viewCount);
-
- lockLokDoc.unlock();
-
// Inform everyone (including this one) about updated view info
- _docManager.notifyViewInfo(viewIds);
+ _docManager.notifyViewInfo();
LOG_INF("Loaded session " << getId());
return true;
diff --git a/kit/ChildSession.hpp b/kit/ChildSession.hpp
index b1fad059..703df776 100644
--- a/kit/ChildSession.hpp
+++ b/kit/ChildSession.hpp
@@ -45,8 +45,9 @@ public:
/// Access to the document instance.
virtual std::shared_ptr<lok::Document> getLOKitDocument() = 0;
- /// Send updated view info to all active sessions
- virtual void notifyViewInfo(const std::vector<int>& viewIds) = 0;
+ /// Send updated view info to all active sessions.
+ virtual void notifyViewInfo() = 0;
+
/// Get a view ID <-> UserInfo map.
virtual std::map<int, UserInfo> getViewInfo() = 0;
virtual std::mutex& getMutex() = 0;
diff --git a/kit/Kit.cpp b/kit/Kit.cpp
index 628e9295..304a62e5 100644
--- a/kit/Kit.cpp
+++ b/kit/Kit.cpp
@@ -1022,14 +1022,8 @@ private:
if (viewCount > 0)
{
- // Get the list of view ids from the core
- std::vector<int> viewIds(viewCount);
- _loKitDocument->getViewIds(viewIds.data(), viewCount);
-
- lockLokDoc.unlock();
-
// Broadcast updated view info
- notifyViewInfo(viewIds);
+ notifyViewInfo();
}
}
@@ -1051,11 +1045,18 @@ private:
}
/// Notify all views of viewId and their associated usernames
- void notifyViewInfo(const std::vector<int>& viewIds) override
+ void notifyViewInfo() override
{
- // Store the list of viewid, username mapping in a map
- std::map<int, UserInfo> viewInfoMap = getViewInfo();
- std::map<std::string, int> viewColorsMap = getViewColors();
+ Util::assertIsLocked(_documentMutex);
+
+ // Get the list of view ids from the core
+ const int viewCount = getLOKitDocument()->getViewsCount();
+ std::vector<int> viewIds(viewCount);
+ getLOKitDocument()->getViewIds(viewIds.data(), viewCount);
+
+ const std::map<int, UserInfo> viewInfoMap = _sessionUserInfo;
+
+ const std::map<std::string, int> viewColorsMap = getViewColors();
// Double check if list of viewids from core and our list matches,
// and create an array of JSON objects containing id and username
@@ -1101,17 +1102,13 @@ private:
// Get the color value for all author names from the core
std::map<std::string, int> getViewColors()
{
- std::string colorValues;
- std::map<std::string, int> viewColors;
-
- {
- std::unique_lock<std::mutex> lock(_documentMutex);
+ Util::assertIsLocked(_documentMutex);
- char* values =
_loKitDocument->getCommandValues(".uno:TrackedChangeAuthors");
- colorValues = std::string(values == nullptr ? "" : values);
- std::free(values);
- }
+ char* values =
_loKitDocument->getCommandValues(".uno:TrackedChangeAuthors");
+ const std::string colorValues = std::string(values == nullptr ? "" :
values);
+ std::free(values);
+ std::map<std::string, int> viewColors;
try
{
if (!colorValues.empty())
diff --git a/test/WhiteBoxTests.cpp b/test/WhiteBoxTests.cpp
index 9258b484..825ebb4e 100644
--- a/test/WhiteBoxTests.cpp
+++ b/test/WhiteBoxTests.cpp
@@ -335,7 +335,7 @@ public:
return nullptr;
}
- void notifyViewInfo(const std::vector<int>& /*viewIds*/) override
+ void notifyViewInfo() override
{
}
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
