[Libreoffice-commits] online.git: loolwsd/ChildProcessSession.cpp loolwsd/LOOLKit.cpp

[Ashod Nakashian]

 loolwsd/ChildProcessSession.cpp |    9 ++++
 loolwsd/LOOLKit.cpp             |   79 ++++++++++++----------------------------
 2 files changed, 33 insertions(+), 55 deletions(-)

New commits:
commit ce64895ceec81f3b28265c635b95c3f4ff636fc5
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Fri Apr 29 21:41:23 2016 -0400

    bccu#1751 deadlock during save
    
    While there are two separate callbacks registered
    (one with lokit and the other with lokitDocument,)
    there is no reason why they should be handled
    separately (and indeed differently).
    
    The lokit callback only sends notifications on
    status indicator (during loading and saving)
    and document password type (if protected).
    
    Due to the different callback handlers
    the status indicator was only sent to the
    first client, not all (as one expects).
    
    Furthermore, because the lokit callback
    was processed on the Core thread, it
    was bound to cause performance and
    thread-safety issues. Specifically it
    deadlocked when another callback was
    in flight when a save issued status
    indicator callback.
    
    By unifying the callbacks and putting
    all callback messages on the message
    queue we avoid all of the above and
    simplify the code.
    
    Change-Id: I5bd790b6ce88b7939186c1ec1dead7fb6cabf7e0
    Reviewed-on: https://gerrit.libreoffice.org/24522
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/ChildProcessSession.cpp b/loolwsd/ChildProcessSession.cpp
index f159ca8..abd18a2 100644
--- a/loolwsd/ChildProcessSession.cpp
+++ b/loolwsd/ChildProcessSession.cpp
@@ -218,6 +218,15 @@ public:
         case LOK_CALLBACK_CONTEXT_MENU:
             _session.sendTextFrame("contextmenu: " + rPayload);
             break;
+        case LOK_CALLBACK_STATUS_INDICATOR_START:
+            _session.sendTextFrame("statusindicatorstart:");
+            break;
+        case LOK_CALLBACK_STATUS_INDICATOR_SET_VALUE:
+            _session.sendTextFrame("statusindicatorsetvalue: " + rPayload);
+            break;
+        case LOK_CALLBACK_STATUS_INDICATOR_FINISH:
+            _session.sendTextFrame("statusindicatorfinish:");
+            break;
         }
     }
 
diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index cb3446a..c62d504 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -575,73 +575,41 @@ public:
     }
 
 private:
-    static void KitCallback(int nType, const char* pPayload, void* pData)
-    {
-        Document* self = reinterpret_cast<Document*>(pData);
-        Log::trace() << "Document::KitCallback "
-                     << LOKitHelper::kitCallbackTypeToString(nType)
-                     << " [" << (pPayload ? pPayload : "") << "]." << Log::end;
-
-        if (self)
-        {
-            std::unique_lock<std::mutex> lock(self->_mutex);
-            for (auto& it: self->_connections)
-            {
-                if (it.second->isRunning())
-                {
-                    auto session = it.second->getSession();
-                    auto sessionLock = session->getLock();
-
-                    switch (nType)
-                    {
-                    case LOK_CALLBACK_STATUS_INDICATOR_START:
-                        session->sendTextFrame("statusindicatorstart:");
-                        break;
-                    case LOK_CALLBACK_STATUS_INDICATOR_SET_VALUE:
-                        session->sendTextFrame("statusindicatorsetvalue: " + std::string(pPayload));
-                        break;
-                    case LOK_CALLBACK_STATUS_INDICATOR_FINISH:
-                        session->sendTextFrame("statusindicatorfinish:");
-                        break;
-                    case LOK_CALLBACK_DOCUMENT_PASSWORD:
-                    case LOK_CALLBACK_DOCUMENT_PASSWORD_TO_MODIFY:
-                        self->setDocumentPassword(nType);
-                        break;
-                    }
-
-                    // Ideally, there would be only one *live* connection at this point of time
-                    // So, just get the first running one and break out.
-                    // TODO: Find a better way to find the correct connection.
-                    break;
-                }
-            }
-        }
-    }
 
     static void ViewCallback(int , const char* , void* )
     {
         //TODO: Delegate the callback.
     }
 
-    static void DocumentCallback(int nType, const char* pPayload, void* pData)
+    static void DocumentCallback(const int nType, const char* pPayload, void* pData)
     {
         Log::trace() << "Document::DocumentCallback "
                      << LOKitHelper::kitCallbackTypeToString(nType)
                      << " [" << (pPayload ? pPayload : "") << "]." << Log::end;
         Document* self = reinterpret_cast<Document*>(pData);
-        if (self)
+        if (self == nullptr)
         {
-            std::unique_lock<std::mutex> lock(self->_mutex);
+            return;
+        }
+
+        std::unique_lock<std::mutex> lock(self->_mutex);
 
-            for (auto& it: self->_connections)
+        if (nType == LOK_CALLBACK_DOCUMENT_PASSWORD_TO_MODIFY ||
+            nType == LOK_CALLBACK_DOCUMENT_PASSWORD)
+        {
+            // Mark the document password type.
+            self->setDocumentPassword(nType);
+            return;
+        }
+
+        for (auto& it: self->_connections)
+        {
+            if (it.second->isRunning())
             {
-                if (it.second->isRunning())
+                auto session = it.second->getSession();
+                if (session)
                 {
-                    auto session = it.second->getSession();
-                    if (session)
-                    {
-                        session->loKitCallback(nType, pPayload);
-                    }
+                    session->loKitCallback(nType, pPayload);
                 }
             }
         }
@@ -740,9 +708,10 @@ private:
 
             if (LIBREOFFICEKIT_HAS(_loKit, registerCallback))
             {
-                _loKit->pClass->registerCallback(_loKit, KitCallback, this);
-                _loKit->pClass->setOptionalFeatures(_loKit, LOK_FEATURE_DOCUMENT_PASSWORD |
-                                                    LOK_FEATURE_DOCUMENT_PASSWORD_TO_MODIFY);
+                _loKit->pClass->registerCallback(_loKit, DocumentCallback, this);
+                const auto flags = LOK_FEATURE_DOCUMENT_PASSWORD
+                                 | LOK_FEATURE_DOCUMENT_PASSWORD_TO_MODIFY;
+                _loKit->pClass->setOptionalFeatures(_loKit, flags);
             }
 
             // Save the provided password with us and the jailed url