Skip to content

Commit

Permalink
Let executeAction always ProcessEventsToIdle
Browse files Browse the repository at this point in the history
An ASan UITest_writer_tests7
UITEST_TEST_NAME=tdf135938.tdf135938.test_tdf135938_cross_reference_update
occasionally failed for me with

> ==1994973==ERROR: AddressSanitizer: heap-use-after-free on address 0x60f00014d718 at pc 0x7f95f4946ffa bp 0x7f95907fd720 sp 0x7f95907fd718
> READ of size 8 at 0x60f00014d718 thread T33
>  #0 in std::__cxx1998::vector<std::unique_ptr<SvLBoxItem, std::default_delete<SvLBoxItem>>, std::allocator<std::unique_ptr<SvLBoxItem, std::default_delete<SvLBoxItem>>>>::size() const at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_vector.h:988:40
>  #1 in SvTreeListEntry::ItemCount() const at vcl/source/treelist/treelistentry.cxx:110:20
>  #2 in SvTabListBox::GetEntryText(SvTreeListEntry const*, unsigned short) at vcl/source/treelist/svtabbx.cxx:289:37
>  #3 in SvTabListBox::GetEntryText(SvTreeListEntry*) const at vcl/source/treelist/svtabbx.cxx:280:12
>  #4 in TreeListEntryUIObject::get_state() at vcl/source/treelist/uiobject.cxx:119:32
>  #5 in UIObjectUnoObj::getState() at vcl/source/uitest/uno/uiobject_uno.cxx:164:29
>  #6 in non-virtual thunk to UIObjectUnoObj::getState() at vcl/source/uitest/uno/uiobject_uno.cxx
>  #7 in gcc3::callVirtualMethod(void*, unsigned int, void*, _typelib_TypeDescriptionReference*, bool, unsigned long*, unsigned int, unsigned long*, double*) at bridges/source/cpp_uno/gcc3_linux_x86-64/callvirtualmethod.cxx:77:5
>  #8 in cpp_call(bridges::cpp_uno::shared::UnoInterfaceProxy*, bridges::cpp_uno::shared::VtableSlot, _typelib_TypeDescriptionReference*, int, _typelib_MethodParameter*, void*, void**, _uno_Any**) at bridges/source/cpp_uno/gcc3_linux_x86-64/uno2cpp.cxx:233:13
>  #9 in unoInterfaceProxyDispatch at bridges/source/cpp_uno/gcc3_linux_x86-64/uno2cpp.cxx:413:13
>  #10 in binaryurp::IncomingRequest::execute_throw(binaryurp::BinaryAny*, std::__debug::vector<binaryurp::BinaryAny, std::allocator<binaryurp::BinaryAny>>*) const at binaryurp/source/incomingrequest.cxx:236:13
>  #11 in binaryurp::IncomingRequest::execute() const at binaryurp/source/incomingrequest.cxx:79:26
>  #12 in request at binaryurp/source/reader.cxx:86:9
>  #13 in cppu_threadpool::JobQueue::enter(void const*, bool) at cppu/source/threadpool/jobqueue.cxx:100:17
>  #14 in cppu_threadpool::ORequestThread::run() at cppu/source/threadpool/thread.cxx:165:31
>
> 0x60f00014d718 is located 120 bytes inside of 168-byte region [0x60f00014d6a0,0x60f00014d748)
> freed by thread T0 here:
>  #0 in operator delete(void*, unsigned long) at ~/github.com/llvm/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:164:3
>  #1 in SvTreeListEntry::~SvTreeListEntry() at vcl/source/treelist/treelistentry.cxx:62:1
>  #2 in std::default_delete<SvTreeListEntry>::operator()(SvTreeListEntry*) const at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/unique_ptr.h:102:2
>  #3 in std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>::~unique_ptr() at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/unique_ptr.h:407:4
>  #4 in void std::destroy_at<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_construct.h:88:15
>  #5 in void std::_Destroy<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_construct.h:149:7
>  #6 in void std::_Destroy_aux<false>::__destroy<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*>(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*, std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_construct.h:163:6
>  #7 in void std::_Destroy<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*>(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*, std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_construct.h:195:7
>  #8 in void std::_Destroy<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*, std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*, std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*, std::allocator<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>&) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/alloc_traits.h:947:7
>  #9 in std::__cxx1998::vector<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>, std::allocator<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>>::_M_erase_at_end(std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>*) at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_vector.h:1932:6
>  #10 in std::__cxx1998::vector<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>, std::allocator<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>>::clear() at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/bits/stl_vector.h:1601:9
>  #11 in std::__debug::vector<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>, std::allocator<std::unique_ptr<SvTreeListEntry, std::default_delete<SvTreeListEntry>>>>::clear() at ~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/13.0.0/../../../../include/c++/13.0.0/debug/vector:729:9
>  #12 in SvTreeListEntry::ClearChildren() at vcl/source/treelist/treelistentry.cxx:28:16
>  #13 in SvTreeList::Clear() at vcl/source/treelist/treelist.cxx:123:16
>  #14 in SvTreeListBox::Clear() at vcl/source/treelist/treelistbox.cxx:422:17
>  #15 in SalInstanceTreeView::clear() at vcl/source/app/salvtables.cxx:4240:18
>  #16 in SwFieldRefPage::Reset(SfxItemSet const*) at sw/source/ui/fldui/fldref.cxx:175:16
>  #17 in SwFieldPage::EditNewField(bool) at sw/source/ui/fldui/fldpage.cxx:111:5
>  #18 in SwFieldDlg::ReInitTabPage(std::basic_string_view<char, std::char_traits<char>>, bool) at sw/source/ui/fldui/fldtdlg.cxx:218:16
>  #19 in SwFieldDlg::Activate() at sw/source/ui/fldui/fldtdlg.cxx:239:9
>  #20 in SwChildWinWrapper::UpdateHdl(Timer*) at sw/source/uibase/fldui/fldwrap.cxx:42:26
>  #21 in SwChildWinWrapper::LinkStubUpdateHdl(void*, Timer*) at sw/source/uibase/fldui/fldwrap.cxx:39:1
>  #22 in Link<Timer*, void>::Call(Timer*) const at include/tools/link.hxx:111:45
>  #23 in Timer::Invoke() at vcl/source/app/timer.cxx:75:21
>  #24 in Scheduler::CallbackTaskScheduling() at vcl/source/app/scheduler.cxx:481:20
>  #25 in SalTimer::CallCallback() at vcl/inc/saltimer.hxx:54:13
>  #26 in SvpSalInstance::CheckTimeout(bool) at vcl/headless/svpinst.cxx:161:53
>  #27 in SvpSalInstance::ImplYield(bool, bool) at vcl/headless/svpinst.cxx:399:17
>  #28 in SvpSalInstance::DoYield(bool, bool) at vcl/headless/svpinst.cxx:471:21
>  #29 in ImplYield(bool, bool) at vcl/source/app/svapp.cxx:475:48
>  #30 in Application::Yield() at vcl/source/app/svapp.cxx:559:5
>  #31 in Application::Execute() at vcl/source/app/svapp.cxx:453:13
>  #32 in desktop::Desktop::Main() at desktop/source/app/app.cxx:1604:13

