tdf#153519 make TreeListEntryUIObject safer

Do not store a raw pointer to an object that go away.
Consequently we can remove the various sleep() hacks

Change-Id: I3200c26b3a2a4eb7592cb2e5c6af64d6b739d1f6
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/158390
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

9 files changed, 45 insertions, 104 deletions

diff --git a/include/vcl/uitest/uiobject.hxx b/include/vcl/uitest/uiobject.hxx
index 293471add9b5..d27140b2c21b 100644
--- a/include/vcl/uitest/uiobject.hxx
+++ b/include/vcl/uitest/uiobject.hxx
@@ -492,7 +492,7 @@ class TreeListEntryUIObject final : public UIObject
 {
 public:
 
-    TreeListEntryUIObject(const VclPtr<SvTreeListBox>& xTreeList, SvTreeListEntry* pEntry);
+    TreeListEntryUIObject(const VclPtr<SvTreeListBox>& xTreeList, std::vector<sal_Int32> nTreePath);
 
     virtual StringMap get_state() override;
 
@@ -507,9 +507,11 @@ public:
 
 private:
 
+    SvTreeListEntry* getEntry() const;
+
     VclPtr<SvTreeListBox> mxTreeList;
 
-    SvTreeListEntry* const mpEntry;
+    std::vector<sal_Int32> maTreePath;
 };
 
 class IconViewUIObject final : public TreeListUIObject
diff --git a/sw/qa/uitest/navigator/movechapterupdown.py b/sw/qa/uitest/navigator/movechapterupdown.py
index c9e41a0971cd..6bcda9b3d4bb 100644
--- a/sw/qa/uitest/navigator/movechapterupdown.py
+++ b/sw/qa/uitest/navigator/movechapterupdown.py
@@ -10,7 +10,6 @@
 from uitest.framework import UITestCase
 from libreoffice.uno.propertyvalue import mkPropertyValues
 from uitest.uihelper.common import get_state_as_dict, get_url_for_data_file
-import time
 
 class movechapterupdown(UITestCase):
 
@@ -26,15 +25,6 @@ class movechapterupdown(UITestCase):
             # wait until the navigator panel is available
             xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-            # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-            # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-            # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can
-            # invalidate the TreeListEntryUIObjects used by the below code (see
-            # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-            # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe
-            # side:
-            time.sleep(2)
-
             # Given the document chapter structure:
                 # 1. One H1
                 #   1.1. one_A (H2)
diff --git a/sw/qa/uitest/navigator/tdf134960.py b/sw/qa/uitest/navigator/tdf134960.py
index 270e9d347abb..8388b63e40f9 100644
--- a/sw/qa/uitest/navigator/tdf134960.py
+++ b/sw/qa/uitest/navigator/tdf134960.py
@@ -9,7 +9,6 @@
 from uitest.framework import UITestCase
 from libreoffice.uno.propertyvalue import mkPropertyValues
 from uitest.uihelper.common import get_state_as_dict, get_url_for_data_file
-import time
 
 class tdf134960_hyperlinks(UITestCase):
 
@@ -28,14 +27,6 @@ class tdf134960_hyperlinks(UITestCase):
         # wait until the navigator panel is available
         xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-        # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-        # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-        # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can invalidate
-        # the TreeListEntryUIObjects used by the below code (see
-        # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-        # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe side:
-        time.sleep(2)
-
         xContentTree = xNavigatorPanel.getChild("contenttree")
         xHyperlinks = self.get_item(xContentTree, 'Hyperlinks')
         self.assertEqual('Hyperlinks', get_state_as_dict(xHyperlinks)['Text'])
diff --git a/sw/qa/uitest/navigator/tdf137274.py b/sw/qa/uitest/navigator/tdf137274.py
index 9bd780a7e5c2..5273ddcb2f91 100644
--- a/sw/qa/uitest/navigator/tdf137274.py
+++ b/sw/qa/uitest/navigator/tdf137274.py
@@ -42,15 +42,6 @@ class tdf137274(UITestCase):
             # wait until the navigator panel is available
             xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-            # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-            # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-            # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can
-            # invalidate the TreeListEntryUIObjects used by the below code (see
-            # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-            # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe
-            # side:
-            time.sleep(2)
-
             xContentTree = xNavigatorPanel.getChild("contenttree")
             xComments = self.get_item(xContentTree, 'Comments')
             self.assertEqual('Comments', get_state_as_dict(xComments)['Text'])
