wsd: move storage attributes to DocBroker

There are a number of races with having Storage
track the attributes. To fix them, we move all
attributes to DocBroker and correct a number
of issues.

The idea of the design is based on the fact that
we want to capture the attributes between
uploads, but based on the saved document.

That is, when we upload a document version, we
want to pass to the storage whether from the
perspective of the *Storage* there has been
any user-modifications or not. Since saving
to disk may happen multiple times between
uploads (not least because of failures), and
since saving resets the modified flag, we need
to capture the modified flag at each save and
propagate it until we upload successfully.
Upon uploading successfully, we reset the
attributes.

For this reason we have two attribute instances;
one is the 'current' attributes as being uploaded
and the other the 'next' one. We capture the
current state at saving into 'next' and we merge
with 'current' when saving succeeds and we
aren't already uploading (otherwise, we update
it and then discard it when uploading succeeds,
losing the last attributes).

Furthermore, because the modified flag is
reset after each save, and because we might
save and upload immediately after a
modification, we may not have the modified flag.
This means that we need some heuristics to
decide if there has been user-issued
modifications. (It is better to be conservative
here.) We try to detect this by introspecting
the commands we receive from users.

In effect, we capture the attributes when issuing
an internal save, we transfer the captured
attributes only when saving succeeds and we aren't
uploading, and from then on, uploading has to
succeed to reset the 'current' attributes. In the
meantime, if we fail to upload and issue another
save, the new attributes will be captured and
merged with the 'current' and the next upload
will not have any lost attributes.

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

