Drake: Destructor order in System2 I/O can lead to memory violations

[david-german-tri]

Consider a Diagram A containing two Diagrams B and C, where C depends on B. It is possible for the output ports in B to be deleted before the input ports in C, leading to access-of-freed-memory when C tries to deregister from B.

This only happens with Diagrams of Diagrams, and not with Diagrams of leaf Systems, because DiagramContext is careful to delete all the subsystem Contexts before any of the subsystem Outputs.

Now consider if B and C were in feedback. There would be no order of deletion that was immune to this problem! We either need a different approach to invalidation (and I'm not sure what it would be), or we need to add a deregistration phase to context destruction.

Coincidentally revealed by #3348, which changed the destruction order in a unit test.

[liangfok]

Can all input ports in a Diagram be deleted prior to deleting any of the output ports in a Diagram?

[liangfok]

Oh, does my comment above constitute a "deregistation phase" in "context destruction"?

[david-german-tri]

Yep, I think that's likely to be the solution. (Working now on a reproducing test case that doesn't depend on #3348.)

[david-german-tri]

Writing that test has _also_ revealed that I neglected to implement Diagram<T>::has_any_direct_feedthrough, so it's always true by default. Yay! Bugs!

[jwnimmer-tri]

Signal-slot destruction and deregistration of observers is a well-understood problem in many existing frameworks. Next week I walk through some of those samples, if it helps.

[david-german-tri]

Well, I could be wrong about the root cause. My repro test case doesn't actually repro. Stay tuned.

_UPDATE: silly mistake, now fixed to repro successfully_

[david-german-tri]

Also, @jwnimmer-tri, I'd be grateful for the input, but would rather not leave this hanging until next week. Any chance you can share a pointer today?

[sherm1]

What about not deregistering individual subsystems but rather treating the destruction of the top-level diagram as a single operation? I'm not sure there is a need to neatly remove a single subsystem from a diagram.

[david-german-tri]

@sherm1, that's what I had in mind earlier, but looking around in response to @jwnimmer-tri's nudge, I discovered that Qt just has the sender notify the receiver to disconnect.

https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qobject.cpp.html#942

That seems pretty easy to do in our case (most of the complexity in QObject is threading-related AFAICT) and would solve the problem, so I'm going to give it a try.