Document, and simplify the two uses of ScMutationGuard.

Creates ScMutationDisable - used to ensure that no core data
structure is mutated below this guard in any thread. This can
also be used to disable access in the same thread now.

Change-Id: I7e4e98d8ff986490ccd5064b3b9af56acd877b49
Reviewed-on: https://gerrit.libreoffice.org/45119
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>

4 files changed, 66 insertions, 47 deletions

diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index c4db749eab6a..bb7bad7652b2 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -294,14 +294,11 @@ struct ScDocumentThreadSpecific
     void MergeBackIntoNonThreadedData(ScDocumentThreadSpecific& rNonThreadedData);
 };
 
-enum class ScMutationGuardFlags
+/// Enumeration to determine which pieces of the code should not be mutated when set.
+enum ScMutationGuardFlags
 {
     // Bit mask bits
-    CORE                = 0x0001,
-    FORMULA             = 0x0002,
-    RECURSIVE_INTERPRET = 0x0004,
-    // How many bits there are
-    N = 3
+    CORE = 0x0001, /// Core calc data structures should not be mutated
 };
 
 class ScDocument
@@ -330,7 +327,8 @@ friend class sc::ColumnSpanSet;
 friend class sc::EditTextIterator;
 friend class sc::FormulaGroupAreaListener;
 friend class sc::TableColumnBlockPositionSet;
-friend class ScMutationGuard;
+friend struct ScMutationGuard;
+friend struct ScMutationDisable;
 
     typedef std::vector<ScTable*> TableContainer;
 
@@ -536,7 +534,7 @@ private:
     bool                mbTrackFormulasPending  : 1;
     bool                mbFinalTrackFormulas    : 1;
 
-    std::recursive_mutex maMutationGuard[static_cast<std::size_t>(ScMutationGuardFlags::N)];
+    size_t              mnMutationGuardFlags;
 
 public:
     bool                     IsCellInChangeTrack(const ScAddress &cell,Color *pColCellBorder);
@@ -550,7 +548,7 @@ public:
                                                                 // number formatter
 public:
     SC_DLLPUBLIC                ScDocument( ScDocumentMode eMode = SCDOCMODE_DOCUMENT,
-                                SfxObjectShell* pDocShell = nullptr );
+                                            SfxObjectShell* pDocShell = nullptr );
     SC_DLLPUBLIC                ~ScDocument();
 
     void              SetName( const OUString& r ) { aDocName = r; }
@@ -2460,17 +2458,64 @@ private:
 
 typedef std::unique_ptr<ScDocument, o3tl::default_delete<ScDocument>> ScDocumentUniquePtr;
 
-class ScMutationGuard
+/**
+ * Instantiate this to ensure that subsequent modification of
+ * the document will cause an assertion failure while this is
+ * in-scope.
+ */
+struct ScMutationDisable
 {
-public:
-    ScMutationGuard(ScDocument* pDocument, ScMutationGuardFlags nFlags);
-    ~ScMutationGuard();
-
-private:
-    ScDocument* const mpDocument;
-    const ScMutationGuardFlags mnFlags;
+    ScMutationDisable(ScDocument* pDocument, ScMutationGuardFlags nFlags)
+    {
+#ifndef NDEBUG
+        mpDocument = pDocument;
+        mnFlagRestore = pDocument->mnMutationGuardFlags;
+        assert((mnFlagRestore & nFlags) == 0);
+        mpDocument->mnMutationGuardFlags |= static_cast<size_t>(nFlags);
+#else
+        (void)pDocument; (void)nFlags;
+#endif
+    }
+#ifndef NDEBUG
+    ~ScMutationDisable()
+    {
+        mpDocument->mnMutationGuardFlags = mnFlagRestore;
+    }
+    size_t mnFlagRestore;
+    ScDocument* mpDocument;
+#endif
 };
 
