[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-2-1' - kit/ChildSession.cpp kit/ChildSession.hpp kit/Kit.cpp test/WhiteBoxTests.cpp

[Ashod Nakashian]

 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.nakashian at 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 <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>
    (cherry picked from commit 8a2c60e5b6f4f0ad8cac44ba1dd39c7594c7bbde)
    Reviewed-on: https://gerrit.libreoffice.org/38141
    Reviewed-by: Jan Holesovsky <kendy at collabora.com>
    Tested-by: Jan Holesovsky <kendy at 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
     {
     }