b2World::Step crashes in some cases after SetPosition and DestroyBody are called

[JakMobius]

The following code crashes at b2_dynamic_tree:47:

import {b2Body, b2BodyDef, b2BodyType} from "./box2d/dynamics/b2_body";
import {b2FixtureDef} from "./box2d/dynamics/b2_fixture";
import {b2PolygonShape} from "./box2d/collision/b2_polygon_shape";
import {b2Vec2} from "./box2d/common/b2_math";
import {b2World} from "./box2d/dynamics/b2_world";

const world = new b2World({x: 0, y: 0})

const bodies: b2Body[] = [];
{
    const bd: b2BodyDef = new b2BodyDef();
    bd.type = b2BodyType.b2_dynamicBody;

    bodies[0] = world.CreateBody(bd);

    const fd: b2FixtureDef = new b2FixtureDef();
    const shape: b2PolygonShape = new b2PolygonShape();
    const vs: b2Vec2[] = [];

    vs[0] = new b2Vec2(-0.33, -1);
    vs[1] = new b2Vec2(0.33, -1);
    vs[2] = new b2Vec2(0.33, 1);
    vs[3] = new b2Vec2(-0.33, 1);
    shape.Set(vs, 4);

    fd.shape = shape;

    bodies[0].CreateFixture(fd);
}
{
    const bd: b2BodyDef = new b2BodyDef();
    bd.type = b2BodyType.b2_staticBody;
    bd.position.Set(320, 640);

    bodies[1] = world.CreateBody(bd);

    const fd: b2FixtureDef = new b2FixtureDef();
    const shape: b2PolygonShape = new b2PolygonShape();
    const vs: b2Vec2[] = [];

    vs[0] = new b2Vec2(20, 20);
    vs[1] = new b2Vec2(20, 320);
    vs[2] = new b2Vec2(0, 320);
    vs[3] = new b2Vec2(0, 20);
    shape.Set(vs, 4);

    fd.shape = shape;

    bodies[1].CreateFixture(fd);
}

const body = bodies[0]
world.Step(0.002, 1, 1)

body.SetPositionXY(260.58118606916815, 935.4530055878474)
body.SetPositionXY(338.1343566894531, 932.1221297264099)

world.DestroyBody(body)
world.Step(0.002, 1, 1)

The stack trace:

    at b2TreeNode.get userData [as userData] (box2d/collision/b2_dynamic_tree.js:47:19)
    at b2BroadPhase.UpdatePairs (box2d/collision/b2_broad_phase.js:142:50)
    at b2ContactManager.FindNewContacts (box2d/dynamics/b2_contact_manager.js:115:27)
    at b2World.Step (box2d/dynamics/b2_world.js:373:35)

The issue seems to be in b2BroadPhase::BufferMove. When body get moved twice, its fixture proxies get duplicated in m_moveBuffer. When body is removed from the world, b2BroadPhase::UnBufferMove is called to remove those proxies from the m_moveBuffer. However, this method does not remove their duplicates. So, when UpdatePairs is called, they get processed as moved proxies even after their removal from the world, which causes null-check to fail.

This is not always the case. My game receives body move and destroy events from the server. It get crashed eventually, after several bullets were fired and removed from the world.

My suggested fix: Use ES6 Set for m_moveBuffer. Thus, no duplicates could be stored in this buffer. As far as I understand, b2BroadPhase::MoveProxy method may be called a lot of times for single proxy (Its documentation says that it can be called as many times as needed). So this approach will likely improve overall performance, preventing same proxy pairs from being processed multiple times. (Also, as b2_broad_phase:121 line says, duplicate pairs are avoided, so this can lead to additional problems)

[flyover]

Thanks for reporting this; it should be fixed now. I didn't use the pull request because I've had performance issues iterating Map/Set/etc. The fix acts more like the upstream box2d code.

[JakMobius]

So duplicate proxy pairs are OK? Isn't it a bottleneck? While debugging that, i've often seen that m_moveCount become very large sometimes, even on simple world setups. I'm actually thinking about making some benchmarks with and w/o Set.