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

[Ashod Nakashian]

 loolwsd/LOOLKit.cpp |  117 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 49 deletions(-)

New commits:
commit 653da3a409faae5d4d6cc8b59f0c9900b70a3764
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sat Apr 2 20:43:02 2016 -0400

    loolwsd: Document::onLoad is now exception safe
    
    In face of exceptions, the lock was not released
    and the condition variable was not signalled,
    thereby causing all subsequent views on the
    same document to fail loading.
    
    Change-Id: I18d3cefcc74a158facefe1e74a9c802ee048b014
    Reviewed-on: https://gerrit.libreoffice.org/23785
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index b37269b..9c56fb7 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -629,7 +629,6 @@ private:
     LibreOfficeKitDocument* onLoad(const std::string& sessionId, const std::string& uri, const std::string& docPassword, bool isDocPasswordProvided)
     {
         Log::info("Session " + sessionId + " is loading. " + std::to_string(_clientViews) + " views loaded.");
-        const unsigned intSessionId = Util::decodeId(sessionId);
 
         std::unique_lock<std::mutex> lock(_mutex);
         while (_isLoading)
@@ -641,10 +640,70 @@ private:
         ++_isLoading;
         lock.unlock();
 
+        try
+        {
+            load(sessionId, uri, docPassword, isDocPasswordProvided);
+        }
+        catch (const std::exception& exc)
+        {
+            Log::error("Exception while loading [" + uri + "] : " + exc.what());
+        }
+
+        // Done loading, let the next one in (if any).
+        lock.lock();
+        ++_clientViews;
+        --_isLoading;
+        _cvLoading.notify_one();
+
+        return _loKitDocument;
+    }
+
+    void onUnload(const std::string& sessionId)
+    {
+        const unsigned intSessionId = Util::decodeId(sessionId);
+        const auto it = _connections.find(intSessionId);
+        if (it == _connections.end() || !it->second || !_loKitDocument)
+        {
+            // Nothing to do.
+            return;
+        }
+
+        auto session = it->second->getSession();
+        auto sessionLock = session->getLock();
+        std::unique_lock<std::mutex> lock(_mutex);
+
+        --_clientViews;
+
+        std::ostringstream message;
+        message << "rmview" << " "
+                << Process::id() << " "
+                << sessionId << " "
+                << "\n";
+        IoUtil::writeFIFO(WriterNotify, message.str());
+
+        Log::info("Session " + sessionId + " is unloading. " + std::to_string(_clientViews) + " views will remain.");
+
+        if (_multiView && _loKitDocument)
+        {
+            Log::info() << "Document [" << _url << "] session ["
+                        << sessionId << "] unloaded, leaving "
+                        << _clientViews << " views." << Log::end;
+
+            const auto viewId = _loKitDocument->pClass->getView(_loKitDocument);
+            _loKitDocument->pClass->registerCallback(_loKitDocument, nullptr, nullptr);
+            _loKitDocument->pClass->destroyView(_loKitDocument, viewId);
+        }
+    }
+
+private:
+
+    LibreOfficeKitDocument* load(const std::string& sessionId, const std::string& uri, const std::string& docPassword, bool isDocPasswordProvided)
+    {
+        const unsigned intSessionId = Util::decodeId(sessionId);
         const auto it = _connections.find(intSessionId);
         if (it == _connections.end() || !it->second)
         {
-            Log::error("Cannot find session [" + sessionId + "] which decoded to " + std::to_string(intSessionId));
+            Log::error("Cannot find session [" + sessionId + "].");
             return nullptr;
         }
 
@@ -667,8 +726,12 @@ private:
             _docPassword = docPassword;
             _jailedUrl = uri;
             _isDocPasswordProtected = false;
-            Log::info("Calling _loKit->pClass->documentLoad");
-            if ((_loKitDocument = _loKit->pClass->documentLoad(_loKit, uri.c_str())) == nullptr)
+
+            Log::debug("Calling documentLoad");
+            _loKitDocument = _loKit->pClass->documentLoad(_loKit, uri.c_str());
+            Log::debug("documentLoad returned");
+
+            if (_loKitDocument == nullptr)
             {
                 Log::error("Failed to load: " + uri + ", error: " + _loKit->pClass->getError(_loKit));
 
@@ -690,7 +753,6 @@ private:
 
                 return nullptr;
             }
-            Log::info("documentLoad() returned");
 
             // Notify the Admin thread
             std::ostringstream message;
@@ -739,59 +801,16 @@ private:
             }
         }
 
-        // Done loading, let the next one in (if any).
-        lock.lock();
-        ++_clientViews;
-        --_isLoading;
-        _cvLoading.notify_one();
-
         std::ostringstream message;
         message << "addview" << " "
                 << Process::id() << " "
                 << sessionId << " "
-                << "\r\n";
+                << "\n";
         IoUtil::writeFIFO(WriterNotify, message.str());
 
         return _loKitDocument;
     }
 
-    void onUnload(const std::string& sessionId)
-    {
-        const unsigned intSessionId = Util::decodeId(sessionId);
-        const auto it = _connections.find(intSessionId);
-        if (it == _connections.end() || !it->second || !_loKitDocument)
-        {
-            // Nothing to do.
-            return;
-        }
-
-        auto session = it->second->getSession();
-        auto sessionLock = session->getLock();
-        std::unique_lock<std::mutex> lock(_mutex);
-
-        --_clientViews;
-
-        std::ostringstream message;
-        message << "rmview" << " "
-                << Process::id() << " "
-                << sessionId << " "
-                << "\r\n";
-        IoUtil::writeFIFO(WriterNotify, message.str());
-
-        Log::info("Session " + sessionId + " is unloading. " + std::to_string(_clientViews) + " views will remain.");
-
-        if (_multiView && _loKitDocument)
-        {
-            Log::info() << "Document [" << _url << "] session ["
-                        << sessionId << "] unloaded, leaving "
-                        << _clientViews << " views." << Log::end;
-
-            const auto viewId = _loKitDocument->pClass->getView(_loKitDocument);
-            _loKitDocument->pClass->registerCallback(_loKitDocument, nullptr, nullptr);
-            _loKitDocument->pClass->destroyView(_loKitDocument, viewId);
-        }
-    }
-
 private:
 
     const bool _multiView;