Partially revert 3af1853260 to fix use-after-free

[tengyifei]

After upgrading to the latest release of Waybar the bar will crash whenever I close the laptop lid. After some debugging I believe it is because the watching added by watch_name in Host is not being correctly canceled using unwatch_name. After the Tray object and Host object are destroyed, additional callbacks will become use-after-free.

Apparently commit 3af1853 removed the unwatch_name. I'm not sure why it did that, but it seems dangerous.

Additionally, bus_name_id_ is created by own_name. According to that function's documentation, the correct inverse operation is unown_name.

[LukashonakV]

This commit was done due to in normal mode each destructor is responsible for resources cleaning for the particular class. For example, watcher should destroy resources that belong to the watcher instance. The same for host. Otherwise, waybar spoiles warning about its impossible to unown resource.
If your issue appeared due to the exact commit, we need to find another solution. For example, to check the event when it is really possible to unown resources belong to another class instances

[tengyifei]

@LukashonakV FWIW I think I was actually following your comment in this PR. So the Host::busAcquired method calls watch_name. Therefore the Host object owns some resource when that happens. When Host is destroyed, I call unwatch_name to release that resource. Without unwatching the name, I get extra callbacks into Host after it is already destroyed (e.g. I close the display to remove the bar). Here's the stack trace:

