tdf#96607 'Save as' doesn't update global auto-recovery state

The auto-recovery service maintains a list of structures (one for each open
document) containing information needed to carry out the auto-save
functionality. One such piece of information is the location of the backup
file, stored in a struct member named 'OldTempURL'.  At every auto-save
interval, this list is iterated through and a function (implts_saveOneDoc)
is called during each iteration to save the current state of the associated
document.

The algorithm works as follows:
 1. A new backup file URL is chosen so as not to conflict with any already
    existing backup files in the backup directory.  This URL is based on the
    file name and incorporates a number (starting at 0) that is incremented
    until a name is chosen that doesn't conflict.

 2. The document is saved to this new backup file URL

 3. The previous backup file (indicated by its structure's 'OldTempURL') is
    deleted

 4. The new backup file URL is stored (in its structure's 'OldTempURL') for the
    next time the file needs to be saved.

Assuming you start with a new Writer doc and then make some changes, when it is
time to auto-save, the backup file name 'untitled_0.odt' (excluding path) will
be selected, the latest state of the open file will be written to that backup
file, and the full URL for the backup file will be saved into the struct
'OldTempURL' member.

The next time changes are made and an auto-save occurs, this algorithm will
result in the name 'untitled_1.odt' being selected, the file contents saved
into this new file, 'untitled_0.odt' being deleted, and the full URL for the
new backup file being saved in 'OldTempURL'.

The third time through results in 'untitled_0.odt' being selected (since this
file doesn't exist on disk), and subsequent iterations of auto-saving cause
the backup file name to alternate between the two aforementioned.

The problem occurs during a 'Save as' operation. When this happens, the backup
file is deleted (which is fine - it was just saved, and the next auto-save will
back it up) but 'OldTempURL' is not properly reset (see below for more info.)
During the next auto-save, 'untitled_0.odt' will be selected for the new backup
file name (since no file exists by this name), and one of two things will
happen (based on how many auto-saves have occurred):

 1. 'OldTempURL' points to 'untitled_1.odt', and the algorithm above continues
    to work correctly (at least in that it continues to backup file contents.)

 2. 'OldTempURL' points to 'untitled_0.odt', the name chosen for the new backup
    file.  In this case, the document contents will be saved to this file
    (step 2) but then the file will be deleted (step 3).  'OldTempURL' will
    maintain this URL from then on out, causing this case to be hit for all
    future auto-save intervals.

So, 50% of the time (30 minutes out of every hour) auto-save will stop backing
up file contents on a 'Save as'.

The function that handles the 'Save as' case (implts_markDocumentAsSaved)
clears 'OldTempURL' and sets other relavent struct members for a local variable
copy of the global struct, but doesn't copy them back. :(  These changes are
effectively lost when the function returns.

There are several other cases where this appears to be happening as well, but
more work is needed to determine whether this is actually the case:
 - implts_prepareSessionShutdown
 - implts_saveDocs, handling the 'dangerousDocs' and in a few other places
 - implts_openDocs
 - implts_resetHandleStates

Also, there is some JUnitTest code for auto-save, but it is currently disabled
(and fails to run successfully.) It'd be great to get these working again, or
to just write python equivalents. Implementing this would like take me a while,
though, so for now I just tested manually to ensure that this fixes the issue.

When I have some more time I'd like to work more on this, but I wanted to send
this patch in for now to address bug #96607.

This may also address bug #99890, since some of the struct members that don't
make it into the global state relate to the file name.  I haven't explicitly
tested this case, though.

Change-Id: Ic702d6f78e60c7cf828a1564ccca118dd45d152b
Reviewed-on: https://gerrit.libreoffice.org/25948
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: jan iversen <jani@documentfoundation.org>

1 files changed, 13 insertions, 0 deletions

diff --git a/framework/source/services/autorecovery.cxx b/framework/source/services/autorecovery.cxx
index 0a1655be7e6a..cf97b6d981dd 100644
--- a/framework/source/services/autorecovery.cxx
+++ b/framework/source/services/autorecovery.cxx
@@ -2667,11 +2667,22 @@ void AutoRecovery::implts_markDocumentAsSaved(const css::uno::Reference< css::fr
         return;
     aInfo = *pIt;
 
+    /* Since the document has been saved, update its entry in the document
+     * cache. We essentially reset the state of the document from an
+     * autorecovery perspective, updating things like the filename (which
+     * would change in the case of a 'Save as' operation) and the associated
+     * backup file URL.  */
+
     aInfo.DocumentState = AutoRecovery::E_UNKNOWN;
     // TODO replace getLocation() with getURL() ... it's a workaround currently only!
     css::uno::Reference< css::frame::XStorable > xDoc(aInfo.Document, css::uno::UNO_QUERY);
     aInfo.OrgURL = xDoc->getLocation();
 
+    /* Save off the backup file URLs and then clear them. NOTE - it is
+     * important that we clear them - otherwise, we could enter a state
+     * where pIt->OldTempURL == pIt->NewTempURL and our backup algorithm
+     * in implts_saveOneDoc will write to that URL and then delete the file
+     * at that URL (bug #96607) */
     sRemoveURL1 = aInfo.OldTempURL;
     sRemoveURL2 = aInfo.NewTempURL;
     aInfo.OldTempURL.clear();
@@ -2692,6 +2703,8 @@ void AutoRecovery::implts_markDocumentAsSaved(const css::uno::Reference< css::fr
 
     aInfo.UsedForSaving = false;
 
+    *pIt = aInfo;
+
     } /* SAFE */
 
     implts_flushConfigItem(aInfo);