Repository navigation
Crash on object_pool_update line 874 when cleaning a node editor and then adding a node. #71
Description
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.
Activity
francesco-cattoglio commentedon Nov 30, 2020
I can also confirm this bug, happened to me just today.
francesco-cattoglio commentedon Nov 30, 2020
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 isnodes.in_use[i]. This will return false for the idx 0. The code reads the phantom data atpool[0]and setsprevious_id = 0, but now a node with that ID exists, it is the one just created!id_map.GetInt(0)returns 1,previous_idxis therefore set to 1 and hell breaks loose:previous_idx != -1therefore 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 commentedon Nov 30, 2020
@Nelarius there are a couple possible ways of fixing this. Setting the node id to -1 when destructing a
NodeDatacould 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-liteIf you want I can create a pull request for the fix.
GeTechG commentedon Dec 31, 2020
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.
Assign unused nodes with sentinel id. Workaround for #71.
Nelarius commentedon Jan 27, 2021
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.
Assign unused nodes with sentinel id. Workaround for Nelarius#71.