Crash in optimizeNode on MSVC after commit 323a7cb

[leksiso]

Commit 323a7cb seems to make crow crash at startup on MSVC. The crash happens at optimizeNode in routing.h. Possibly something wrong with std::move() on line 762?

[okaestne]

Can you give more details about the exact crash cause?

[okaestne]

What looks like a likely issue is that isSimpleNode() doesn't check that there is ANY child in children. That way, we might try to optimize away a non-existing first child.

Can you change the condition line 756 to if (node.children.size() == 1 && node.IsSimpleNode()) and test again?

[leksiso]

Can you give more details about the exact crash cause?

An exception seems to be thrown somewhere deep in the STL implementation (in a file called xmemory).

Here's a stack trace:

What I gather from that is that we move some vector (the children?) and the contents end up somehow being invalid.

Can you change the condition line 756 to if (node.children.size() == 1 && node.IsSimpleNode()) and test again?

This doesn't seem to fix it.

[leksiso]

I had a closer look at that. I think the problem has to be somewhat as follows:
When doing node.children = std::move(child_temp.children);,
the elements of node.children get destroyed/invalidated as the right-hand side of the assignment is about to replace them.
This invalidates child_temp whose children we try to move next, which causes the exception.

Maybe we should move the children to a temporary variable before moving them back to to the node:

auto children_temp = std::move(node.children);
auto& child_temp = children_temp[0];
node.key += child_temp.key;
node.rule_index = child_temp.rule_index;
node.blueprint_index = child_temp.blueprint_index;
node.children = std::move(child_temp.children);
optimizeNode(node);

This seems to fix the crash.

[okaestne]

Can you change the condition line 756 to if (node.children.size() == 1 && node.IsSimpleNode()) and test again?

This doesn't seem to fix it.

There I had a thinking error. The if clause before this one already ensures that there has to be at least one child.

I had a closer look at that. I think the problem has to be somewhat as follows: When doing node.children = std::move(child_temp.children);, the elements of node.children get destroyed/invalidated as the right-hand side of the assignment is about to replace them. This invalidates child_temp whose children we try to move next, which causes the exception.

Maybe we should move the children to a temporary variable before moving them back to to the node:

That makes sense to me and also explains why it worked before my change. Do you want to open a PR?