wsd: do not upload unmodified document

When always_save_on_exit=true we should
still not upload the document when it
isn't modified.

In this case, because we now always
save the document (forced) when
always_save_on_exit=true, and because
saving always generates a new file on
disk, with a new timestamp, we couldn't
detect that there are no modifications.

We now still force save, but ask Core
to skip it if the document is unmodified.

This is safe since we now always issue
the save, but rely on Core to do the
right thing. When the document is saved,
we do the normal upload as in that case
we know we have a new version of the
document, which must be uploaded.

Worth noting that the closedocument
command doesn't trigger the same path.
To reproduce the issue, we need a new
test that disconnects, instead of the
graceful closedocument command.

Change-Id: Iaa4e0363ed2eca124f2d1943393e65c0c187aa18
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>

test/UnitWOPISaveOnExit.cpp

@@ -308,17 +308,19 @@ public:
 
 /// Test upload behavior with always_save_on_exit.
 /// The test verifies that an unmodified document
-/// is *not* uploaded when always_save_on_exit=true.
-class UnitSaveOnExitUnmodified : public WopiTestServer
+/// is *not* uploaded when always_save_on_exit=true
+/// and is closed (ownertermination).
+class UnitSaveOnExitUnmodifiedClosed : public WopiTestServer
 {
     using Base = WopiTestServer;
 
+protected:
     STATE_ENUM(Phase, Load, WaitLoadStatus, WaitDestroy, Done)
     _phase;
 
 public:
-    UnitSaveOnExitUnmodified()
-        : Base("UnitSaveOnExitUnmodified")
+    UnitSaveOnExitUnmodifiedClosed(const std::string& name = "UnitSaveOnExitUnmodifiedClosed")
+        : Base(name)
         , _phase(Phase::Load)
     {
     }
@@ -340,13 +342,16 @@ public:
         return nullptr;
     }
 
-    /// The document is loaded.
-    bool onDocumentLoaded(const std::string& message) override
+    /// Wait for ModifiedStatus=false before closing.
+    /// This is sent right after loading.
+    bool onDocumentUnmodified(const std::string& message) override
     {
         LOG_TST("Got: [" << message << ']');
         LOK_ASSERT_STATE(_phase, Phase::WaitLoadStatus);
 
         TRANSITION_STATE(_phase, Phase::WaitDestroy);
+
+        LOG_TST("Closing document");
         WSD_CMD("closedocument");
 
         return true;
@@ -395,10 +400,41 @@ public:
     }
 };
 
+/// Test upload behavior with always_save_on_exit.
+/// The test verifies that an unmodified document
+/// is *not* uploaded when always_save_on_exit=true
+/// and is disconnected.
+class UnitSaveOnExitUnmodifiedDisconnect : public UnitSaveOnExitUnmodifiedClosed
+{
+public:
+    UnitSaveOnExitUnmodifiedDisconnect()
+        : UnitSaveOnExitUnmodifiedClosed("UnitSaveOnExitUnmodifiedDisconnect")
+    {
+    }
+
+    /// Wait for ModifiedStatus=false before disconnecting.
+    /// This is sent right after loading.
+    bool onDocumentUnmodified(const std::string& message) override
+    {
+        LOG_TST("Got: [" << message << ']');
+        LOK_ASSERT_STATE(_phase, Phase::WaitLoadStatus);
+
+        TRANSITION_STATE(_phase, Phase::WaitDestroy);
+
+        // Disconnect to trigger the auto-save logic.
+        LOG_TST("Disconnecting");
+        deleteSocketAt(0);
+
+        return true;
+    }
+};
+
 UnitBase** unit_create_wsd_multi(void)
 {
-    return new UnitBase* [4] {
-        new UnitWOPISaveOnExit(), new UnitSaveOnExitSaved(), new UnitSaveOnExitUnmodified(), nullptr
+    return new UnitBase* [5]
+    {
+        new UnitWOPISaveOnExit(), new UnitSaveOnExitSaved(), new UnitSaveOnExitUnmodifiedClosed(),
+            new UnitSaveOnExitUnmodifiedDisconnect(), nullptr
     };
 }
 

wsd/DocumentBroker.cpp

@@ -2725,7 +2725,12 @@ std::size_t DocumentBroker::removeSession(const std::shared_ptr<ClientSession>&
         const std::size_t activeSessionCount = countActiveSessions();
 
         const bool lastEditableSession = session->isEditable() && !haveAnotherEditableSession(id);
-        const bool dontSaveIfUnmodified = !_alwaysSaveOnExit;
+        // Forcing a save when always_save_on_exit=true creates a new
+        // file on disk, with a new timestamp, which makes it hard to
+        // avoid uploading when there really isn't any modifications.
+        // Instead, we rely on always issuing a save through forced
+        // auto-save and expect Core has the correct modified flag.
+        constexpr bool dontSaveIfUnmodified = true;
 
         LOG_INF("Removing session [" << id << "] on docKey [" << _docKey << "]. Have "
                                      << _sessions.size() << " sessions (" << activeSessionCount