bgsave: close race of typing while a background save completes.

We need to mark core unmodified so we can track modifications
to the core that date from after the background save process is
forked.

We avoid telling WSD about our new modification status until
we are sure the background save completed successfully, and only
in the case that we have not been subsequently modified.

Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: I2c9fbce942ff0af2bb727c3685f4ec479e18fa27

kit/ChildSession.cpp

@@ -891,8 +891,6 @@ bool ChildSession::saveDocumentBackground(const StringVector &tokens)
     // Keep the session alive over the lifetime of an async save
     if (!_docManager->forkToSave([this, tokens]{
 
-        sendTextFrame("asyncsave start");
-
         // FIXME: re-directing our sockets perhaps over
         // a pipe to our parent process ?
         unoCommand(tokens);
@@ -900,8 +898,6 @@ bool ChildSession::saveDocumentBackground(const StringVector &tokens)
         // FIXME: did we send our responses properly ? ...
         SigUtil::addActivity("async save process exiting");
 
-        sendTextFrame("asyncsave end");
-
         LOG_TRC("Finished synchronous background saving ...");
         // Next: we wait for an async UNO_COMMAND_RESULT on .uno:Save
         // cf. Document::handleSaveMessage.
@@ -3005,13 +3001,20 @@ void ChildSession::loKitCallback(const int type, const std::string& payload)
         sendTextFrame("hyperlinkclicked: " + payload);
         break;
     case LOK_CALLBACK_STATE_CHANGED:
-        sendTextFrame("statechanged: " + payload);
+    {
+        bool filter = false;
+        if (payload.find(".uno:ModifiedStatus") != std::string::npos)
+            filter = _docManager->trackDocModifiedState(payload);
+
+        if (!filter)
+            sendTextFrame("statechanged: " + payload);
         if (payload.starts_with(".uno:SlideMasterPage"))
         {
             std::string status = LOKitHelper::documentStatus(getLOKitDocument()->get());
             sendTextFrame("status: " + status);
         }
         break;
+    }
     case LOK_CALLBACK_SEARCH_NOT_FOUND:
         sendTextFrame("searchnotfound: " + payload);
         break;
@@ -3034,6 +3037,7 @@ void ChildSession::loKitCallback(const int type, const std::string& payload)
         auto success = object->get("success");
 
         bool saveCommand = false;
+
         if (!commandName.isEmpty() && commandName.toString() == ".uno:Save")
         {
             if (!Util::isMobileApp())

kit/Kit.cpp

@@ -699,6 +699,7 @@ Document::Document(const std::shared_ptr<lok::Office>& loKit,
       _obfuscatedFileId(Util::getFilenameFromURL(docKey)),
       _tileQueue(std::make_shared<TileQueue>()),
       _websocketHandler(websocketHandler),
+      _modified(ModifiedState::UnModified),
       _isBgSaveProcess(false),
       _haveDocPassword(false),
       _isDocPasswordProtected(false),
@@ -1482,6 +1483,8 @@ bool Document::forkToSave(const std::function<void()> &childSave, int viewId)
         parentSocket.reset();
         // now we have a socket to the child: childSocket
 
+        forceDocUnmodifiedForBgSave();
+
         auto bgSaveChild = std::make_shared<BgSaveParentWebSocketHandler>(
             "bgsv_kit_ws", pid, shared_from_this(),
             findSessionByViewId(viewId));
@@ -2274,7 +2277,94 @@ std::shared_ptr<lok::Document> Document::getLOKitDocument()
     return _loKitDocument;
 }
 
-    /// Stops theads, flushes buffers, and exits the process.
+void Document::postForceModifiedCommand(bool modified)
+{
+    std::string args = "{ \"Modified\": { \"type\": \"boolean\", ";
+    args += "\"value\": \"";
+    args += (modified ? "true" : "false");
+    args += "\" } }";
+
+    LOG_TRC("post force modified command: .uno:Modified " << args);
+    getLOKitDocument()->postUnoCommand(
+        ".uno:Modified", args.c_str(), true);
+}
+
+void Document::forceDocUnmodifiedForBgSave()
+{
+    LOG_TRC("force document unmodified from state " << toString(_modified));
+    if (_modified == ModifiedState::Modified)
+    {
+        SigUtil::addActivity("Force clear modified");
+        _modified = ModifiedState::UnModifiedButSaving;
+        // but tell the core we are not modified to track real changes
+        postForceModifiedCommand(false);
+    }
+}
+
+void Document::updateModifiedOnFailedBgSave()
+{
+    if (_modified == ModifiedState::UnModifiedButSaving)
+    {
+        SigUtil::addActivity("Force re-modified");
+        _modified = ModifiedState::Modified;
+        postForceModifiedCommand(true);
+    }
+}
+
+void Document::notifySyntheticUnmodifiedState()
+{
+    // no need to change core state that happened earlier
+    if (_modified == ModifiedState::UnModifiedButSaving)
+    {
+        LOG_TRC("document was not modified while background saving");
+        _modified = ModifiedState::UnModified;
+        notifyAll("statechanged: .uno:ModifiedStatus=false");
+    }
+}
+
+bool Document::trackDocModifiedState(const std::string &stateChanged)
+{
+    bool filter = false;
+
+    StringVector tokens(StringVector::tokenize(stateChanged, '='));
+    bool modified = tokens.size() > 1 && tokens.equals(1, "true");
+    ModifiedState newState = _modified;
+    // NB. since 'modified' state is (oddly) notified per view we get
+    // several duplicate transitions from state A -> A again.
+    switch (_modified) {
+    case ModifiedState::Modified:
+        if (!modified)
+            newState = ModifiedState::UnModified;
+        // else duplicate
+        break;
+    // Only present in background save mode
+    case ModifiedState::UnModifiedButSaving:
+        if (modified)
+        {
+            // now we're really modified
+            newState = ModifiedState::Modified;
+        }
+        else // ignore being notified of our own force unmodification.
+        {
+            LOG_TRC("Ignore self generated unmodified notification");
+            filter = true;
+        }
+        break;
+    case ModifiedState::UnModified:
+        if (modified)
+            newState = ModifiedState::Modified;
+        // else duplicate
+        break;
+    }
+    if (_modified != newState)
+        LOG_TRC("Transition modified state from " <<
+                toString(_modified) << " to " << toString(newState));
+    _modified = newState;
+
+    return filter;
+}
+
+/// Stops theads, flushes buffers, and exits the process.
 void Document::flushAndExit(int code)
 {
     flushTraceEventRecordings();
@@ -2306,6 +2396,7 @@ void Document::dumpState(std::ostream& oss)
         << "\n\tmobileAppDocId: " << _mobileAppDocId
         << "\n\tinputProcessingEnabled: " << processInputEnabled()
         << "\n\tduringLoad: " << _duringLoad
+        << "\n\tmodified: " << toString(_modified)
         << "\n";
 
     // dumpState:

kit/Kit.hpp

@@ -16,6 +16,7 @@
 #include <string>
 
 #include <common/Util.hpp>
+#include <common/StateEnum.hpp>
 #include <common/Session.hpp>
 #include <common/ThreadPool.hpp>
 #include <wsd/TileDesc.hpp>
@@ -215,6 +216,7 @@ public:
 
     void renderTiles(TileCombined& tileCombined);
 
+
     bool sendTextFrame(const std::string& message)
     {
         return sendFrame(message.data(), message.size());
@@ -339,7 +341,27 @@ public:
 
     bool isBackgroundSaveProcess() const { return _isBgSaveProcess; }
 
+    /// Save is async, so we need to set 'unmodified' while we are saving
+    /// but this can transition back to modified if save fails.
+    STATE_ENUM(ModifiedState, Modified, UnModifiedButSaving, UnModified);
+
+    ModifiedState getModified() const { return _modified; }
+
+    /// Lets us get notified whether a doc is modified during bgsave.
+    void forceDocUnmodifiedForBgSave();
+
+    /// Restore the Document's 'modified' state if necessary
+    void updateModifiedOnFailedBgSave();
+
+    /// Let WSD know our true modified state affter bg save success.
+    void notifySyntheticUnmodifiedState();
+
+    /// Snoop document modified, and return true if filtering notification
+    bool trackDocModifiedState(const std::string &stateChanged);
+
 private:
+    void postForceModifiedCommand(bool modified);
+
     /// Stops theads, flushes buffers, and exits the process.
     void flushAndExit(int code);
 
@@ -366,6 +388,7 @@ private:
 
     // Connection a child background save process has to its parent: a precious thing.
     std::weak_ptr<WebSocketHandler> _saveProcessParent;
+    ModifiedState _modified;
     bool _isBgSaveProcess;
 
     // Document password provided

kit/KitWebSocket.cpp

@@ -251,20 +251,13 @@ void BgSaveParentWebSocketHandler::handleMessage(const std::vector<char>& data)
             object->get("commandName").toString() == ".uno:Save")
         {
             if (object->get("success").toString() == "true")
-            {
-                // Force Modified state off, expecting a notification in a bit ...
-                LOG_TRC("Force modified state clear");
-                SigUtil::addActivity("Force clear modified");
-                _document->getLOKitDocument()->postUnoCommand(".uno:Modified", "{ \"Modified\": { \"type\": \"boolean\", \"value\": \"false\" } }", true);
-#if 0
-                // Synthesize modified status change
-                std::string modMsg = tokens[0] + " statechanged: .uno:ModifiedStatus=false";
-                LOG_TRC("Synthesize modified status clear");
-                _document->sendFrame(modMsg.c_str(), modMsg.size(), WSOpCode::Text);
-#endif
-            }
+                _document->notifySyntheticUnmodifiedState();
+
             else
+            {
+                _document->updateModifiedOnFailedBgSave();
                 LOG_DBG("Failed to save, not synthesizing modified state");
+            }
         }
     }
 }