[PATCH libreoffice-4-0] resolved fdo#63421 crash in pivot table with accessibility

Hi,

I have submitted a patch for review:

    https://gerrit.libreoffice.org/3372

To pull it, you can do:

    git pull ssh://gerrit.libreoffice.org:29418/core refs/changes/72/3372/1

resolved fdo#63421 crash in pivot table with accessibility

The scenario of fdo#63421 (loading data and re-dragging the same field)
is not needed, simple data is sufficient and crash happened also when
dragging (removing) a field from a pane and dropping it anywhere else.

Multiple errors:
* getAccessibleChildCount() must return the real current count of
  children, not what mpFieldWindow says; AtkListener::updateChildList()
  uses this value to repopulate its own list; a child is added after it
  is added to mpFieldWindow but removed before it is removed from
  mpFieldWindow;
* LostFocus() uses an index of -1 if the last child was already removed
  and the field was dropped after dragging it away from a pane, handle
  that but it still does not look right
* RemoveField() called CommitChange() with
  AccessibleEventObject::NewValue set instead of OldValue, leading to
  AtkListener::handleChildAdded() being called instead of
  handleChildRemoved()

Apparently this never worked since 2002.

Change-Id: Idfb59d947002d2212bc67b414daecb65c55edae8
(cherry picked from commit 26114dcdf9d55a5a2490de6de619337e9733b0e2)
---
M sc/source/ui/Accessibility/AccessibleDataPilotControl.cxx
1 file changed, 64 insertions(+), 27 deletions(-)



diff --git a/sc/source/ui/Accessibility/AccessibleDataPilotControl.cxx 
b/sc/source/ui/Accessibility/AccessibleDataPilotControl.cxx
index 035004ae..8490572 100644
--- a/sc/source/ui/Accessibility/AccessibleDataPilotControl.cxx
+++ b/sc/source/ui/Accessibility/AccessibleDataPilotControl.cxx
@@ -259,7 +259,7 @@
         AccessibleEventObject aEvent;
         aEvent.EventId = AccessibleEventId::CHILD;
         aEvent.Source = uno::Reference< XAccessibleContext >(this);
-        aEvent.NewValue <<= xTempAcc;
+        aEvent.OldValue <<= xTempAcc;
 
         CommitChange(aEvent); // gone child - event
 
