assert on duplicate listener in SfxListener

To enable finding the source of the duplicate calls, I add new SAL
API (only for internal use) to retrieve and symbolise stack
backtraces.
The theory is that it relatively cheap to just store a backtrace,
but quite expense to symbolise it to strings. Note that the
backtrace() library we use on Linux does not do a particularly
good job, but it gives enough information that developers can use
the addr2line tool to get more precise info.

Explanation of fixes in the code that triggered the assert:

In SwFrameHolder, we need to only call StartListening() if the
pFrame member is actually changing. We also need to call
EndListening() on the old values when pFrame changes.

In SwNavigationPI, there is already a StartListening() call in
the only place we assign to m_pCreateView.

In ImpEditEngine, we need to ignore duplicates, because it is
doing a ref-counting thing. By storing duplicates on the listener
list, it doesn't need to keep track of which stylesheets its
child nodes are using. Given that it therefore will see
duplicate events, there is probably some performance optimisation
opportunities here.

In MasterPageObserver::Implementation::RegisterDocument, we
seem to be getting called multiple times with the same
SdDrawDocument, so just check if we've been registered already
before calling StartListening()

In SvxShape::impl_initFromSdrObject, do the same thing we do
elsewhere in this class, i.e. only call StartListening()
if the model has changed.

Change-Id: I7eae5c774e1e8d56f0ad7bec80e4df0017b287ac
Reviewed-on: https://gerrit.libreoffice.org/41045
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

1 files changed, 39 insertions, 8 deletions

diff --git a/svl/source/notify/lstner.cxx b/svl/source/notify/lstner.cxx
index 4279e12f4a5c..0b3d2e9a3af5 100644
--- a/svl/source/notify/lstner.cxx
+++ b/svl/source/notify/lstner.cxx
@@ -21,17 +21,22 @@
 
 #include <svl/hint.hxx>
 #include <svl/SfxBroadcaster.hxx>
+#include <sal/backtrace.hxx>
 
 #include <algorithm>
 #include <cassert>
 #include <deque>
-
+#include <memory>
+#include <map>
 
 typedef std::deque<SfxBroadcaster*> SfxBroadcasterArr_Impl;
 
 struct SfxListener::Impl
 {
     SfxBroadcasterArr_Impl maBCs;
+#ifdef DBG_UTIL
+    std::map<SfxBroadcaster*, std::unique_ptr<BacktraceState>> maCallStacks;
+#endif
 };
 
 // simple ctor of class SfxListener
@@ -68,27 +73,47 @@ void SfxListener::RemoveBroadcaster_Impl( SfxBroadcaster& rBroadcaster )
     auto it = std::find( mpImpl->maBCs.begin(), mpImpl->maBCs.end(), &rBroadcaster );
     if (it != mpImpl->maBCs.end()) {
         mpImpl->maBCs.erase( it );
+#ifdef DBG_UTIL
+        mpImpl->maCallStacks.erase( &rBroadcaster );
+#endif
     }
 }
 
 
-// registers a specific SfxBroadcaster
 
-void SfxListener::StartListening( SfxBroadcaster& rBroadcaster, bool bPreventDups )
+/**
+ Registers a specific SfxBroadcaster.
+
+ Some code uses duplicates as a kind of ref-counting thing i.e. they add and remove listeners
+ on different code paths, and they only really stop listening when the last EndListening() is called.
+*/
+void SfxListener::StartListening( SfxBroadcaster& rBroadcaster, bool bPreventDuplicates )
 {
-    if ( !bPreventDups || !IsListening( rBroadcaster ) )
+    bool bListeningAlready = IsListening( rBroadcaster );
+
+#ifdef DBG_UTIL
+    if (bListeningAlready && !bPreventDuplicates)
+    {
+        auto f = mpImpl->maCallStacks.find( &rBroadcaster );
+        SAL_INFO("svl", "previous StartListening call came from: " << sal_backtrace_to_string(f->second.get()));
+    }
+#endif
+    assert(!(bListeningAlready && !bPreventDuplicates) && "duplicate listener, try building with DBG_UTIL to find the other insert site.");
+
+    if ( !bListeningAlready || !bPreventDuplicates )
     {
         rBroadcaster.AddListener(*this);
         mpImpl->maBCs.push_back( &rBroadcaster );
-
+#ifdef DBG_UTIL
+        mpImpl->maCallStacks.emplace( &rBroadcaster, sal_backtrace_get(10) );
+#endif
         assert(IsListening(rBroadcaster) && "StartListening failed");
     }
 }
 
-
 // unregisters a specific SfxBroadcaster
 
-void SfxListener::EndListening( SfxBroadcaster& rBroadcaster, bool bAllDups )
+void SfxListener::EndListening( SfxBroadcaster& rBroadcaster, bool bRemoveAllDuplicates )
 {
     SfxBroadcasterArr_Impl::iterator beginIt = mpImpl->maBCs.begin();
     do
@@ -100,8 +125,11 @@ void SfxListener::EndListening( SfxBroadcaster& rBroadcaster, bool bAllDups )
         }
         rBroadcaster.RemoveListener(*this);
         beginIt = mpImpl->maBCs.erase( it );
+#ifdef DBG_UTIL
+        mpImpl->maCallStacks.erase( &rBroadcaster );
+#endif
     }
-    while ( bAllDups );
+    while ( bRemoveAllDuplicates );
 }
 
 
@@ -116,6 +144,9 @@ void SfxListener::EndListeningAll()
         pBC->RemoveListener(*this);
         mpImpl->maBCs.pop_front();
     }
+#ifdef DBG_UTIL
+    mpImpl->maCallStacks.clear();
+#endif
 }