Access violation in the serialization of a flight plan trajectory

[eggrobin]

Read from location 000000c1 caused an access violation.

This happened while journalling (non-verbose), so I have at least part of a journal.
This is below the current master (comparison), but note that this is not an instance of #872, as it is above the fix.

See also the logs.

Decoded stack trace

physics/forkable_body.hpp:258
ksp_plugin/vessel_body.hpp:189
ksp_plugin/plugin.cpp:632
ksp_plugin/interface.cpp:814

[eggrobin]

Post-mortem

1. The fix to Crash reported by maccollo #872, namely Do not forget the past of histories if that would destroy flight plans #879, was incorrect (max instead of min).
2. The test was incorrect: the bug occurs when histories are forgotten after the beginning of the flight plan. The test called plugin_->ForgetAllHistoriesBefore(HistoryTime(sync_time, 1)) whereas the flight plan had an initial time of HistoryTime(sync_time, 4). It is thus likely that the test would not have failed, even in the absence of the fix.
3. The fail-safe that was supposed to ensure that an accidental destruction of the trajectories of the flight plan would cause a FATAL failure, rather than a use-after-free, was incorrect: the on_destroy callback was not reinstated on deserialization. In the current master (3bc0a40), the callback is added in
   three distinct places.

The first item is an ordinary programming error.

The second item above is a failure of TDD: for a bug fix, even if the test is, for convenience, written after the fix, it should always be written or cherry-picked onto a commit pre-dating the fix, so that it can be seen that it exercises the bug.

The third item points to a design flaw in FlightPlan; the fail-safe callback needs to be managed in several places, and while the deserialization could be factored by having two separate constructors, one delegating to the other, it is harder to see how the remaining use (in CoastIfReachesManœuvreInitialTime) could be factored. Additionally, while it is possible to factor the registration of those callbacks post hoc, there is no check that they are not mistakenly removed as was the case here in the deserialization.
It shall be noted that #895, if it were to be merged, would fully address this item.