@@ -270,25 +270,41 @@
 
 void ScAccessibleDataPilotControl::FieldFocusChange(sal_Int32 nOldIndex, sal_Int32 nNewIndex)
 {
-    OSL_ENSURE(static_cast<size_t>(nOldIndex) < maChildren.size() &&
-                static_cast<size_t>(nNewIndex) < maChildren.size(), "did not recognize a child 
count change");
+    if (0 <= nOldIndex && static_cast<size_t>(nOldIndex) < maChildren.size())
+    {
+        uno::Reference < XAccessible > xTempAcc = maChildren[nOldIndex].xWeakAcc;
+        if (xTempAcc.is() && maChildren[nOldIndex].pAcc)
+            maChildren[nOldIndex].pAcc->ResetFocused();
+    }
+    else
+    {
+        SAL_WARN( "sc", "ScAccessibleDataPilotControl::FieldFocusChange() old index out of bounds: 
" << nOldIndex);
+    }
 
-    uno::Reference < XAccessible > xTempAcc = maChildren[nOldIndex].xWeakAcc;
-    if (xTempAcc.is() && maChildren[nOldIndex].pAcc)
-        maChildren[nOldIndex].pAcc->ResetFocused();
-
-    xTempAcc = maChildren[nNewIndex].xWeakAcc;
-    if (xTempAcc.is() && maChildren[nNewIndex].pAcc)
-        maChildren[nNewIndex].pAcc->SetFocused();
+    if (0 <= nNewIndex && static_cast<size_t>(nNewIndex) < maChildren.size())
+    {
+        uno::Reference < XAccessible > xTempAcc = maChildren[nNewIndex].xWeakAcc;
+        if (xTempAcc.is() && maChildren[nNewIndex].pAcc)
+            maChildren[nNewIndex].pAcc->SetFocused();
+    }
+    else
+    {
+        SAL_WARN( "sc", "ScAccessibleDataPilotControl::FieldFocusChange() new index out of bounds: 
" << nNewIndex);
+    }
 }
 
 void ScAccessibleDataPilotControl::FieldNameChange(sal_Int32 nIndex)
 {
-    OSL_ENSURE(static_cast<size_t>(nIndex) < maChildren.size(), "did not recognize a child count 
change");
-
-    uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
-    if (xTempAcc.is() && maChildren[nIndex].pAcc)
-        maChildren[nIndex].pAcc->ChangeName();
+    if (0 <= nIndex && static_cast<size_t>(nIndex) < maChildren.size())
+    {
+        uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
+        if (xTempAcc.is() && maChildren[nIndex].pAcc)
+            maChildren[nIndex].pAcc->ChangeName();
+    }
+    else
+    {
+        SAL_WARN( "sc", "ScAccessibleDataPilotControl::FieldNameChange() index out of bounds: " << 
nIndex);
+    }
 }
 
 void ScAccessibleDataPilotControl::GotFocus()
@@ -298,9 +314,16 @@
         OSL_ENSURE(mpFieldWindow->GetFieldCount() == maChildren.size(), "did not recognize a child 
count change");
 
         sal_Int32 nIndex(mpFieldWindow->GetSelectedField());
-        uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
-        if (xTempAcc.is() && maChildren[nIndex].pAcc)
-            maChildren[nIndex].pAcc->SetFocused();
+        if (0 <= nIndex && static_cast<size_t>(nIndex) < maChildren.size())
+        {
+            uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
+            if (xTempAcc.is() && maChildren[nIndex].pAcc)
+                maChildren[nIndex].pAcc->SetFocused();
+        }
+        else
+        {
+            SAL_WARN( "sc", "ScAccessibleDataPilotControl::GotFocus() field index out of bounds: " 
<< nIndex);
+        }
     }
 }
 
@@ -311,9 +334,21 @@
         OSL_ENSURE(mpFieldWindow->GetFieldCount() == maChildren.size(), "did not recognize a child 
count change");
 
         sal_Int32 nIndex(mpFieldWindow->GetSelectedField());
-        uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
-        if (xTempAcc.is() && maChildren[nIndex].pAcc)
-            maChildren[nIndex].pAcc->ResetFocused();
+        if (0 <= nIndex && static_cast<size_t>(nIndex) < maChildren.size())
+        {
+            uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
+            if (xTempAcc.is() && maChildren[nIndex].pAcc)
+                maChildren[nIndex].pAcc->ResetFocused();
+        }
+        else
+        {
+            // This may actually happen if the last field is dropped somewhere
+            // and was already removed from the pane by dragging it away. This
+            // is odd.. looks like all LostFocus() are called for the previous
+            // field of the same original pane in the remove case?
+            SAL_WARN_IF( nIndex != -1 || !maChildren.empty(), "sc",
+                    "ScAccessibleDataPilotControl::LostFocus() field index out of bounds: " << 
nIndex);
+        }
     }
 }
 
@@ -390,10 +425,7 @@
 {
     SolarMutexGuard aGuard;
     IsObjectValid();
-    if (mpFieldWindow)
-        return mpFieldWindow->GetFieldCount();
-    else
-        return 0;
+    return static_cast<sal_Int32>(maChildren.size());
 }
 
 uno::Reference< XAccessible> SAL_CALL ScAccessibleDataPilotControl::getAccessibleChild(sal_Int32 
nIndex)
@@ -404,14 +436,19 @@
     uno::Reference<XAccessible> xAcc;
     if (mpFieldWindow)
     {
-        if (nIndex < 0 || static_cast< size_t >( nIndex ) >= mpFieldWindow->GetFieldCount())
+        if (nIndex < 0 || static_cast< size_t >( nIndex ) >= maChildren.size())
             throw lang::IndexOutOfBoundsException();
 
-        OSL_ENSURE(mpFieldWindow->GetFieldCount() == maChildren.size(), "did not recognize a child 
count change");
+        OSL_ENSURE( mpFieldWindow->GetFieldCount() == maChildren.size()         // all except ...
+                ||  mpFieldWindow->GetFieldCount() == maChildren.size() + 1,    // in CommitChange 
during RemoveField
+                "did not recognize a child count change");
 
         uno::Reference < XAccessible > xTempAcc = maChildren[nIndex].xWeakAcc;
         if (!xTempAcc.is())
         {
+            if (static_cast< size_t >( nIndex ) >= mpFieldWindow->GetFieldCount())
+                throw lang::IndexOutOfBoundsException();
+
             maChildren[nIndex].pAcc = new ScAccessibleDataPilotButton(this, mpFieldWindow, nIndex);
             xTempAcc = maChildren[nIndex].pAcc;
             maChildren[nIndex].xWeakAcc = xTempAcc;

-- 
To view, visit https://gerrit.libreoffice.org/3372
To unsubscribe, visit https://gerrit.libreoffice.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idfb59d947002d2212bc67b414daecb65c55edae8
Gerrit-PatchSet: 1
Gerrit-Project: core
Gerrit-Branch: libreoffice-4-0
Gerrit-Owner: Eike Rathke <erack@redhat.com>