From a4928075958fd911d751a74b3a06e6730b557272 Mon Sep 17 00:00:00 2001 From: Caolán McNamara Date: Thu, 17 Jul 2014 10:29:36 +0100 Subject: fix memleak circular dependency of CElementList and CElement launching impress leaks 70+k ==1458== 78,741 (152 direct, 78,589 indirect) bytes in 1 blocks are definitely lost in loss record 24,296 of 24,315 ==1458== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1458== by 0x4C3895D: rtl_allocateMemory_SYSTEM(unsigned long) (alloc_global.cxx:270) ==1458== by 0x4C38A64: rtl_allocateMemory (alloc_global.cxx:303) ==1458== by 0x2DCC0B67: cppu::OWeakObject::operator new(unsigned long) (weak.hxx:85) ==1458== by 0x2DCCB3D3: DOM::CDocument::getElementsByTagName(rtl::OUString const&) (document.cxx:714) ==1458== by 0x25DC99D6: SdDrawDocument::InitLayoutVector() (drawdoc.cxx:1008) because the CElementList owns the CElement via m_pElement and m_pElement owns the CElementList via the addEventListener. Use a WeakEventListener pattern to let the CElement own that helper which itself doesn't own the CElementList but is owned by it instead, and forwards the events to the CElementList In order to use that pattern the CElementList must be have a m_refCount of 1 when the addEventListener is called, i.e. post ctor, so rename the original CElementList as CElementListImpl and call its registerListener from a wrapper CElementList Change-Id: Ibd4f19b619543a4ef580366c69efb61b526696ab --- unoxml/source/dom/elementlist.cxx | 65 ++++++++++++++++++++++++++++++++++----- unoxml/source/dom/elementlist.hxx | 50 ++++++++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 11 deletions(-) (limited to 'unoxml/source') diff --git a/unoxml/source/dom/elementlist.cxx b/unoxml/source/dom/elementlist.cxx index 98d150b2436e..9230917a2266 100644 --- a/unoxml/source/dom/elementlist.cxx +++ b/unoxml/source/dom/elementlist.cxx @@ -28,6 +28,34 @@ using namespace css::uno; using namespace css::xml::dom; using namespace css::xml::dom::events; +namespace +{ + class WeakEventListener : public ::cppu::WeakImplHelper1 + { + private: + css::uno::WeakReference mxOwner; + + public: + WeakEventListener(const css::uno::Reference& rOwner) + : mxOwner(rOwner) + { + } + + virtual ~WeakEventListener() + { + } + + virtual void SAL_CALL handleEvent(const css::uno::Reference& rEvent) + throw(css::uno::RuntimeException, std::exception) SAL_OVERRIDE + { + css::uno::Reference xOwner(mxOwner.get(), + css::uno::UNO_QUERY); + if (xOwner.is()) + xOwner->handleEvent(rEvent); + } + }; +} + namespace DOM { @@ -43,25 +71,46 @@ namespace DOM CElementList::CElementList(::rtl::Reference const& pElement, ::osl::Mutex & rMutex, OUString const& rName, OUString const*const pURI) + : m_xImpl(new CElementListImpl(pElement, rMutex, rName, pURI)) + { + if (pElement.is()) { + m_xImpl->registerListener(*pElement); + } + } + + CElementListImpl::CElementListImpl(::rtl::Reference const& pElement, + ::osl::Mutex & rMutex, + OUString const& rName, OUString const*const pURI) : m_pElement(pElement) , m_rMutex(rMutex) , m_pName(lcl_initXmlString(rName)) , m_pURI((pURI) ? lcl_initXmlString(*pURI) : 0) , m_bRebuild(true) { - if (m_pElement.is()) { - registerListener(*m_pElement); + } + + CElementListImpl::~CElementListImpl() + { + if (m_xEventListener.is() && m_pElement.is()) + { + Reference< XEventTarget > xTarget(static_cast(m_pElement.get()), UNO_QUERY); + assert(xTarget.is()); + if (!xTarget.is()) + return; + bool capture = false; + xTarget->removeEventListener("DOMSubtreeModified", m_xEventListener, capture); } } - void CElementList::registerListener(CElement & rElement) + void CElementListImpl::registerListener(CElement & rElement) { try { Reference< XEventTarget > const xTarget( static_cast(& rElement), UNO_QUERY_THROW); bool capture = false; + m_xEventListener = new WeakEventListener(this); xTarget->addEventListener("DOMSubtreeModified", - Reference< XEventListener >(this), capture); + m_xEventListener, capture); } catch (const Exception &e){ OString aMsg("Exception caught while registering NodeList as listener:\n"); aMsg += OUStringToOString(e.Message, RTL_TEXTENCODING_ASCII_US); @@ -69,7 +118,7 @@ namespace DOM } } - void CElementList::buildlist(xmlNodePtr pNode, bool start) + void CElementListImpl::buildlist(xmlNodePtr pNode, bool start) { // bail out if no rebuild is needed if (start) { @@ -107,7 +156,7 @@ namespace DOM /** The number of nodes in the list. */ - sal_Int32 SAL_CALL CElementList::getLength() throw (RuntimeException, std::exception) + sal_Int32 SAL_CALL CElementListImpl::getLength() throw (RuntimeException, std::exception) { ::osl::MutexGuard const g(m_rMutex); @@ -120,7 +169,7 @@ namespace DOM /** Returns the indexth item in the collection. */ - Reference< XNode > SAL_CALL CElementList::item(sal_Int32 index) + Reference< XNode > SAL_CALL CElementListImpl::item(sal_Int32 index) throw (RuntimeException, std::exception) { if (index < 0) throw RuntimeException(); @@ -139,7 +188,7 @@ namespace DOM } // tree mutations can change the list - void SAL_CALL CElementList::handleEvent(Reference< XEvent > const&) + void SAL_CALL CElementListImpl::handleEvent(Reference< XEvent > const&) throw (RuntimeException, std::exception) { ::osl::MutexGuard const g(m_rMutex); diff --git a/unoxml/source/dom/elementlist.hxx b/unoxml/source/dom/elementlist.hxx index 3cde5bce6fef..18289e6d111d 100644 --- a/unoxml/source/dom/elementlist.hxx +++ b/unoxml/source/dom/elementlist.hxx @@ -36,6 +36,7 @@ #include #include +#include #include namespace DOM @@ -44,11 +45,16 @@ namespace DOM typedef std::vector< xmlNodePtr > nodevector_t; - class CElementList + class CElementListImpl : public cppu::WeakImplHelper2< css::xml::dom::XNodeList, css::xml::dom::events::XEventListener > { private: + /** @short proxy weak binding to forward Events to ourself without + an ownership cycle + */ + css::uno::Reference< css::xml::dom::events::XEventListener > m_xEventListener; + ::rtl::Reference const m_pElement; ::osl::Mutex & m_rMutex; ::boost::scoped_array const m_pName; @@ -57,13 +63,16 @@ namespace DOM nodevector_t m_nodevector; void buildlist(xmlNodePtr pNode, bool start=true); - void registerListener(CElement & rElement); public: - CElementList(::rtl::Reference const& pElement, + CElementListImpl(::rtl::Reference const& pElement, ::osl::Mutex & rMutex, OUString const& rName, OUString const*const pURI = 0); + void registerListener(CElement & rElement); + + ~CElementListImpl(); + /** The number of nodes in the list. */ @@ -78,6 +87,41 @@ namespace DOM virtual void SAL_CALL handleEvent(const css::uno::Reference< css::xml::dom::events::XEvent >& evt) throw (css::uno::RuntimeException, std::exception) SAL_OVERRIDE; }; + + class CElementList + : public cppu::WeakImplHelper2< css::xml::dom::XNodeList, + css::xml::dom::events::XEventListener > + { + private: + rtl::Reference m_xImpl; + public: + CElementList(::rtl::Reference const& pElement, + ::osl::Mutex & rMutex, + OUString const& rName, OUString const*const pURI = 0); + + /** + The number of nodes in the list. + */ + virtual sal_Int32 SAL_CALL getLength() throw (css::uno::RuntimeException, std::exception) SAL_OVERRIDE + { + return m_xImpl->getLength(); + } + /** + Returns the indexth item in the collection. + */ + virtual css::uno::Reference< css::xml::dom::XNode > SAL_CALL item(sal_Int32 index) + throw (css::uno::RuntimeException, std::exception) SAL_OVERRIDE + { + return m_xImpl->item(index); + } + + // XEventListener + virtual void SAL_CALL handleEvent(const css::uno::Reference< css::xml::dom::events::XEvent >& evt) + throw (css::uno::RuntimeException, std::exception) SAL_OVERRIDE + { + m_xImpl->handleEvent(evt); + } + }; } #endif -- cgit