EventDispatcher::removeAllEventListeners types Vector is broken

[mori1234]

• axmol version: 2.4.1
• devices test on:
• developing environments
  • NDK version: r23c
  • Xcode version: 14.2+
  • Visual Studio:
    • VS version: 2022 (17.9+)
    • MSVC version: 19.39+
    • Windows SDK version: 10.0.22621.0+
  • cmake version:
    Steps to Reproduce:
    ``

1. void EventDispatcher::removeAllEventListeners()
   {
   bool cleanMap = true;
   std::vector<std::string_view> types;
   ......
   }

types vector is broken
std::vector<std::string_view> types; -> std::vector<std::string> types;

[rh101]

types vector is broken
std::vectorstd::string_view types; -> std::vectorstd::string types;

Why is it broken? You have provided no context to the issue. If you have code in your project that is resulting in some issue with EventDispatcher::removeAllEventListeners(), then show that code.

[mori1234]

when std::vector<std::string_view> types;

original
types[5] = __ax_keyboard
types[5] => \0_ax_key_board

std::vector<std::string> types;
works normally

[rh101]

I'm still not clear on what the issue actually is. Is there a crash?

types is defined as this:

std::vector<std::string_view> types;
types.reserve(_listenerMap.size());

The string_view entries are a read-only reference to the strings in hlookup::string_map<EventListenerVector*> _listenerMap;, where hlookup::string_map is defined as using string_map = tsl::robin_map<std::string, _Valty, string_hash, equal_to>;. The types vector is set in this section of code:

for (const auto& e : _listenerMap)
{
    if (_internalCustomListenerIDs.find(e.first) != _internalCustomListenerIDs.end())
    {
        cleanMap = false;
    }
    else
    {
        types.emplace_back(e.first);
    }
}

When removeEventListenersForListenerID is called, the event in _listenerMap is erased:
_listenerMap.erase(listenerItemIter);

This means that the std::string in that hlookup::string_map is no longer valid, so any memory associated with is undefined. This in turn means that the types vector std::string_view entry is now pointing at undefined memory, so it can contain any random values.

So, as far as I can tell, the screenshots you have provided show it working correctly. The screenshot showing the entry \0_ax_keyboard is correct, since that event listener has already been erased, and the std::string it was pointing to is no longer valid.

What is the actual problem that you are having?

[mori1234]

No crash, but memory leak occurs.
__ax_keyboard event listener is not deleted.
__ax_keyboard string is changed to \0_ax_key_board,
so it is not found in _listenerMap and is not deleted.

[rh101]

No crash, but memory leak occurs.
__ax_keyboard event listener is not deleted.
__ax_keyboard string is changed to \0_ax_key_board,
so it is not found in _listenerMap and is not deleted.

Something isn't right, and it is unlikely to be in EventDispatcher::removeAllEventListeners(). Please upload a minimal test project source code that reproduces the memory leak (either in a zip or provide github repo link).

One common cause of a memory leak in user projects is if a retain() is called on the event listener, but release() is never called on it. In this case, removeAllEventListeners() will remove the listener, but a reference is still being held by the user code, so it is never freed.

[mori1234]

I tested it with MainScene.cpp in the basic template.
It's easy to see when debugging with Visual Studio.
When debugging while closing the project,
the system tries to delete the listener, but because the string changes from __ax_keyboard to \0_ax_key_board,
it can't clear the listener, causing a memory leak.

[memory] LEAK: object 'class ax::EventListenerKeyboard' still active with reference count 1.