diff --git a/sw/qa/uitest/navigator/tdf144672.py b/sw/qa/uitest/navigator/tdf144672.py
index 2dc11a8d3cf9..4bded66dcb08 100644
--- a/sw/qa/uitest/navigator/tdf144672.py
+++ b/sw/qa/uitest/navigator/tdf144672.py
@@ -10,7 +10,6 @@
 from uitest.framework import UITestCase
 from libreoffice.uno.propertyvalue import mkPropertyValues
 from uitest.uihelper.common import get_state_as_dict, get_url_for_data_file
-import time
 
 class tdf144672(UITestCase):
 
@@ -33,15 +32,6 @@ class tdf144672(UITestCase):
             # wait until the navigator panel is available
             xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-            # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-            # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-            # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can
-            # invalidate the TreeListEntryUIObjects used by the below code (see
-            # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-            # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe
-            # side:
-            time.sleep(2)
-
             xContentTree = xNavigatorPanel.getChild("contenttree")
 
             xReferences = self.get_item(xContentTree, 'References')
diff --git a/sw/qa/uitest/navigator/tdf151051.py b/sw/qa/uitest/navigator/tdf151051.py
index d0fb4ef0e708..1778cc94fe68 100644
--- a/sw/qa/uitest/navigator/tdf151051.py
+++ b/sw/qa/uitest/navigator/tdf151051.py
@@ -10,7 +10,6 @@
 from uitest.framework import UITestCase
 from libreoffice.uno.propertyvalue import mkPropertyValues
 from uitest.uihelper.common import get_state_as_dict, get_url_for_data_file
-import time
 
 class tdf151051(UITestCase):
 
@@ -26,15 +25,6 @@ class tdf151051(UITestCase):
             # wait until the navigator panel is available
             xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-            # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-            # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-            # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can
-            # invalidate the TreeListEntryUIObjects used by the below code (see
-            # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-            # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe
-            # side:
-            time.sleep(2)
-
             xContentTree = xNavigatorPanel.getChild("contenttree")
 
             xHeadings = xContentTree.getChild('0')
diff --git a/sw/qa/uitest/navigator/tdf154212.py b/sw/qa/uitest/navigator/tdf154212.py
index e7222b3d9097..2637780e2f4e 100644
--- a/sw/qa/uitest/navigator/tdf154212.py
+++ b/sw/qa/uitest/navigator/tdf154212.py
@@ -10,7 +10,6 @@
 from uitest.framework import UITestCase
 from libreoffice.uno.propertyvalue import mkPropertyValues
 from uitest.uihelper.common import get_state_as_dict, get_url_for_data_file
-import time
 
 class tdf154212(UITestCase):
 
@@ -26,15 +25,6 @@ class tdf154212(UITestCase):
             # wait until the navigator panel is available
             xNavigatorPanel = self.ui_test.wait_until_child_is_available('NavigatorPanel')
 
-            # HACK, see the `m_aUpdTimer.SetTimeout(1000)` in the SwContentTree ctor in
-            # sw/source/uibase/utlui/content.cxx, where that m_aUpdTimer is started by
-            # SwContentTree::ShowTree triggered from the SIDEBAR action above, and which can
-            # invalidate the TreeListEntryUIObjects used by the below code (see
-            # 2798430c8a711861fdcdfbf9ac00a0527abd3bfc "Mark the uses of TreeListEntryUIObject as
-            # dubious"); lets double that 1000 ms timeout value here to hopefully be on the safe
-            # side:
-            time.sleep(2)
-
             xNavigatorPanelContentTree = xNavigatorPanel.getChild("contenttree")
 
             xNavigatorPanelContentTreeHeadings = xNavigatorPanelContentTree.getChild('0')
diff --git a/sw/qa/uitest/writer_tests7/tdf135938.py b/sw/qa/uitest/writer_tests7/tdf135938.py
index 8faab5c47938..07298ab8d8e1 100755
--- a/sw/qa/uitest/writer_tests7/tdf135938.py
+++ b/sw/qa/uitest/writer_tests7/tdf135938.py
@@ -10,7 +10,6 @@
 from uitest.framework import UITestCase
 from uitest.uihelper.common import get_state_as_dict
 from libreoffice.uno.propertyvalue import mkPropertyValues
-import time
 
 class tdf135938(UITestCase):
 
@@ -29,15 +28,6 @@ class tdf135938(UITestCase):
                 xInsert = xDialog.getChild("ok")
                 xInsert.executeAction("CLICK", tuple())
 
