ImVector erase assertion failure

[sigmareaver]

Version/Branch of Dear ImGui:

Version: 1.87 WIP
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_opengl2.cpp
Compiler: MSVC v142 C++ (x86 Debug configuration)
Operating System: Windows 7

My Issue/Question:
Assertion failed:
it >= Data && it < Data + Size && it_last > it && it_last <= Data + Size, file \imgui\imgui.h, line 1807

Stacktrace:

ImVector<ImGuiInputEvent>::erase(const ImGuiInputEvent * it=0x0871fbf0, const ImGuiInputEvent * it_last=0x0871fbf0) Line 1807	C++
ImGui::UpdateInputEvents(bool trickle_fast_inputs=true) Line 7871	C++
ImGui::NewFrame() Line 4329	C++

Screenshot

Standalone, minimal, complete and verifiable example: (see #2261)
The assertion occurred while I was moving a slider (FrameRounding) in the ImGui::ShowStyleEditor();

This isn't the first time I've had ImGui crash while playing with the built-in style editor, but it's very rare.

So, it's doing g.InputEventsQueue.erase(g.InputEventsQueue.Data, g.InputEventsQueue.Data + event_n);
I'm noticing that g.InputEventsQueue.Data has an address of 0x0870e2e8 in the frame that is calling erase, yet inside of erase, the iterator argument passed in has a different address of 0x0871fbf0?

[sigmareaver]

Additional notes:
This crash seems to be happening more frequently. Things I've changed prior to it happening more frequently are:

• Using ImGui Freetype
• Loading a non-default font

I had it happen in two different finite state machine states. The GUI code I use in each state is completely different except for the StyleEditor window. For testing, I'm using that in both states. This second time it's crashed, I wasn't doing anything except moving the mouse across the application window.

Can this be related to the way I pass events to ImGui?

[PathogenDavid]

What platform backend are you using?

Can you reproduce this issue in the official demo?

[sigmareaver]

I'm using OpenGL 1/2. I haven't attempted to reproduce this with the official demo.
I found this:
https://stackoverflow.com/questions/433274/c-parameters-value-changes-between-stack-frames-in-stdvector
I believe this issue is related.

[PathogenDavid]

OpenGL is your rendering backend. You haven't specified your platform backend. (IE: GLFW, SDL, Win32, custom)

[thedmd]

It looks like it_last > it condition should be it_last >= it.

This code should not assert:

ImVector<int> foo;
foo.push_back(10);
foo.erase(foo.begin(), foo.begin());
IM_ASSERT(foo.size() == 1);
IM_ASSERT(foo[0] == 10);

 imgui.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imgui.h b/imgui.h
index 2204a86d5..a3e497cf7 100644
--- a/imgui.h
+++ b/imgui.h
@@ -1815,7 +1815,7 @@ struct ImVector
     inline void         pop_back()                          { IM_ASSERT(Size > 0); Size--; }
     inline void         push_front(const T& v)              { if (Size == 0) push_back(v); else insert(Data, v); }
     inline T*           erase(const T* it)                  { IM_ASSERT(it >= Data && it < Data + Size); const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + 1, ((size_t)Size - (size_t)off - 1) * sizeof(T)); Size--; return Data + off; }
-    inline T*           erase(const T* it, const T* it_last){ IM_ASSERT(it >= Data && it < Data + Size && it_last > it && it_last <= Data + Size); const ptrdiff_t count = it_last - it; const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + count, ((size_t)Size - (size_t)off - (size_t)count) * sizeof(T)); Size -= (int)count; return Data + off; }
+    inline T*           erase(const T* it, const T* it_last){ IM_ASSERT(it >= Data && it < Data + Size && it_last >= it && it_last <= Data + Size); const ptrdiff_t count = it_last - it; const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + count, ((size_t)Size - (size_t)off - (size_t)count) * sizeof(T)); Size -= (int)count; return Data + off; }
     inline T*           erase_unsorted(const T* it)         { IM_ASSERT(it >= Data && it < Data + Size);  const ptrdiff_t off = it - Data; if (it < Data + Size - 1) memcpy(Data + off, Data + Size - 1, sizeof(T)); Size--; return Data + off; }
     inline T*           insert(const T* it, const T& v)     { IM_ASSERT(it >= Data && it <= Data + Size); const ptrdiff_t off = it - Data; if (Size == Capacity) reserve(_grow_capacity(Size + 1)); if (off < (int)Size) memmove(Data + off + 1, Data + off, ((size_t)Size - (size_t)off) * sizeof(T)); memcpy(&Data[off], &v, sizeof(v)); Size++; return Data + off; }
     inline bool         contains(const T& v) const          { const T* data = Data;  const T* data_end = Data + Size; while (data < data_end) if (*data++ == v) return true; return false; }

[sigmareaver]