* thread #1, name = 'waybar', stop reason = signal SIGSEGV: invalid address (fault address: 0x555003649a37)
  * frame #0: 0x00005555557d6e6c waybar`std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::_Identity<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::_M_mbegin(this=0x0000555003649a27) const at stl_tree.h:735:64
    frame #1: 0x00005555557d6c15 waybar`std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::_Identity<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::_M_begin(this=0x0000555003649a27) at stl_tree.h:739:16
    frame #2: 0x00005555557d6b59 waybar`std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::_Identity<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::~_Rb_tree(this=0x0000555003649a27) at stl_tree.h:984:18
    frame #3: 0x00005555557d6aa5 waybar`std::set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::~set(this=size=1) at stl_set.h:281:22
    frame #4: 0x00005555557d6960 waybar`waybar::modules::SNI::Item::~Item(this=0x000055500364978f) at item.hpp:29:19
    frame #5: 0x00005555557d682b waybar`std::default_delete<waybar::modules::SNI::Item>::operator(this=0x000055555605fce0, __ptr=0x000055500364978f)(waybar::modules::SNI::Item*) const at unique_ptr.h:95:2
    frame #6: 0x00005555557d6793 waybar`std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>::~unique_ptr(this=0x55500364978f) at unique_ptr.h:396:4
    frame #7: 0x00005555557d6745 waybar`void std::_Destroy<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>>(__pointer=0x55500364978f) at stl_construct.h:151:19
    frame #8: 0x00005555557d6717 waybar`void std::_Destroy_aux<false>::__destroy<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>*>(__first=0x55500364978f, __last=0x6d6574) at stl_construct.h:163:6
    frame #9: 0x00005555557d66dd waybar`void std::_Destroy<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>*>(__first=0x55500364978f, __last=0x6d6574) at stl_construct.h:195:7
    frame #10: 0x00005555557d6651 waybar`void std::_Destroy<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>*, std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>>(__first=0x55500364978f, __last=0x6d6574, (null)=0x00005555559cc908) at alloc_traits.h:850:7
    frame #11: 0x00005555557d76ef waybar`std::vector<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>, std::allocator<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>>>::_M_erase_at_end(this=size=1, __pos=0x55500364978f) at stl_vector.h:1932:6
    frame #12: 0x00005555557d5228 waybar`std::vector<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>, std::allocator<std::unique_ptr<waybar::modules::SNI::Item, std::default_delete<waybar::modules::SNI::Item>>>>::clear(this=size=1) at stl_vector.h:1601:9
    frame #13: 0x00005555557d3e75 waybar`waybar::modules::SNI::Host::nameVanished(this=0x00005555559cc908, conn=0x00007fffffffc908, name=<unavailable>) at host.cpp:51:10
    frame #14: 0x00005555557d6596 waybar`sigc::bound_mem_functor2<void, waybar::modules::SNI::Host, Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring>::operator(this=0x0000555555a9ed98, _A_a1=0x00007fffffffc908, _A_a2=0x00007fffffffc910)(Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring const&) const at mem_fun.h:2143:14
    frame #15: 0x00005555557d64e9 waybar`sigc::adaptor_functor<sigc::bound_mem_functor2<void, waybar::modules::SNI::Host, Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring>>::deduce_result_type<Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring const&, void, void, void, void, void>::type sigc::adaptor_functor<sigc::bound_mem_functor2<void, waybar::modules::SNI::Host, Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring>>::operator(this=0x0000555555a9ed90, _A_arg1=0x00007fffffffc908, _A_arg2=0x00007fffffffc910)<Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring const&>(Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring const&) const at adaptor_trait.h:108:14
    frame #16: 0x00005555557d64b1 waybar`sigc::internal::slot_call2<sigc::bound_mem_functor2<void, waybar::modules::SNI::Host, Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring>, void, Glib::RefPtr<Gio::DBus::Connection> const&, Glib::ustring>::call_it(rep=0x0000555555a9ed60, a_1=0x00007fffffffc908, a_2=0x00007fffffffc910) at slot.h:205:36
    frame #17: 0x00007ffff772c3e8 libgiomm-2.4.so.1`___lldb_unnamed_symbol12247 + 184
    frame #18: 0x00007ffff6b358b8 libgio-2.0.so.0`___lldb_unnamed_symbol6826 + 232
    frame #19: 0x00007ffff6b35b06 libgio-2.0.so.0`___lldb_unnamed_symbol6828 + 310
    frame #20: 0x00007ffff6b2520b libgio-2.0.so.0`___lldb_unnamed_symbol6722 + 123
    frame #21: 0x00007ffff68df67f libglib-2.0.so.0`g_main_context_dispatch + 367
    frame #22: 0x00007ffff68dfa38 libglib-2.0.so.0`___lldb_unnamed_symbol2482 + 520
    frame #23: 0x00007ffff68dfacc libglib-2.0.so.0`g_main_context_iteration + 44
    frame #24: 0x00007ffff6aff65d libgio-2.0.so.0`g_application_run + 477
    frame #25: 0x00005555556c27c1 waybar`waybar::Client::main(this=0x000055555587f400, argc=1, argv=0x00007fffffffd918) at client.cpp:242:12
    frame #26: 0x00005555556a38f4 waybar`main(argc=1, argv=0x00007fffffffd918) at main.cpp:106:21
    frame #27: 0x00007ffff622618a libc.so.6`__libc_start_call_main(main=(waybar`main at main.cpp:72), argc=1, argv=0x00007fffffffd918) at libc_start_call_main.h:58:16
    frame #28: 0x00007ffff6226245 libc.so.6`__libc_start_main_impl(main=(waybar`main at main.cpp:72), argc=1, argv=0x00007fffffffd918, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffd908) at libc-start.c:381:3
    frame #29: 0x00005555556464e1 waybar`_start + 33

If your issue appeared due to the exact commit, we need to find another solution. For example, to check the event when it is really possible to unown resources belong to another class instances

Could you clarify which resource this is talking about? Is it about the watch_name and watcher_id_, or the own_name and bus_name_id_?

[LukashonakV]

Hi @tengyifei I would be able to check next week. Still on vacation)

[tengyifei]

Thanks of course -- it's not urgent. I'm running a locally compiled waybar which fixed the crash for me in the meantime.

[Alexays]

I'll merge this PR in order to publish a new Waybar update.
I prefer to have a warning instead of a crash.
Feel free to open another PR to properly fix the problem :)
Thanks @tengyifei and @LukashonakV