[Libreoffice-commits] core.git: editeng/source include/sal include/svl sal/osl sal/util sd/source svl/source svx/source sw/source
Noel Grandin
noel.grandin at collabora.co.uk
Wed Aug 16 08:43:26 UTC 2017
editeng/source/editeng/impedit5.cxx | 2 -
include/sal/backtrace.hxx | 44 ++++++++++++++++++++++++
include/svl/lstner.hxx | 4 +-
sal/osl/unx/backtraceapi.cxx | 30 ++++++++++++++++
sal/osl/w32/backtrace.cxx | 51 ++++++++++++++++++++++++++++
sal/util/sal.map | 6 +++
sd/source/ui/sidebar/MasterPageObserver.cxx | 4 +-
svl/source/notify/lstner.cxx | 47 +++++++++++++++++++++----
svx/source/unodraw/unoshape.cxx | 15 +++++---
sw/source/core/layout/frmtool.cxx | 9 +++-
sw/source/uibase/utlui/navipi.cxx | 3 -
11 files changed, 194 insertions(+), 21 deletions(-)
New commits:
commit bc9a2ba677ce3fcd46c2bbef6e8faeacb14292c1
Author: Noel Grandin <noel.grandin at collabora.co.uk>
Date: Thu Aug 10 10:39:59 2017 +0200
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 at libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
diff --git a/editeng/source/editeng/impedit5.cxx b/editeng/source/editeng/impedit5.cxx
index 5ec8e65e32d6..5ba3549c9360 100644
--- a/editeng/source/editeng/impedit5.cxx
+++ b/editeng/source/editeng/impedit5.cxx
@@ -92,7 +92,7 @@ void ImpEditEngine::SetStyleSheet( sal_Int32 nPara, SfxStyleSheet* pStyle )
EndListening( *pCurStyle );
pNode->SetStyleSheet( pStyle, aStatus.UseCharAttribs() );
if ( pStyle )
- StartListening( *pStyle );
+ StartListening( *pStyle, true );
ParaAttribsChanged( pNode );
}
FormatAndUpdate();
diff --git a/include/sal/backtrace.hxx b/include/sal/backtrace.hxx
new file mode 100644
index 000000000000..2c854e102eef
--- /dev/null
+++ b/include/sal/backtrace.hxx
@@ -0,0 +1,44 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#ifndef INCLUDED_SAL_BACKTRACE_HXX
+#define INCLUDED_SAL_BACKTRACE_HXX
+
+#include <sal/config.h>
+#include <sal/saldllapi.h>
+#include <sal/types.h>
+#include <rtl/ustring.hxx>
+#include <memory>
+
+/// @cond INTERNAL
+/**
+ Two stage API for recording and then later decoding stack backtraces.
+ Useful for debugging facilities where we are only interested in decoding
+ a small handful of recorded stack traces.
+
+ @param backtraceDepth value indicating the maximum backtrace depth; must be > 0
+*/
+#if defined LIBO_INTERNAL_ONLY
+
+struct BacktraceState {
+ void** buffer;
+ int nDepth;
+};
+
+SAL_DLLPUBLIC std::unique_ptr<BacktraceState> SAL_CALL sal_backtrace_get(
+ sal_uInt32 backtraceDepth);
+
+SAL_DLLPUBLIC OUString SAL_CALL sal_backtrace_to_string(
+ BacktraceState* backtraceState);
+
+#endif
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/include/svl/lstner.hxx b/include/svl/lstner.hxx
index 527a13baa834..ad79698728cb 100644
--- a/include/svl/lstner.hxx
+++ b/include/svl/lstner.hxx
@@ -39,8 +39,8 @@ public:
SfxListener( const SfxListener &rCopy );
virtual ~SfxListener() COVERITY_NOEXCEPT_FALSE;
- void StartListening( SfxBroadcaster& rBroadcaster, bool bPreventDups = false );
- void EndListening( SfxBroadcaster& rBroadcaster, bool bAllDups = false );
+ void StartListening( SfxBroadcaster& rBroadcaster, bool bPreventDuplicates = false );
+ void EndListening( SfxBroadcaster& rBroadcaster, bool bRemoveAllDuplicates = false );
void EndListeningAll();
bool IsListening( SfxBroadcaster& rBroadcaster ) const;
diff --git a/sal/osl/unx/backtraceapi.cxx b/sal/osl/unx/backtraceapi.cxx
index 91be8d5d40c4..4838474be36c 100644
--- a/sal/osl/unx/backtraceapi.cxx
+++ b/sal/osl/unx/backtraceapi.cxx
@@ -18,6 +18,7 @@
#include <rtl/ustrbuf.hxx>
#include <rtl/ustring.hxx>
#include <sal/types.h>
+#include <sal/backtrace.hxx>
#include "backtrace.h"
#include "backtraceasstring.hxx"
@@ -58,4 +59,33 @@ OUString osl::detail::backtraceAsString(sal_uInt32 maxDepth) {
return b3.makeStringAndClear();
}
+std::unique_ptr<BacktraceState> sal_backtrace_get(sal_uInt32 maxDepth)
+{
+ assert(maxDepth != 0);
+ auto const maxInt = static_cast<unsigned int>(
+ std::numeric_limits<int>::max());
+ if (maxDepth > maxInt) {
+ maxDepth = static_cast<sal_uInt32>(maxInt);
+ }
+ auto b1 = new void *[maxDepth];
+ int n = backtrace(b1, static_cast<int>(maxDepth));
+ return std::unique_ptr<BacktraceState>(new BacktraceState{ b1, n });
+}
+
+OUString sal_backtrace_to_string(BacktraceState* backtraceState)
+{
+ FreeGuard b2(backtrace_symbols(backtraceState->buffer, backtraceState->nDepth));
+ if (b2.buffer == nullptr) {
+ return OUString();
+ }
+ OUStringBuffer b3;
+ for (int i = 0; i != backtraceState->nDepth; ++i) {
+ if (i != 0) {
+ b3.append("\n");
+ }
+ b3.append(o3tl::runtimeToOUString(b2.buffer[i]));
+ }
+ return b3.makeStringAndClear();
+}
+
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sal/osl/w32/backtrace.cxx b/sal/osl/w32/backtrace.cxx
index a8a4cf445e9f..230adca4a2f8 100644
--- a/sal/osl/w32/backtrace.cxx
+++ b/sal/osl/w32/backtrace.cxx
@@ -19,6 +19,7 @@
#include <DbgHelp.h>
#include <rtl/ustrbuf.hxx>
+#include <sal/backtrace.hxx>
#include "backtraceasstring.hxx"
@@ -64,4 +65,54 @@ OUString osl::detail::backtraceAsString(sal_uInt32 maxDepth)
return aBuf.makeStringAndClear();
}
+std::unique_ptr<BacktraceState> sal_backtrace_get(sal_uInt32 maxDepth)
+{
+ assert(maxDepth != 0);
+ auto const maxUlong = std::numeric_limits<ULONG>::max();
+ if (maxDepth > maxUlong) {
+ maxDepth = static_cast<sal_uInt32>(maxUlong);
+ }
+
+ HANDLE hProcess = GetCurrentProcess();
+ SymInitialize( hProcess, nullptr, true );
+
+ auto pStack = new void *[maxDepth];
+ // https://msdn.microsoft.com/en-us/library/windows/desktop/bb204633.aspx
+ // "CaptureStackBackTrace function" claims that you "can capture up to
+ // MAXUSHORT frames", and on Windows Server 2003 and Windows XP it even
+ // "must be less than 63", but assume that a too large input value is
+ // clamped internally, instead of resulting in an error:
+ int nFrames = CaptureStackBackTrace( 0, static_cast<ULONG>(maxDepth), pStack, nullptr );
+
+ return std::unique_ptr<BacktraceState>(new BacktraceState{ pStack, nFrames });
+}
+
+OUString sal_backtrace_to_string(BacktraceState* backtraceState)
+{
+ OUStringBuffer aBuf;
+
+ SYMBOL_INFO * pSymbol;
+ pSymbol = static_cast<SYMBOL_INFO *>(calloc( sizeof( SYMBOL_INFO ) + 1024 * sizeof( char ), 1 ));
+ pSymbol->MaxNameLen = 1024 - 1;
+ pSymbol->SizeOfStruct = sizeof( SYMBOL_INFO );
+ HANDLE hProcess = GetCurrentProcess();
+
+ auto nFrames = backtraceState->nDepth;
+ for( int i = 0; i < nFrames; i++ )
+ {
+ SymFromAddr( hProcess, reinterpret_cast<DWORD64>(backtraceState->buffer[ i ]), nullptr, pSymbol );
+ aBuf.append( (sal_Int32)(nFrames - i - 1) );
+ aBuf.append( ": " );
+ aBuf.appendAscii( pSymbol->Name );
+ aBuf.append( " - 0x" );
+ aBuf.append( (sal_Int64)pSymbol->Address, 16 );
+ aBuf.append( "\n" );
+ }
+
+ free( pSymbol );
+
+ return aBuf.makeStringAndClear();
+}
+
+
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sal/util/sal.map b/sal/util/sal.map
index b04487e4d548..e9195a1434f8 100644
--- a/sal/util/sal.map
+++ b/sal/util/sal.map
@@ -733,6 +733,12 @@ PRIVATE_1.3 { # LibreOffice 5.4
sal_detail_log_report;
} PRIVATE_1.2;
+PRIVATE_1.4 { # LibreOffice 6.0
+ global:
+ _Z17sal_backtrace_getj;
+ _Z23sal_backtrace_to_stringP14BacktraceState;
+} PRIVATE_1.3;
+
PRIVATE_textenc.1 { # LibreOffice 3.6
global:
_ZN3sal6detail7textenc20convertCharToUnicode*;
diff --git a/sd/source/ui/sidebar/MasterPageObserver.cxx b/sd/source/ui/sidebar/MasterPageObserver.cxx
index 86a9c15e3f3d..57ea45fd6e23 100644
--- a/sd/source/ui/sidebar/MasterPageObserver.cxx
+++ b/sd/source/ui/sidebar/MasterPageObserver.cxx
@@ -162,9 +162,11 @@ void MasterPageObserver::Implementation::RegisterDocument (
aMasterPageSet.insert (pMasterPage->GetName());
}
+ bool bAlreadyExists = maUsedMasterPages.find(&rDocument) != maUsedMasterPages.end();
maUsedMasterPages[&rDocument] = aMasterPageSet;
- StartListening (rDocument);
+ if (!bAlreadyExists)
+ StartListening (rDocument);
}
void MasterPageObserver::Implementation::UnregisterDocument (
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
}
diff --git a/svx/source/unodraw/unoshape.cxx b/svx/source/unodraw/unoshape.cxx
index 49dbba665cd4..d0e59bcbeb8b 100644
--- a/svx/source/unodraw/unoshape.cxx
+++ b/svx/source/unodraw/unoshape.cxx
@@ -353,15 +353,20 @@ void SvxShape::impl_initFromSdrObject()
}
osl_atomic_decrement( &m_refCount );
- mpModel = mpObj->GetModel();
+ auto pNewModel = mpObj->GetModel();
+
+ if (pNewModel != mpModel)
+ {
+ if (mpModel)
+ EndListening( *mpModel );
+ if (pNewModel)
+ StartListening( *pNewModel );
+ mpModel = pNewModel;
+ }
// #i40944#
// Do not simply return when no model but do the type corrections
// following below.
- if(mpModel)
- {
- StartListening( *mpModel );
- }
const SdrInventor nInventor = mpObj->GetObjInventor();
diff --git a/sw/source/core/layout/frmtool.cxx b/sw/source/core/layout/frmtool.cxx
index f058859f0401..5beb864aa1f6 100644
--- a/sw/source/core/layout/frmtool.cxx
+++ b/sw/source/core/layout/frmtool.cxx
@@ -3181,8 +3181,13 @@ public:
void SwFrameHolder::SetFrame( SwFrame* pHold )
{
bSet = true;
- pFrame = pHold;
- StartListening(*pHold);
+ if (pFrame != pHold)
+ {
+ if (pFrame)
+ EndListening(*pFrame);
+ StartListening(*pHold);
+ pFrame = pHold;
+ }
}
void SwFrameHolder::Reset()
diff --git a/sw/source/uibase/utlui/navipi.cxx b/sw/source/uibase/utlui/navipi.cxx
index e580cefb034f..9765031357ed 100644
--- a/sw/source/uibase/utlui/navipi.cxx
+++ b/sw/source/uibase/utlui/navipi.cxx
@@ -718,8 +718,6 @@ SwNavigationPI::SwNavigationPI(SfxBindings* _pBindings,
m_aGlobalTree->SetFont(aFont);
StartListening(*SfxGetpApp());
- if ( m_pCreateView )
- StartListening(*m_pCreateView);
sal_uInt16 nNavId = m_aContentToolBox->GetItemId("navigation");
m_aContentToolBox->SetItemBits(nNavId, m_aContentToolBox->GetItemBits(nNavId) | ToolBoxItemBits::DROPDOWNONLY );
@@ -886,6 +884,7 @@ void SwNavigationPI::Notify( SfxBroadcaster& rBrdc, const SfxHint& rHint )
{
if (rHint.GetId() == SfxHintId::Dying)
{
+ EndListening(*m_pCreateView);
m_pCreateView = nullptr;
}
}
More information about the Libreoffice-commits
mailing list