@PathogenDavid Ah, sorry. I'm using a modified version of One Lone Coder Pixel Game Engine. Although it is cross-platform, I'm only developing on Win32. OLC PGE is a single header, and for all intents and purposes can just be considered a Win32 wrapper. It does spawn a thread for the engine loop, but on Win32, the main thread simply blocks and waits for the spawned thread to rejoin it. At present I'm also using main() and not WinMain().

I've changed some preprocessor defines for both ImGui and my application. I'm going to see if that makes a difference. For some reason, the two projects had different ones, even though I'm fairly certain I set them both the same when I created them. The main application had _MBCS defined, and I didn't even know what that was before. I think Visual Studio must have added it in when some non-English characters got saved in a source file.

I will let you know if I get an assertion failure again. I'm still testing.

[sigmareaver]

I haven't been able to reproduce the behavior, so I have to believe it was due to either ImGui and my project being compiled with different flags, or because of MSVC's iterator debugging. In any case, I'm going to close this issue now.

@thedmd I don't believe that is related to my issue, which involved parameters being passed to erase() becoming invalid between stack frames.

Apologies for any inconvenience.

[sigmareaver]

Well, I guess I spoke too soon. I just had this issue happen again. It's just extremely intermittent. It can happen twice in a row, or I can go a day without it happening at all. That is going to make tracking this down extremely painful I fear. I do not see a way I can create a minimum working example that reproduces this behavior if it simply happens so randomly.

[thedmd]

It looks like it_last > it condition should be it_last >= it.

This code should not assert:

ImVector<int> foo;
foo.push_back(10);
foo.erase(foo.begin(), foo.begin());
IM_ASSERT(foo.size() == 1);
IM_ASSERT(foo[0] == 10);

 imgui.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imgui.h b/imgui.h
index 2204a86d5..a3e497cf7 100644
--- a/imgui.h
+++ b/imgui.h
@@ -1815,7 +1815,7 @@ struct ImVector
     inline void         pop_back()                          { IM_ASSERT(Size > 0); Size--; }
     inline void         push_front(const T& v)              { if (Size == 0) push_back(v); else insert(Data, v); }
     inline T*           erase(const T* it)                  { IM_ASSERT(it >= Data && it < Data + Size); const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + 1, ((size_t)Size - (size_t)off - 1) * sizeof(T)); Size--; return Data + off; }
-    inline T*           erase(const T* it, const T* it_last){ IM_ASSERT(it >= Data && it < Data + Size && it_last > it && it_last <= Data + Size); const ptrdiff_t count = it_last - it; const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + count, ((size_t)Size - (size_t)off - (size_t)count) * sizeof(T)); Size -= (int)count; return Data + off; }
+    inline T*           erase(const T* it, const T* it_last){ IM_ASSERT(it >= Data && it < Data + Size && it_last >= it && it_last <= Data + Size); const ptrdiff_t count = it_last - it; const ptrdiff_t off = it - Data; memmove(Data + off, Data + off + count, ((size_t)Size - (size_t)off - (size_t)count) * sizeof(T)); Size -= (int)count; return Data + off; }
     inline T*           erase_unsorted(const T* it)         { IM_ASSERT(it >= Data && it < Data + Size);  const ptrdiff_t off = it - Data; if (it < Data + Size - 1) memcpy(Data + off, Data + Size - 1, sizeof(T)); Size--; return Data + off; }
     inline T*           insert(const T* it, const T& v)     { IM_ASSERT(it >= Data && it <= Data + Size); const ptrdiff_t off = it - Data; if (Size == Capacity) reserve(_grow_capacity(Size + 1)); if (off < (int)Size) memmove(Data + off + 1, Data + off, ((size_t)Size - (size_t)off) * sizeof(T)); memcpy(&Data[off], &v, sizeof(v)); Size++; return Data + off; }
     inline bool         contains(const T& v) const          { const T* data = Data;  const T* data_end = Data + Size; while (data < data_end) if (*data++ == v) return true; return false; }

@ocornut Would you consider patching this assertion condition?

@sigmareaver Assertion is still incorrect.
You may hit it when no events were consumed in current frame, but are still queued. I'm not sure how to craft a repro. I would have to look closer at the code. It does look like fixing assertion should also solve your problem.
Or did you got it with patch I posted earlier applied?

[sigmareaver]

@thedmd No I did not. I actually currently have unrolled the IM_ASSERT macro so that I could put breakpoints inside. I could add this change and see if the assert still triggers.

That does appear to be a bug in ImGui though. As shown in the original screenshots, event_n was 0, which as you've shown is equal to the psuedo-code erase(begin(), begin()). The assert itself contains the condition it_last > it, which will fail. I do not know why event_n is 0 sometimes though. Is it important to erase only if event_n > 0? If so, a check should be added to UpdateInputEvents() instead?

[thedmd]

No, no. The point is call to erase when event_n == 0 should not assert. event_n == 0 is valid case.
Adding if (event_n > 0) in UpdateInputEvent() is workaround, does not fix the underlying issue.

I'm following the assumption that ImVector::erase API should not be surprising and should have same preconditions like std::vector::erase.

[ocornut]

Will fix, thanks both for looking into it.