causing

> ERROR: test_tdf135938_cross_reference_update (tdf135938.tdf135938)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "uitest/uitest/test.py", line 125, in execute_dialog_through_command
>     yield xDialog
>   File "uitest/uitest/test.py", line 140, in execute_modeless_dialog_through_command
>     yield xDialog
>   File "sw/qa/uitest/writer_tests7/tdf135938.py", line 40, in test_tdf135938_cross_reference_update
>     self.assertEqual("ABC", get_state_as_dict(xSelect.getChild(0))["Text"])
>   File "uitest/uitest/uihelper/common.py", line 13, in get_state_as_dict
>     return convert_property_values_to_dict(ui_object.getState())
> uitest.uihelper.common.com.sun.star.lang.DisposedException: Binary URP bridge disposed during call at binaryurp/source/bridge.cxx:613

The issue apparently is that TreeListEntryUIObject UNO objects, introduced in
71f562f "uitest: support tree lists", reference
SvTreeListEntry instances by pointers that can apparently go stale while those
UNO objects are still alive.  I'm not sure there would be an easy fix for this,
short of making those SvTreeListEntry instances ref-counted, or ripping out
UITest support for those TreeListEntryUIObjects again.

However, that underlying issue would presumably not be of practical concern if
the test_tdf135938_cross_reference_update Python code would have waited for
processEventsToIdle between its

>                 xInsert.executeAction("CLICK", tuple())

and the following block

>                 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"])

That way, there should be no more opportunity for the pointers in the passed-out
TreeListEntryUIObjects to become stale while the Python code accesses them.

And in Python UITest code, having to follow a call to executeAction by a call to
processEventsToIdle appears to be a common and recurring requirement, just look
at the most recent af1ca68 "another stab at
making this test reliable", d183dae
"tdf#153161: (Ab)use a call to XTextRange::getString to flush edits",
3cc6b87 "make an effort to fix the
UITest_inputLine intermittent failure", f466854
"tdf#150443 sw: fix crash of rejecting table row deletion".

So make executeAction always call ProcessEventsToIdle, to simplify this and
hopefully make UITests more robust.  (Removing any now-redundant calls to
processEventsToIdle, following calls to executeAction, from Python test code is
left as follow-up clean up for now.)

(I /think/ that the implementation of UIObjectUnoObj::executeAction could be
simplified now, as waiting for Scheduler::ProcessEventsToIdle() should already
make sure that aIdle has been processed, removing the need for the Notifier.
But lets leave that as a follow-up TODO for now.)

Change-Id: I41a5d51515dedaae44fb810b0ad3b0264c90abf3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/146434
Reviewed-by: Xisco Fauli <[email protected]>
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <[email protected]>
(cherry picked from commit 51a2e1a)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/146404
  • Loading branch information
stbergmann authored and Xisco Fauli committed Feb 1, 2023
1 parent 06fc2f9 commit 7341bed
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions vcl/source/uitest/uno/uiobject_uno.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <comphelper/propertyvalue.hxx>
#include <cppuhelper/supportsservice.hxx>
#include <tools/link.hxx>
#include <vcl/scheduler.hxx>
#include <vcl/svapp.hxx>
#include <vcl/idle.hxx>
#include <vcl/window.hxx>
Expand Down Expand Up @@ -153,6 +154,7 @@ void SAL_CALL UIObjectUnoObj::executeAction(const OUString& rAction, const css::

SolarMutexGuard aGuard;
aIdle.reset();
Scheduler::ProcessEventsToIdle();
}

css::uno::Sequence<css::beans::PropertyValue> UIObjectUnoObj::getState()
Expand Down

0 comments on commit 7341bed

Please sign in to comment.