editeng/source/accessibility/AccessibleStaticTextBase.cxx | 15 +-- sc/CppunitTest_sc_a11y.mk | 1 sc/qa/extras/accessibility/basics.cxx | 18 ++++ sc/source/ui/Accessibility/AccessibleContextBase.cxx | 56 +++++++++----- sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx | 10 -- sc/source/ui/inc/AccessibleContextBase.hxx | 18 +--- sc/source/ui/inc/AccessibleSpreadsheet.hxx | 3 7 files changed, 71 insertions(+), 50 deletions(-)
New commits: commit bf421456ac92b695e8a8e40d0d434c8e52782f90 Author: Noel Grandin <noelgran...@gmail.com> AuthorDate: Sat Oct 7 11:42:40 2023 +0200 Commit: Patrick Luby <plub...@neooffice.org> CommitDate: Sun Oct 8 01:33:09 2023 +0200 tdf#157568 After deleting the content of a cell by pressing the delete.. .. key, Orca still speaks the content. this reverts commit f22cb3dfab413a2917cd810b8e1b8f644a016327 Author: Noel Grandin <noelgran...@gmail.com> Date: Mon Jun 12 20:02:19 2023 +0200 tdf#155376 weakly cache ScAccessibleCell which was a nice idea, but means that we would need to have some way of updating the ScAccessibleCell when the associated cell data changes. Which is likely to be complicated. So return to creating new ScAccessibleCell objects all the time, but fix them to not leak. Change-Id: Ie17ee5c950c9809d4c7281f93761584f75256121 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157673 Tested-by: Jenkins Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk> (cherry picked from commit 8e886993f32b7db11a99bdecf06451e6de6c3842) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157627 Reviewed-by: Michael Weghorn <m.wegh...@posteo.de> Reviewed-by: Patrick Luby <plub...@neooffice.org> diff --git a/editeng/source/accessibility/AccessibleStaticTextBase.cxx b/editeng/source/accessibility/AccessibleStaticTextBase.cxx index 954117189798..85082bc4cb21 100644 --- a/editeng/source/accessibility/AccessibleStaticTextBase.cxx +++ b/editeng/source/accessibility/AccessibleStaticTextBase.cxx @@ -121,8 +121,7 @@ namespace accessibility void SetEventSource( const uno::Reference< XAccessible >& rInterface ) { - - mxThis = rInterface; + mpThis = rInterface.get(); } void SetOffset( const Point& ); @@ -163,8 +162,8 @@ namespace accessibility // our frontend class (the one implementing the actual // interface). That's not necessarily the one containing the impl - // pointer - uno::Reference< XAccessible > mxThis; + // pointer. Note that this is not an uno::Reference to prevent ref-counting cycles and leaks. + XAccessible* mpThis; // implements our functionality, we're just an adapter (guarded by solar mutex) mutable rtl::Reference<AccessibleEditableTextPara> mxTextParagraph; @@ -207,7 +206,7 @@ namespace accessibility mxTextParagraph->Dispose(); // drop references - mxThis = nullptr; + mpThis = nullptr; mxTextParagraph.clear(); } @@ -215,7 +214,7 @@ namespace accessibility { if( !mxTextParagraph.is() ) - throw lang::DisposedException ("object has been already disposed", mxThis ); + throw lang::DisposedException ("object has been already disposed", mpThis ); // TODO: Have a different method on AccessibleEditableTextPara // that does not care about state changes @@ -273,7 +272,7 @@ namespace accessibility if( nFlatIndex < 0 ) throw lang::IndexOutOfBoundsException("AccessibleStaticTextBase_Impl::Index2Internal: character index out of bounds", - mxThis); + mpThis); // gratuitously accepting larger indices here, AccessibleEditableTextPara will throw eventually sal_Int32 nCurrPara, nCurrIndex, nParas, nCurrCount; @@ -305,7 +304,7 @@ namespace accessibility // not found? Out of bounds throw lang::IndexOutOfBoundsException("AccessibleStaticTextBase_Impl::Index2Internal: character index out of bounds", - mxThis); + mpThis); } bool AccessibleStaticTextBase_Impl::SetSelection( sal_Int32 nStartPara, sal_Int32 nStartIndex, diff --git a/sc/CppunitTest_sc_a11y.mk b/sc/CppunitTest_sc_a11y.mk index e013beb987cc..3418f2fd4c44 100644 --- a/sc/CppunitTest_sc_a11y.mk +++ b/sc/CppunitTest_sc_a11y.mk @@ -16,6 +16,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,sc_a11y, \ $(eval $(call gb_CppunitTest_use_libraries,sc_a11y, \ sal \ cppu \ + cppuhelper \ subsequenttest \ test \ tl \ diff --git a/sc/qa/extras/accessibility/basics.cxx b/sc/qa/extras/accessibility/basics.cxx index f07a24d1a353..03b5eadabe54 100644 --- a/sc/qa/extras/accessibility/basics.cxx +++ b/sc/qa/extras/accessibility/basics.cxx @@ -108,5 +108,23 @@ CPPUNIT_TEST_FIXTURE(test::AccessibleTestBase, Test64BitChildIndices) CPPUNIT_ASSERT_EQUAL(nCol, xTable->getAccessibleColumn(nChildIndex)); } +CPPUNIT_TEST_FIXTURE(test::AccessibleTestBase, tdf157568) +{ + load(u"private:factory/scalc"); + + uno::Reference<accessibility::XAccessibleTable> sheet( + getDocumentAccessibleContext()->getAccessibleChild(0)->getAccessibleContext(), // sheet 1 + uno::UNO_QUERY_THROW); + + uno::Reference<accessibility::XAccessible> xCell = sheet->getAccessibleCellAt(1, 1); + CPPUNIT_ASSERT(xCell); + uno::WeakReference<accessibility::XAccessible> xCellWeak(xCell); + xCell.clear(); + // Verify that there are no reference cycles and that the ScAccessibleCell object dies after we + // are done with it. + uno::Reference<accessibility::XAccessible> xCell2(xCellWeak); + CPPUNIT_ASSERT(!xCell2.is()); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/sc/source/ui/Accessibility/AccessibleContextBase.cxx b/sc/source/ui/Accessibility/AccessibleContextBase.cxx index f66f51077ca8..793669db3f26 100644 --- a/sc/source/ui/Accessibility/AccessibleContextBase.cxx +++ b/sc/source/ui/Accessibility/AccessibleContextBase.cxx @@ -36,6 +36,37 @@ using namespace ::com::sun::star; using namespace ::com::sun::star::accessibility; +/** + The listener is an internal class to prevent reference-counting cycles and therefore memory leaks. +*/ +typedef cppu::WeakComponentImplHelper< + css::accessibility::XAccessibleEventListener + > ScAccessibleContextBaseEventListenerWeakImpl; +class ScAccessibleContextBase::ScAccessibleContextBaseEventListener : public cppu::BaseMutex, public ScAccessibleContextBaseEventListenerWeakImpl +{ +public: + ScAccessibleContextBaseEventListener(ScAccessibleContextBase& rBase) + : ScAccessibleContextBaseEventListenerWeakImpl(m_aMutex), mrBase(rBase) {} + + using WeakComponentImplHelperBase::disposing; + + ///===== XAccessibleEventListener ======================================== + + virtual void SAL_CALL disposing( const lang::EventObject& rSource ) override + { + SolarMutexGuard aGuard; + if (rSource.Source == mrBase.mxParent) + dispose(); + } + + virtual void SAL_CALL + notifyEvent( + const css::accessibility::AccessibleEventObject& /*aEvent*/ ) override {} +private: + ScAccessibleContextBase& mrBase; +}; + + ScAccessibleContextBase::ScAccessibleContextBase( uno::Reference<XAccessible> xParent, const sal_Int16 aRole) @@ -67,7 +98,11 @@ void ScAccessibleContextBase::Init() { uno::Reference< XAccessibleEventBroadcaster > xBroadcaster (mxParent->getAccessibleContext(), uno::UNO_QUERY); if (xBroadcaster.is()) - xBroadcaster->addAccessibleEventListener(this); + { + if (!mxEventListener) + mxEventListener = new ScAccessibleContextBaseEventListener(*this); + xBroadcaster->addAccessibleEventListener(mxEventListener); + } } msName = createAccessibleName(); msDescription = createAccessibleDescription(); @@ -91,8 +126,8 @@ void SAL_CALL ScAccessibleContextBase::disposing() if (mxParent.is()) { uno::Reference< XAccessibleEventBroadcaster > xBroadcaster (mxParent->getAccessibleContext(), uno::UNO_QUERY); - if (xBroadcaster.is()) - xBroadcaster->removeAccessibleEventListener(this); + if (xBroadcaster && mxEventListener) + xBroadcaster->removeAccessibleEventListener(mxEventListener); mxParent = nullptr; } } @@ -378,21 +413,6 @@ void SAL_CALL } } - //===== XAccessibleEventListener ======================================== - -void SAL_CALL ScAccessibleContextBase::disposing( - const lang::EventObject& rSource ) -{ - SolarMutexGuard aGuard; - if (rSource.Source == mxParent) - dispose(); -} - -void SAL_CALL ScAccessibleContextBase::notifyEvent( - const AccessibleEventObject& /* aEvent */ ) -{ -} - // XServiceInfo OUString SAL_CALL ScAccessibleContextBase::getImplementationName() { diff --git a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx index 03b92c39b996..6e61edf2ddfd 100644 --- a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx +++ b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx @@ -927,15 +927,7 @@ rtl::Reference<ScAccessibleCell> ScAccessibleSpreadsheet::GetAccessibleCellAt(sa } else { - rtl::Reference<ScAccessibleCell> xCell; - auto it = m_mapCells.find(aCellAddress); - if (it != m_mapCells.end()) - xCell = it->second.get(); - if (xCell) - return xCell; - xCell = ScAccessibleCell::create(this, mpViewShell, aCellAddress, getAccessibleIndex(nRow, nColumn), meSplitPos, mpAccDoc); - m_mapCells.insert(std::make_pair(aCellAddress, xCell)); - return xCell; + return ScAccessibleCell::create(this, mpViewShell, aCellAddress, getAccessibleIndex(nRow, nColumn), meSplitPos, mpAccDoc); } } } diff --git a/sc/source/ui/inc/AccessibleContextBase.hxx b/sc/source/ui/inc/AccessibleContextBase.hxx index b7276873487f..f66dab963175 100644 --- a/sc/source/ui/inc/AccessibleContextBase.hxx +++ b/sc/source/ui/inc/AccessibleContextBase.hxx @@ -31,6 +31,7 @@ #include <cppuhelper/basemutex.hxx> #include <cppuhelper/compbase.hxx> #include <cppuhelper/implbase1.hxx> +#include <rtl/ref.hxx> namespace tools { class Rectangle; } @@ -38,14 +39,12 @@ namespace tools { class Rectangle; } This base class provides an implementation of the <code>AccessibleContext</code> service. */ - typedef cppu::WeakComponentImplHelper< css::accessibility::XAccessible, css::accessibility::XAccessibleComponent, css::accessibility::XAccessibleContext, css::accessibility::XAccessibleEventBroadcaster, - css::lang::XServiceInfo, - css::accessibility::XAccessibleEventListener + css::lang::XServiceInfo > ScAccessibleContextBaseWeakImpl; class ScAccessibleContextBase @@ -53,6 +52,8 @@ class ScAccessibleContextBase public ScAccessibleContextBaseWeakImpl, public SfxListener { +class ScAccessibleContextBaseEventListener; + public: //===== internal ======================================================== ScAccessibleContextBase( @@ -160,15 +161,6 @@ public: removeAccessibleEventListener( const css::uno::Reference<css::accessibility::XAccessibleEventListener>& xListener) override; - ///===== XAccessibleEventListener ======================================== - - virtual void SAL_CALL - disposing( const css::lang::EventObject& Source ) override; - - virtual void SAL_CALL - notifyEvent( - const css::accessibility::AccessibleEventObject& aEvent ) override; - ///===== XServiceInfo ==================================================== /** Returns an identifier for the implementation of this object. @@ -256,6 +248,8 @@ private: /** This is the role of this object. */ sal_Int16 maRole; + + rtl::Reference<ScAccessibleContextBaseEventListener> mxEventListener; }; /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/source/ui/inc/AccessibleSpreadsheet.hxx b/sc/source/ui/inc/AccessibleSpreadsheet.hxx index cfe604c7d4d6..78e735942270 100644 --- a/sc/source/ui/inc/AccessibleSpreadsheet.hxx +++ b/sc/source/ui/inc/AccessibleSpreadsheet.hxx @@ -22,7 +22,6 @@ #include <sal/config.h> #include <rtl/ref.hxx> -#include <unotools/weakref.hxx> #include "AccessibleTableBase.hxx" #include "viewdata.hxx" @@ -268,8 +267,6 @@ private: OUString m_strCurCellValue; ScRangeList m_LastMarkedRanges; OUString m_strOldTabName; - std::map<ScAddress, unotools::WeakReference< ScAccessibleCell > > - m_mapCells; }; /* vim:set shiftwidth=4 softtabstop=4 expandtab: */