4 files changed, 162 insertions, 86 deletions

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 8a57547fc3..ee38f33e1c 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -939,7 +939,8 @@ bool DocumentBroker::download(const std::shared_ptr<ClientSession>& session, con
         // Only lock the document on storage for editing sessions
         // FIXME: why not lock before downloadStorageFileToLocal? Would also prevent race conditions
         if (!session->isReadOnly() &&
-            !_storage->updateLockState(session->getAuthorization(), *_lockCtx, true))
+            !_storage->updateLockState(session->getAuthorization(), *_lockCtx, true,
+                                       _currentStorageAttrs))
         {
             LOG_ERR("Failed to lock!");
             session->setLockFailed(_lockCtx->_lockFailureReason);
@@ -1146,7 +1147,8 @@ void DocumentBroker::endRenameFileCommand()
 
 bool DocumentBroker::attemptLock(const ClientSession& session, std::string& failReason)
 {
-    const bool bResult = _storage->updateLockState(session.getAuthorization(), *_lockCtx, true);
+    const bool bResult = _storage->updateLockState(session.getAuthorization(), *_lockCtx, true,
+                                                   _currentStorageAttrs);
     if (!bResult)
         failReason = _lockCtx->_lockFailureReason;
     return bResult;
@@ -1273,6 +1275,16 @@ void DocumentBroker::handleSaveResponse(const std::string& sessionId, bool succe
     // Record that we got a response to avoid timing out on saving.
     _saveManager.setLastSaveResult(success || result == "unmodified");
 
+    if (success && !isAsyncUploading())
+    {
+        // Update the storage attributes to capture what's
+        // new and applies to this new version and reset.
+        // These are the attributes of the next version to be uploaded.
+        // Note: these are owned by us and this is thread-safe.
+        _currentStorageAttrs.merge(_nextStorageAttrs);
+        _nextStorageAttrs.reset();
+    }
+
     // The the clients know of any save failures.
     if (!success && result != "unmodified")
     {
@@ -1367,9 +1379,11 @@ void DocumentBroker::uploadToStorage(const std::string& sessionId, bool force)
 {
     assertCorrectThread();
 
-    if (force && _storage)
+    if (force)
     {
-        _storage->forceSave();
+        // Don't reset the force flag if it was set
+        // (which would imply we failed to upload).
+        _currentStorageAttrs.setForced(force);
     }
 
     if (force || _storageManager.lastUploadSuccessful() || _storageManager.canUploadNow())
@@ -1535,7 +1549,8 @@ void DocumentBroker::uploadToStorageInternal(const std::string& sessionId,
     };
 
     _storage->uploadLocalFileToStorageAsync(session->getAuthorization(), *_lockCtx, saveAsPath,
-                                            saveAsFilename, isRename, *_poll, asyncUploadCallback);
+                                            saveAsFilename, isRename, _currentStorageAttrs, *_poll,
+                                            asyncUploadCallback);
 }
 
 void DocumentBroker::handleUploadToStorageResponse(const StorageBase::UploadResult& uploadResult)
@@ -1586,6 +1601,9 @@ void DocumentBroker::handleUploadToStorageResponse(const StorageBase::UploadResu
             // After a successful save, we are sure that document in the storage is same as ours
             _documentChangedInStorage = false;
 
+            // Reset the storage attributes; They've been used and we can discard them.
+            _currentStorageAttrs.reset();
+
             LOG_DBG("Uploaded docKey ["
                     << _docKey << "] to URI [" << _uploadRequest->uriAnonym()
                     << "] and updated timestamps. Document modified timestamp: "
@@ -1872,7 +1890,8 @@ void DocumentBroker::refreshLock()
     else
     {
         std::shared_ptr<ClientSession> session = it->second;
-        if (!session || !_storage->updateLockState(session->getAuthorization(), *_lockCtx, true))
+        if (!session || !_storage->updateLockState(session->getAuthorization(), *_lockCtx, true,
+                                                   _currentStorageAttrs))
             LOG_ERR("Failed to refresh lock");
     }
 }
@@ -2160,15 +2179,13 @@ bool DocumentBroker::sendUnoSave(const std::string& sessionId, bool dontTerminat
         // arguments end
         oss << '}';
 
-        if (_storage)
-        {
-            // If we can't upload, we will quarantine.
-            //FIXME: these must be tracked in DocBroker as they will
-            // get clobbered when uploading fails and we save again.
-            _storage->setIsAutosave(isAutosave || UnitWSD::get().isAutosave());
-            _storage->setIsExitSave(isExitSave);
-            _storage->setExtendedData(extendedData);
-        }
+        // At this point, if we have any potential modifications, we need to capture the fact.
+        _nextStorageAttrs.setUserModified(isModified() || haveModifyActivityAfterSaveRequest());
+
+        //FIXME: It's odd to capture these here, but this function is used from ClientSession too.
+        _nextStorageAttrs.setIsAutosave(isAutosave || UnitWSD::get().isAutosave());
+        _nextStorageAttrs.setIsExitSave(isExitSave);
+        _nextStorageAttrs.setExtendedData(extendedData);
 
         const std::string saveArgs = oss.str();
         LOG_TRC(".uno:Save arguments: " << saveArgs);
@@ -2405,7 +2422,8 @@ void DocumentBroker::disconnectSessionInternal(const std::string& id)
             // Unlock the document, if last editable sessions, before we lose a token that can unlock.
             if (lastEditableSession && _lockCtx->_isLocked && _storage)
             {
-                if (!_storage->updateLockState(it->second->getAuthorization(), *_lockCtx, false))
+                if (!_storage->updateLockState(it->second->getAuthorization(), *_lockCtx, false,
+                                               _currentStorageAttrs))
                     LOG_ERR("Failed to unlock!");
             }
 
@@ -3130,13 +3148,6 @@ void DocumentBroker::setModified(const bool value)
     }
 #endif
 
-    if (value || _storageManager.lastUploadSuccessful())
-    {
-        // Set the X-COOL-WOPI-IsModifiedByUser header flag sent to storage.
-        // But retain the last value if we failed to upload, so we don't clobber it.
-        _storage->setUserModified(value);
-    }
-
     LOG_DBG("Modified state set to " << value << " for Doc [" << _docId << ']');
     _isModified = value;
 }
@@ -3671,6 +3682,11 @@ void DocumentBroker::dumpState(std::ostream& os)
     os << "\n  StorageManager:";
     _storageManager.dumpState(os, "\n    ");
 
+    os << "\n    Current StorageAttributes:";
+    _currentStorageAttrs.dumpState(os, "\n      ");
+    os << "\n    Next StorageAttributes:";
+    _nextStorageAttrs.dumpState(os, "\n      ");
+
     _lockCtx->dumpState(os);
 
     if (_tileCache)
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 3014d233c4..eb13fd01f5 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -1351,6 +1351,15 @@ private:
     std::set<std::string> _isInitialStateSet;
 
     std::unique_ptr<StorageBase> _storage;
+
+    /// The current upload request's attributes. Re-used to retry after failure.
+    /// Updated right before uploading.
+    StorageBase::Attributes _currentStorageAttrs;
+    /// The next upload request's attributes. Avoids clobbering
+    /// _currentStorageAttrs until the current request succeeds.
+    /// Updated right before saving.
+    StorageBase::Attributes _nextStorageAttrs;
+
     std::unique_ptr<TileCache> _tileCache;
     std::atomic<bool> _isModified;
     int _cursorPosX;
diff --git a/wsd/Storage.cpp b/wsd/Storage.cpp
index d1a1035ab7..235ea7edd4 100644
--- a/wsd/Storage.cpp
+++ b/wsd/Storage.cpp
@@ -395,7 +395,7 @@ void LocalStorage::uploadLocalFileToStorageAsync(const Authorization& /*auth*/,
                                                  LockContext& /*lockCtx*/,
                                                  const std::string& /*saveAsPath*/,
                                                  const std::string& /*saveAsFilename*/,
-                                                 bool /*isRename*/, SocketPoll&,
+                                                 bool /*isRename*/, const Attributes&, SocketPoll&,
                                                  const AsyncUploadCallback& asyncUploadCallback)
 {
     const std::string path = getUri().getPath();
@@ -921,7 +921,8 @@ WopiStorage::WOPIFileInfo::WOPIFileInfo(const FileInfo &fileInfo,
         _disableExport = true;
 }
 
-bool WopiStorage::updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock)
+bool WopiStorage::updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock,
+                                  const Attributes& attribs)
 {
     lockCtx._lockFailureReason.clear();
     if (!lockCtx._supportsLocks)
@@ -948,10 +949,10 @@ bool WopiStorage::updateLockState(const Authorization& auth, LockContext& lockCt
 
         request.set("X-WOPI-Override", lock ? "LOCK" : "UNLOCK");
         request.set("X-WOPI-Lock", lockCtx._lockToken);
-        if (!getExtendedData().empty())
+        if (!attribs.getExtendedData().empty())
         {
-            request.set("X-COOL-WOPI-ExtendedData", getExtendedData());
-            request.set("X-LOOL-WOPI-ExtendedData", getExtendedData());
+            request.set("X-COOL-WOPI-ExtendedData", attribs.getExtendedData());
+            request.set("X-LOOL-WOPI-ExtendedData", attribs.getExtendedData());
         }
 
         // IIS requires content-length for POST requests: see https://forums.iis.net/t/1119456.aspx
@@ -1185,7 +1186,8 @@ private:
 void WopiStorage::uploadLocalFileToStorageAsync(const Authorization& auth, LockContext& lockCtx,
                                                 const std::string& saveAsPath,
                                                 const std::string& saveAsFilename,
-                                                const bool isRename, SocketPoll& socketPoll,
+                                                const bool isRename, const Attributes& attribs,
+                                                SocketPoll& socketPoll,
                                                 const AsyncUploadCallback& asyncUploadCallback)
 {
     ProfileZone profileZone("WopiStorage::uploadLocalFileToStorage", { {"url", _fileUrl} });
@@ -1247,21 +1249,21 @@ void WopiStorage::uploadLocalFileToStorageAsync(const Authorization& auth, LockC
         {
             // normal save
             httpHeader.set("X-WOPI-Override", "PUT");
-            httpHeader.set("X-COOL-WOPI-IsModifiedByUser", isUserModified() ? "true" : "false");
-            httpHeader.set("X-LOOL-WOPI-IsModifiedByUser", isUserModified() ? "true" : "false");
-            httpHeader.set("X-COOL-WOPI-IsAutosave", isAutosave() ? "true" : "false");
-            httpHeader.set("X-LOOL-WOPI-IsAutosave", isAutosave() ? "true" : "false");
-            httpHeader.set("X-COOL-WOPI-IsExitSave", isExitSave() ? "true" : "false");
-            httpHeader.set("X-LOOL-WOPI-IsExitSave", isExitSave() ? "true" : "false");
-            if (isExitSave())
+            httpHeader.set("X-COOL-WOPI-IsModifiedByUser", attribs.isUserModified() ? "true" : "false");
+            httpHeader.set("X-LOOL-WOPI-IsModifiedByUser", attribs.isUserModified() ? "true" : "false");
+            httpHeader.set("X-COOL-WOPI-IsAutosave", attribs.isAutosave() ? "true" : "false");
+            httpHeader.set("X-LOOL-WOPI-IsAutosave", attribs.isAutosave() ? "true" : "false");
+            httpHeader.set("X-COOL-WOPI-IsExitSave", attribs.isExitSave() ? "true" : "false");
+            httpHeader.set("X-LOOL-WOPI-IsExitSave", attribs.isExitSave() ? "true" : "false");
+            if (attribs.isExitSave())
                 httpHeader.set("Connection", "close"); // Don't maintain the socket if we are exiting.
-            if (!getExtendedData().empty())
+            if (!attribs.getExtendedData().empty())
             {
-                httpHeader.set("X-COOL-WOPI-ExtendedData", getExtendedData());
-                httpHeader.set("X-LOOL-WOPI-ExtendedData", getExtendedData());
+                httpHeader.set("X-COOL-WOPI-ExtendedData", attribs.getExtendedData());
+                httpHeader.set("X-LOOL-WOPI-ExtendedData", attribs.getExtendedData());
             }
 
-            if (!getForceSave())
+            if (!attribs.isForced())
             {
                 // Request WOPI host to not overwrite if timestamps mismatch
                 httpHeader.set("X-COOL-WOPI-Timestamp", getFileInfo().getLastModifiedTime());
@@ -1450,9 +1452,6 @@ WopiStorage::handleUploadToStorageResponse(const WopiUploadDetails& details,
 
                     result.setSaveAsResult(name, url);
                 }
-                // Reset the force save flag now, if any, since we are done saving
-                // Next saves shouldn't be saved forcefully unless commanded
-                forceSave(false);
             }
             else
             {
diff --git a/wsd/Storage.hpp b/wsd/Storage.hpp
index 0ece812122..76fa398d91 100644
--- a/wsd/Storage.hpp
+++ b/wsd/Storage.hpp
@@ -101,6 +101,92 @@ public:
         std::string _modifiedTime; //< Opaque modified timestamp as received from the server.
     };
 
+    /// Represents attributes of interest to the storage.
+    /// These are typically set in the PUT headers.
+    /// They include flags to indicate auto-save, exit-save,
+    /// forced-uploading, and whether or not the document
+    /// had been modified, amongst others.
+    /// The reason for this class is to avoid clobbering
+    /// these attributes when uploading fails--or indeed
+    /// racing with uploading.
+    class Attributes
+    {
+    public:
+        Attributes()
+            : _forced(false)
+            , _isUserModified(false)
+            , _isAutosave(false)
+            , _isExitSave(false)
+        {}
+
+        /// Reset the attributes to clear them after using them.
+        void reset()
+        {
+            _forced = false;
+            _isUserModified = false;
+            _isAutosave = false;
+            _isExitSave = false;
+            _extendedData.clear();
+        }
+
+        void merge(const Attributes& lhs)
+        {
+            // Whichever is true.
+            _forced = lhs._forced ? true : _forced;
+            _isUserModified = lhs._isUserModified ? true : _isUserModified;
+            _isAutosave = lhs._isAutosave ? true : _isAutosave;
+            _isExitSave = lhs._isExitSave ? true : _isExitSave;
+
+            // Clobber with the lhs, assuming it's newer.
+            if (!lhs._extendedData.empty())
+                _extendedData = lhs._extendedData;
+        }
+
+        /// Asks the storage object to force overwrite
+        /// even if document turned out to be changed in storage.
+        /// Used to resolve storage conflicts by clobbering.
+        void setForced(bool forced = true) { _forced = forced; }
+        bool isForced() const { return _forced; }
+
+        /// To be able to set the WOPI extension header appropriately.
+        void setUserModified(bool userModified) { _isUserModified = userModified; }
+        bool isUserModified() const { return _isUserModified; }
+
+        /// To be able to set the WOPI 'is autosave/is exitsave?' headers appropriately.
+        void setIsAutosave(bool newIsAutosave) { _isAutosave = newIsAutosave; }
+        bool isAutosave() const { return _isAutosave; }
+
+        /// Set only when saving on exit.
+        void setIsExitSave(bool exitSave) { _isExitSave = exitSave; }
+        bool isExitSave() const { return _isExitSave; }
+
+        /// Misc extended data.
+        void setExtendedData(const std::string& extendedData) { _extendedData = extendedData; }
+        const std::string& getExtendedData() const { return _extendedData; }
+
+        /// Dump the internals of this instance.
+        void dumpState(std::ostream& os, const std::string& indent = "\n  ")
+        {
+            os << indent << "forced: " << std::boolalpha << isForced();
+            os << indent << "user-modified: " << std::boolalpha << isUserModified();
+            os << indent << "auto-save: " << std::boolalpha << isAutosave();
+            os << indent << "exit-save: " << std::boolalpha << isExitSave();
+            os << indent << "extended-data: " << getExtendedData();
+        }
+
+    private:
+        /// Whether or not we want to force uploading.
+        bool _forced;
+        /// The document has been modified by the user.
+        bool _isUserModified;
+        /// This save operation is an autosave.
+        bool _isAutosave;
+        /// Saving on exit (when the document is cleaned up from memory)
+        bool _isExitSave;
+        /// The client-provided saving extended data to send to the WOPI host.
+        std::string _extendedData;
+    };
+
     /// Represents the upload request result, with a Result code
     /// and a reason message (typically for errors).
     /// Note: the reason message may be displayed to the clients.
@@ -196,11 +282,7 @@ public:
         _localStorePath(localStorePath),
         _jailPath(jailPath),
         _fileInfo(std::string(), "cool", std::string()),
-        _isDownloaded(false),
-        _forceSave(false),
-        _isUserModified(false),
-        _isAutosave(false),
-        _isExitSave(false)
+        _isDownloaded(false)
     {
         setUri(uri);
         LOG_DBG("Storage ctor: " << COOLWSD::anonymizeUrl(_uri.toString()));
@@ -243,24 +325,6 @@ public:
 
     bool isDownloaded() const { return _isDownloaded; }
 
-    /// Asks the storage object to force overwrite to storage upon next save
-    /// even if document turned out to be changed in storage
-    void forceSave(bool newSave = true) { _forceSave = newSave; }
-
-    bool getForceSave() const { return _forceSave; }
-
-    /// To be able to set the WOPI extension header appropriately.
-    void setUserModified(bool userModified) { _isUserModified = userModified; }
-
-    bool isUserModified() const { return _isUserModified; }
-
-    /// To be able to set the WOPI 'is autosave/is exitsave?' headers appropriately.
-    void setIsAutosave(bool newIsAutosave) { _isAutosave = newIsAutosave; }
-    bool isAutosave() const { return _isAutosave; }
-    void setIsExitSave(bool exitSave) { _isExitSave = exitSave; }
-    bool isExitSave() const { return _isExitSave; }
-    void setExtendedData(const std::string& extendedData) { _extendedData = extendedData; }
-
     void setFileInfo(const FileInfo& fileInfo) { _fileInfo = fileInfo; }
 
     /// Returns the basic information about the file.
@@ -270,7 +334,8 @@ public:
     std::string getFileExtension() const { return Poco::Path(_fileInfo.getFilename()).getExtension(); }
 
     /// Update the locking state (check-in/out) of the associated file
-    virtual bool updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock) = 0;
+    virtual bool updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock,
+                                 const Attributes& attribs) = 0;
 
     /// Returns a local file path for the given URI.
     /// If necessary copies the file locally first.
@@ -286,7 +351,7 @@ public:
     virtual void uploadLocalFileToStorageAsync(const Authorization& auth, LockContext& lockCtx,
                                                const std::string& saveAsPath,
                                                const std::string& saveAsFilename,
-                                               const bool isRename, SocketPoll&,
+                                               const bool isRename, const Attributes&, SocketPoll&,
                                                const AsyncUploadCallback& asyncUploadCallback) = 0;
 
     /// Get the progress state of an asynchronous LocalFileToStorage upload.
@@ -343,9 +408,6 @@ protected:
     /// Returns the root path of the jail directory of docs.
     std::string getLocalRootPath() const;
 
-    /// Returns the client-provided extended data to send to the WOPI host.
-    const std::string& getExtendedData() const { return _extendedData; }
-
 private:
     Poco::URI _uri;
     const std::string _localStorePath;
@@ -354,17 +416,6 @@ private:
     std::string _jailedFilePathAnonym;
     FileInfo _fileInfo;
     bool _isDownloaded;
-    bool _forceSave;
-
-    /// The document has been modified by the user.
-    bool _isUserModified;
-
-    /// This save operation is an autosave.
-    bool _isAutosave;
-    /// Saving on exit (when the document is cleaned up from memory)
-    bool _isExitSave;
-    /// The client-provided saving extended data to send to the WOPI host.
-    std::string _extendedData;
 
     static bool FilesystemEnabled;
     /// If true, use only the WOPI URL for whether to use SSL to talk to storage server
@@ -410,7 +461,7 @@ public:
     /// obtained using getFileInfo method
     std::unique_ptr<LocalFileInfo> getLocalFileInfo();
 
-    bool updateLockState(const Authorization&, LockContext&, bool) override
+    bool updateLockState(const Authorization&, LockContext&, bool, const Attributes&) override
     {
         return true;
     }
@@ -421,7 +472,7 @@ public:
     void uploadLocalFileToStorageAsync(const Authorization& auth, LockContext& lockCtx,
                                        const std::string& saveAsPath,
                                        const std::string& saveAsFilename, const bool isRename,
-                                       SocketPoll&,
+                                       const Attributes&, SocketPoll&,
                                        const AsyncUploadCallback& asyncUploadCallback) override;
 
 private:
@@ -572,7 +623,8 @@ public:
                                                         unsigned redirectLimit);
 
     /// Update the locking state (check-in/out) of the associated file
-    bool updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock) override;
+    bool updateLockState(const Authorization& auth, LockContext& lockCtx, bool lock,
+                         const Attributes& attribs) override;
 
     /// uri format: http://server/<...>/wopi*/files/<id>/content
     std::string downloadStorageFileToLocal(const Authorization& auth, LockContext& lockCtx,
@@ -581,7 +633,7 @@ public:
     void uploadLocalFileToStorageAsync(const Authorization& auth, LockContext& lockCtx,
                                        const std::string& saveAsPath,
                                        const std::string& saveAsFilename, const bool isRename,
-                                       SocketPoll& socketPoll,
+                                       const Attributes&, SocketPoll& socketPoll,
                                        const AsyncUploadCallback& asyncUploadCallback) override;
 
     /// Total time taken for making WOPI calls during uploading.