WARNING: Visual Leak Detector detected memory leaks!
---------- Block 2122 at 0x000000008FB3AB50: 288 bytes ----------
Leak Hash: 0x2901388D, Count: 1, Total 288 bytes
Call Stack (TID 19776):
ucrtbased.dll!malloc()
D:\a_work\1\s\src\vctools\crt\vcstartup\src\heap\new_scalar.cpp (35): JewelsUnchartednew.exe!operator new() + 0xA bytes
D:\TestProject\axmol\core\base\EventListenerKeyboard.cpp (48): JewelsUnchartednew.exe!ax::EventListenerKeyboard::create() + 0xA bytes
D:\TestProject\Source\MainScene.cpp (97): JewelsUnchartednew.exe!MainScene::init() + 0x5 bytes
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\type_traits (1549): JewelsUnchartednew.exe!std::invoke<bool (__cdecl MainScene::const &)(void),MainScene * &>()
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\functional (565): JewelsUnchartednew.exe!std::_Mem_fn<bool (__cdecl MainScene::)(void)>::operator()<MainScene * &>()
D:\TestProject\axmol\core\base\Utils.h (318): JewelsUnchartednew.exe!ax::utils::createInstance<MainScene,bool (__cdecl MainScene::*)(void)>() + 0x29 bytes
D:\TestProject\axmol\core\base\Utils.h (339): JewelsUnchartednew.exe!ax::utils::createInstance()
D:\TestProject\Source\AppDelegate.cpp (93): JewelsUnchartednew.exe!AppDelegate::applicationDidFinishLaunching() + 0x5 bytes
D:\TestProject\axmol\core\platform\win32\Application-win32.cpp (83): JewelsUnchartednew.exe!ax::Application::run() + 0x16 bytes
D:\TestProject\proj.win32\main.cpp (41): JewelsUnchartednew.exe!axmol_main() + 0xD bytes
D:\TestProject\proj.win32\main.cpp (55): JewelsUnchartednew.exe!wWinMain() + 0x5 bytes
D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl (123): JewelsUnchartednew.exe!invoke_main()
D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl (288): JewelsUnchartednew.exe!__scrt_common_main_seh() + 0x5 bytes
D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl (331): JewelsUnchartednew.exe!__scrt_common_main()
D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_wwinmain.cpp (17): JewelsUnchartednew.exe!wWinMainCRTStartup()
KERNEL32.DLL!BaseThreadInitThunk() + 0x14 bytes
ntdll.dll!RtlUserThreadStart() + 0x21 bytes

std::vector<std::string_view> types => std::vector<std::string> types change

[rh101]

I tested it with MainScene.cpp in the basic template. It's easy to see when debugging with Visual Studio. When debugging while closing the project, the system tries to delete the listener, but because the string changes from __ax_keyboard to \0_ax_key_board, it can't clear the listener, causing a memory leak.

That helps, thank you. Using a new project, I can now see what you are referring to.

The issue is happening when an event listener is removed, it corrupts the std::string_view that is next in the std::vector. This is reproducible with a new project, causing problems with the __ax_keyboard entry. If used with different event listeners in the list, then it does not always occur.

You can see it happening here:

In this you can see that the memory changes when __ax_touch_all_at_once is removed:

The std::vector<std::string_view> is used in several places, but I'm not too familiar with the workings of the std::string_view to know if it is the problem, or something else is, such as the std::string it is pointing to. At a guess, the std::string it is pointing to is re-allocated and copied (as can be seen by the 2nd gif above), but the std::string_view still points to the old std::string, which is corrupted.

@halx99 Any ideas regarding this? Should we change all usage of the temporary std::vector<std::string_view> to std::vector<std::string>?

[rh101]

This is the callstack when the memory is changed:

  void erase_from_bucket(iterator pos) {
    pos.m_bucket->clear();
    m_nb_elements--;

    /**
     * Backward shift, swap the empty bucket, previous_ibucket, with the values
     * on its right, ibucket, until we cross another empty bucket or if the
     * other bucket has a distance_from_ideal_bucket == 0.
     *
     * We try to move the values closer to their ideal bucket.
     */
    std::size_t previous_ibucket =
        static_cast<std::size_t>(pos.m_bucket - m_buckets);
    std::size_t ibucket = next_bucket(previous_ibucket);

    while (m_buckets[ibucket].dist_from_ideal_bucket() > 0) {
      tsl_rh_assert(m_buckets[previous_ibucket].empty());

      // *** This section is entered when erasing the second-last entry in the bucket, which causes the problem with the string ***

      const distance_type new_distance =
          distance_type(m_buckets[ibucket].dist_from_ideal_bucket() - 1);
      m_buckets[previous_ibucket].set_value_of_empty_bucket(
          new_distance, m_buckets[ibucket].truncated_hash(),
          std::move(m_buckets[ibucket].value())); // this section here is what causes the issue
      m_buckets[ibucket].clear();

      previous_ibucket = ibucket;
      ibucket = next_bucket(ibucket);
    }
  }

