Crash on object_pool_update line 874 when cleaning a node editor and then adding a node.

[Tackwin]

Hi,

I have a crash that I can reproduce in 3 frames. First you need to draw two nodes in a editor. Then draw none and finally draw one. The application will assert on line 874 in the function object_pool_update.

Here is a code exemple:

                ImGui::Begin("test");
        imnodes::BeginNodeEditor();
        if (i == 0) { // frame 1

            imnodes::BeginNode(0);
            imnodes::EndNode();
            imnodes::BeginNode(1);
            imnodes::EndNode();

            i++;
        } else if (i == 1) { // frame 2

            i++;
        } else if (i == 2) { // frame 3

            imnodes::BeginNode(0);
            imnodes::EndNode();
            i++;
        }
        imnodes::EndNodeEditor();
        ImGui::End();

You don't need anything more than that. The crash doesn't happen if on frame 3 the node use another id so it is maybe an intended crash ? If that's the case I still think that at least the error reporting is sub-par but still I don't see why this couldn't work.

I do admit that I'm not familiar with imnodes so I might be missing something, I hope that you have all the informations you need.

[francesco-cattoglio]

I can also confirm this bug, happened to me just today.

[francesco-cattoglio]

I think I figured out what happens, and for this bug to fire one needs to do a two things:

First, delete TWO nodes one after the other. The free_list of nodes will get updated to mark 2 idx as unused. Lets say both 0 and 1 are on the free list. When deleting two nodes, during void object_pool_update(ObjectPool<NodeData>& nodes) a destructor gets called at the end of the big else branch: (nodes.pool.Data + i)->~NodeData(); but this destructor does nothing: the node id contained at the memory location nodes.pool[idx] is still the same: 0 and 1.

Second, create a new node with an unlucky ID. When we create a new node, a new node gets allocated in the pool from the free list, and the ID that the user chose is used to update the id_map. If the idx returned by the free list is 1 and the ID chosen by user is 0, now the pool data has two entries, both containing 0 as ID. The first one is considered dead but the data is still there. On the next run of void object_pool_update(ObjectPool<NodeData>& nodes) the first check is nodes.in_use[i]. This will return false for the idx 0. The code reads the phantom data at pool[0] and sets previous_id = 0, but now a node with that ID exists, it is the one just created! id_map.GetInt(0) returns 1, previous_idx is therefore set to 1 and hell breaks loose: previous_idx != -1 therefore we try to delete the element with idx 0, which was deleted already from the depth_stack, and the assert at line 847 fails.

[francesco-cattoglio]

@Nelarius there are a couple possible ways of fixing this. Setting the node id to -1 when destructing a NodeData could be a solution. An alternative could be having something akin to an std::optional, and store a pool of options instead of NodeData, so that "double delete" bugs are easier to spot. One lib that comes to my mind to have options in C++98 is https://github.com/martinmoene/optional-lite If you want I can create a pull request for the fix.

[GeTechG]

I also have a similar problem, I noticed it if I delete both nodes at the same time, so that leaves 0. UPD, My problem is that assert works. Can someone help? as I am sitting idle because of this problem, thank you.

[Nelarius]

Thanks for bringing the issue up with a simple repro! And sorry that it took so long to respond.

Indeed, the repro case should be a perfectly valid way to use imnodes. @francesco-cattoglio 's comment is right about the cause, and I pushed a commit with the sentinel node id to master. 👍 It's not a real fix, however, because I originally intended all ints to be valid ids, and now one of them is a reserved.

Reviewing the code in the object pool now, I realized that I've made a number of... rather questionable choices 😅 So I opened the tracking issue #81 which should be the real way to solve this particular crash. I need to rethink how some of the internals should work.