+/**
+ * A pretty assertion that checks that the relevant bits in
+ * the @nFlags are not set on the document at entry and exit.
+ *
+ * Its primary use is for debugging threading. As such, an
+ * @ScMutationDisable is created to forbid mutation, and this
+ * condition is then asserted on at prominent sites that
+ * mutate @nFlags.
+ */
+struct ScMutationGuard
+{
+    ScMutationGuard(ScDocument* pDocument, ScMutationGuardFlags nFlags)
+    {
+#ifndef NDEBUG
+        mpDocument = pDocument;
+        mnFlags = static_cast<size_t>(nFlags);
+        assert((mpDocument->mnMutationGuardFlags & mnFlags) == 0);
+#else
+        (void)pDocument; (void)nFlags;
+#endif
+    }
+#ifndef NDEBUG
+    ~ScMutationGuard()
+    {
+        assert((mpDocument->mnMutationGuardFlags & mnFlags) == 0);
+    }
+    size_t mnFlags;
+    ScDocument* mpDocument;
+#endif
+};
 
 #endif
 
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index 9bf0fcf73194..f2e0c7d35b67 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -221,7 +221,8 @@ ScDocument::ScDocument( ScDocumentMode eMode, SfxObjectShell* pDocShell ) :
         mnNamedRangesLockCount(0),
         mbUseEmbedFonts(false),
         mbTrackFormulasPending(false),
-        mbFinalTrackFormulas(false)
+        mbFinalTrackFormulas(false),
+        mnMutationGuardFlags(0)
 {
     SetStorageGrammar( formula::FormulaGrammar::GRAM_STORAGE_DEFAULT);
 
@@ -485,7 +486,7 @@ void ScDocument::InitClipPtrs( ScDocument* pSourceDoc )
     if ( pSourceValid )
         pValidationList = new ScValidationDataList(this, *pSourceValid);
 
-                        // store Links in Stream
+    // store Links in Stream
     delete pClipData;
     if (pSourceDoc->GetDocLinkManager().hasDdeLinks())
     {
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index 019bfcb99f4a..a585c0342774 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -6742,33 +6742,6 @@ void ScDocument::SetAutoNameCache(  ScAutoNameCache* pCache )
     pAutoNameCache = pCache;
 }
 
-ScMutationGuard::ScMutationGuard(ScDocument* pDocument, ScMutationGuardFlags nFlags) :
-    mpDocument(pDocument),
-    mnFlags(nFlags)
-{
-    (void) mpDocument;
-    for (unsigned b = 0; b < static_cast<std::size_t>(ScMutationGuardFlags::N); b++)
-    {
-        if (static_cast<std::size_t>(mnFlags) & (static_cast<std::size_t>(1) << b))
-        {
-            assert(mpDocument->maMutationGuard[b].try_lock());
-        }
-    }
-}
-
-ScMutationGuard::~ScMutationGuard()
-{
-#ifndef NDEBUG
-    for (unsigned b = 0; b < static_cast<std::size_t>(ScMutationGuardFlags::N); b++)
-    {
-        if (static_cast<std::size_t>(mnFlags) & (static_cast<std::size_t>(1) << b))
-        {
-            mpDocument->maMutationGuard[b].unlock();
-        }
-    }
-#endif
-}
-
 thread_local ScDocumentThreadSpecific ScDocument::maThreadSpecific;
 
 ScRecursionHelper& ScDocument::GetRecursionHelper()
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 3aaf87a1e629..79b51cc9d25c 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -4416,7 +4416,7 @@ bool ScFormulaCell::InterpretFormulaGroup()
             assert(!pDocument->mbThreadedGroupCalcInProgress);
             pDocument->mbThreadedGroupCalcInProgress = true;
 
-            ScMutationGuard aGuard(pDocument, ScMutationGuardFlags::CORE);
+            ScMutationDisable aGuard(pDocument, ScMutationGuardFlags::CORE);
 
             // Start nThreadCount new threads
             std::shared_ptr<comphelper::ThreadTaskTag> aTag = comphelper::ThreadPool::createThreadTaskTag();