[halx99]

I will test it later

[rh101]

Simple reproducible example of the issue:

hlookup::string_map<void*> map;

map.emplace("__one", nullptr);
map.emplace("__two", nullptr);
map.emplace("__three", nullptr);
map.emplace("__four", nullptr);

std::vector<std::string_view> keyList;
keyList.reserve(map.size());

for (const auto & valuePair : map)
{
    keyList.emplace_back(valuePair.first);
}

for (const auto & item : keyList)
{
    map.erase(item);
}

// At this point you will notice that key "__three" is never removed from map

It seems that we cannot reliably hold references to keys in a hlookup::string_map using std::string_view, since the internal robin_map implementation does move data around, invalidating the std::string_view references. It doesn't seem like anything is wrong withe robin_map implementation, so this is just something we need to be aware of, and just use std::vector<std::string> instead to store the keys.

EDIT: Does this remark apply to this issue?

References and pointers to keys or values in the map are invalidated in the same way as iterators to these keys-values.

From: https://github.com/Tessil/robin-map?tab=readme-ov-file#differences-compared-to-stdunordered_map

[halx99]

I reproduced it, and test std::unordered_map work well, current best solution is reverted to use std::vector<std::string>, and consider in the future:

1. wip-v3: when we decide use c++23 as minmal c++std, c++23 stl map/unordered_map fully support heterogeneous find and erase operations, may switch to stl
2. Actually we should avoid use std::string as key, so may refactor SpriteFrameCache, use int64(a xxhash64 value) as key, it will reduce lots of memory in sprite frame cache

[halx99]

I just create a refactor PR: #2444

[rh101]

I reproduced it, and test std::unordered_map work well, current best solution is reverted to use std::vector<std::string>, and consider in the future:

Using std::vector<std::string> should fix all issues with anything related to holding keys from robin_map. There is a chance a similar problem is responsible for #2292 as well, because of std::vector<std::string_view> toRemoveFrames; in SpriteFrameCache::removeUnusedSpriteFrames()

2. Actually we should avoid use std::string as key, so may refactor SpriteFrameCache, use int64(a xxhash64 value) as key, it will reduce lots of memory in sprite frame cache

If there are no downsides to doing this, then that would be great.

[halx99]

I reproduced it, and test std::unordered_map work well, current best solution is reverted to use std::vector<std::string>, and consider in the future:

Using std::vector<std::string> should fix all issues with anything related to holding keys from robin_map. There is a chance a similar problem is responsible for #2292 as well, because of std::vector<std::string_view> toRemoveFrames; in SpriteFrameCache::removeUnusedSpriteFrames()

2. Actually we should avoid use std::string as key, so may refactor SpriteFrameCache, use int64(a xxhash64 value) as key, it will reduce lots of memory in sprite frame cache

If there are no downsides to doing this, then that would be great.

Yes, I also write test case, generate 100W 10~255 string, test1: insert directly, erase 50W, test2: calc xxhash64 then insert to int64 key, erase 50W, calc xxh64 first then call map erase

Debug:

=== Test1: string key ===
Insert: 4982 ms
Delete: 3536ms
Total: 8518ms


=== Test2: xxhash int64 key ===
Insert: 2325 ms
Delete: 1566ms
Total: 3891ms

Release (std::map)

=== Generating 1000000 keys ...
=== Test1: string key ===
Insert: 1914 ms
Delete: 1369ms
Total: 3283ms


=== Test2: xxhash int64 key ===
Insert: 974 ms
Delete: 778ms
Total: 1752ms

Release(tsl::robin_map)

=== Generating 1000000 keys ...
=== Test1: string key ===
Insert: 492 ms
Delete: 272ms
Total: 764ms


=== Test2: xxhash int64 key ===
Insert: 197 ms
Delete: 86ms
Total: 283ms