-                # HACK, see the `m_aUpdateTimer.SetTimeout(200)` (to "avoid flickering of buttons")
-                # in the SwChildWinWrapper ctor in sw/source/uibase/fldui/fldwrap.cxx, where that
-                # m_aUpdateTimer is started by SwChildWinWrapper::ReInitDlg triggered from the
-                # xInsert click above, and which can invalidate the TreeListEntryUIObjects used by
-                # the below get_state_as_dict calls (see 2798430c8a711861fdcdfbf9ac00a0527abd3bfc
-                # "Mark the uses of TreeListEntryUIObject as dubious"); lets double that 200 ms
-                # timeout value here to hopefully be on the safe side:
-                time.sleep(.4);
-
                 xSelect = xDialog.getChild("select-ref")
                 self.assertEqual("1", get_state_as_dict(xSelect)["Children"])
                 self.assertEqual("ABC", get_state_as_dict(xSelect.getChild(0))["Text"])
@@ -46,15 +36,6 @@ class tdf135938(UITestCase):
                 xName.executeAction("TYPE", mkPropertyValues({"TEXT": "DEF"}))
                 xInsert.executeAction("CLICK", tuple())
 
-                # HACK, see the `m_aUpdateTimer.SetTimeout(200)` (to "avoid flickering of buttons")
-                # in the SwChildWinWrapper ctor in sw/source/uibase/fldui/fldwrap.cxx, where that
-                # m_aUpdateTimer is started by SwChildWinWrapper::ReInitDlg triggered from the
-                # xInsert click above, and which can invalidate the TreeListEntryUIObjects used by
-                # the below get_state_as_dict calls (see 2798430c8a711861fdcdfbf9ac00a0527abd3bfc
-                # "Mark the uses of TreeListEntryUIObject as dubious"); lets double that 200 ms
-                # timeout value here to hopefully be on the safe side:
-                time.sleep(.4);
-
                 self.assertEqual("2", get_state_as_dict(xSelect)["Children"])
                 self.assertEqual("ABC", get_state_as_dict(xSelect.getChild(0))["Text"])
                 self.assertEqual("DEF", get_state_as_dict(xSelect.getChild(1))["Text"])
diff --git a/vcl/source/treelist/uiobject.cxx b/vcl/source/treelist/uiobject.cxx
index 30ad0eaebc53..ca45d76fa9e6 100644
--- a/vcl/source/treelist/uiobject.cxx
+++ b/vcl/source/treelist/uiobject.cxx
@@ -68,10 +68,7 @@ std::unique_ptr<UIObject> TreeListUIObject::get_child(const OUString& rID)
         if (!pEntry)
             return nullptr;
 
-        return std::unique_ptr<UIObject>(new TreeListEntryUIObject(mxTreeList, pEntry));
-            //TODO: this looks dangerous, as there appears to be no protocol to guarantee that
-            // *pEntry will live as long as the new TreeListEntryUIObject referencing it by raw
-            // pointer
+        return std::unique_ptr<UIObject>(new TreeListEntryUIObject(mxTreeList, {nID}));
     }
     else if (nID == -1) // FIXME hack?
     {
@@ -109,24 +106,38 @@ std::unique_ptr<UIObject> TreeListUIObject::create(vcl::Window* pWindow)
     return std::unique_ptr<UIObject>(new TreeListUIObject(pTreeList));
 }
 
-TreeListEntryUIObject::TreeListEntryUIObject(const VclPtr<SvTreeListBox>& xTreeList, SvTreeListEntry* pEntry):
+TreeListEntryUIObject::TreeListEntryUIObject(const VclPtr<SvTreeListBox>& xTreeList, std::vector<sal_Int32> nTreePath):
     mxTreeList(xTreeList),
-    mpEntry(pEntry)
+    maTreePath(std::move(nTreePath))
 {
 }
 
