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 <noelgrandin@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.grandin@collabora.co.uk>

7 files changed, 71 insertions, 50 deletions

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 8e04c87dacda..06cce36ded49 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 5bb0c83c144c..59f2f399032e 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 5319fd5ba442..fa57dd1a4fe7 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 9c9e86261492..d683dee6c2d2 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; }
 class AbsoluteScreenPixelRectangle;
@@ -39,14 +40,12 @@ class AbsoluteScreenPixelRectangle;
         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
@@ -54,6 +53,8 @@ class ScAccessibleContextBase
         public ScAccessibleContextBaseWeakImpl,
         public SfxListener
 {
+class ScAccessibleContextBaseEventListener;
+
 public:
     //=====  internal  ========================================================
     ScAccessibleContextBase(
@@ -161,15 +162,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.
@@ -257,6 +249,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 2202d92cdd6b..5cf8b7018b01 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: */