+SvTreeListEntry* TreeListEntryUIObject::getEntry() const
+{
+    SvTreeListEntry* pEntry = nullptr;
+    for (sal_Int32 nID : maTreePath)
+    {
+        pEntry = mxTreeList->GetEntry(pEntry, nID);
+        if (!pEntry)
+            throw css::uno::RuntimeException("Could not find child with id: " + OUString::number(nID));
+    }
+    return pEntry;
+}
+
 StringMap TreeListEntryUIObject::get_state()
 {
+    SvTreeListEntry* pEntry = getEntry();
+
     StringMap aMap;
 
-    aMap["Text"] = mxTreeList->GetEntryText(mpEntry);
-    aMap["Children"] = OUString::number(mxTreeList->GetLevelChildCount(mpEntry));
-    aMap["VisibleChildCount"] = OUString::number(mxTreeList->GetVisibleChildCount(mpEntry));
-    aMap["IsSelected"] = OUString::boolean(mxTreeList->IsSelected(mpEntry));
+    aMap["Text"] = mxTreeList->GetEntryText(pEntry);
+    aMap["Children"] = OUString::number(mxTreeList->GetLevelChildCount(pEntry));
+    aMap["VisibleChildCount"] = OUString::number(mxTreeList->GetVisibleChildCount(pEntry));
+    aMap["IsSelected"] = OUString::boolean(mxTreeList->IsSelected(pEntry));
 
-    aMap["IsSemiTransparent"] = OUString::boolean(bool(mpEntry->GetFlags() & SvTLEntryFlags::SEMITRANSPARENT));
+    aMap["IsSemiTransparent"] = OUString::boolean(bool(pEntry->GetFlags() & SvTLEntryFlags::SEMITRANSPARENT));
 
-    SvLBoxButton* pItem = static_cast<SvLBoxButton*>(mpEntry->GetFirstItem(SvLBoxItemType::Button));
+    SvLBoxButton* pItem = static_cast<SvLBoxButton*>(pEntry->GetFirstItem(SvLBoxItemType::Button));
     if (pItem)
         aMap["IsChecked"] = OUString::boolean(pItem->IsStateChecked());
 
@@ -135,49 +146,52 @@ StringMap TreeListEntryUIObject::get_state()
 
 void TreeListEntryUIObject::execute(const OUString& rAction, const StringMap& /*rParameters*/)
 {
+    SvTreeListEntry* pEntry = getEntry();
+
     if (rAction == "COLLAPSE")
     {
-        mxTreeList->Collapse(mpEntry);
+        mxTreeList->Collapse(pEntry);
     }
     else if (rAction == "EXPAND")
     {
-        mxTreeList->Expand(mpEntry);
+        mxTreeList->Expand(pEntry);
     }
     else if (rAction == "SELECT")
     {
-        mxTreeList->Select(mpEntry);
+        mxTreeList->Select(pEntry);
     }
     else if (rAction == "DESELECT")
     {
-        mxTreeList->Select(mpEntry, false);
+        mxTreeList->Select(pEntry, false);
     }
     else if (rAction == "CLICK")
     {
-        SvLBoxButton* pItem = static_cast<SvLBoxButton*>(mpEntry->GetFirstItem(SvLBoxItemType::Button));
+        SvLBoxButton* pItem = static_cast<SvLBoxButton*>(pEntry->GetFirstItem(SvLBoxItemType::Button));
         if (!pItem)
             return;
-        pItem->ClickHdl(mpEntry);
+        pItem->ClickHdl(pEntry);
     }
     else if (rAction == "DOUBLECLICK")
     {
-        mxTreeList->SetCurEntry(mpEntry);
+        mxTreeList->SetCurEntry(pEntry);
         mxTreeList->DoubleClickHdl();
     }
 }
 
 std::unique_ptr<UIObject> TreeListEntryUIObject::get_child(const OUString& rID)
 {
+    SvTreeListEntry* pParentEntry = getEntry();
+
     sal_Int32 nID = rID.toInt32();
     if (nID >= 0)
     {
-        SvTreeListEntry* pEntry = mxTreeList->GetEntry(mpEntry, nID);
+        SvTreeListEntry* pEntry = mxTreeList->GetEntry(pParentEntry, nID);
         if (!pEntry)
             return nullptr;
 
-        return std::unique_ptr<UIObject>(new TreeListEntryUIObject(mxTreeList, pEntry));
-            //TODO: this looks dangerous, as there appears to be no protocol to guarantee that
-            // *pEntry will live as long as the new TreeListEntryUIObject referencing it by raw
-            // pointer
+        std::vector<sal_Int32> aChildTreePath(maTreePath);
+        aChildTreePath.push_back(nID);
+        return std::unique_ptr<UIObject>(new TreeListEntryUIObject(mxTreeList, std::move(aChildTreePath)));
     }
 
     return nullptr;
@@ -185,9 +199,11 @@ std::unique_ptr<UIObject> TreeListEntryUIObject::get_child(const OUString& rID)
 
 std::set<OUString> TreeListEntryUIObject::get_children() const
 {
+    SvTreeListEntry* pEntry = getEntry();
+
     std::set<OUString> aChildren;
 
-    size_t nChildren = mxTreeList->GetLevelChildCount(mpEntry);
+    size_t nChildren = mxTreeList->GetLevelChildCount(pEntry);
     for (size_t i = 0; i < nChildren; ++i)
     {
         aChildren.insert(OUString::number(i));