A very common pattern in our synthetic child providers was to make the
child ValueObject using ValueObjectConstResult::Create or some form of
the static ValueObject::CreateValueObjectFrom*** methods, and store and
hand that out as the child. Doing that creates a "root" ValueObject
whose lifecycle is not linked to the lifecycle of the ValueObject it is
a child of. And that means it is possible that either the child or the
parent could have gotten destroyed when the other ValueObject gets asked
a question about it.
For the most part this doesn't happen because there are usually enough
other shared pointer references binding the two to keep both sides
alive. But we have gotten a small but steady stream of reports for years
now of crashes where a ValueObject accesses its ClusterManager but that
has already been deleted. I've never been able to find a reproducible
case of this, but one plausible cause is that we are violating the
contract that "all the children of a ValueObject have coterminous
lifespans, enforced by the ClusterManager". So it is unsurprising that
they might sometimes not stay alive as long as they should.
This patch addresses that by providing a way to use these static create
methods but passing in the ClusterManager to be used, and adds or
modifies where extant the CreateChildValueObjectFrom*** methods to pass
in the parent manager.
This patch is not complete. It turns out that routing the ClusterManager
from CreateValueObjectFromExpression all the way to where the expression
result VO is made is quite intrusive, and would have made this already
quite large (though largly mechanical) patch become unweildy. Since I am
mostly concerned about synthetic child providers, and we discourage
using expressions for them, I think that's an acceptable separation.
I also added a test where the ValueObjectSynthetic hands out its
children, checking whether the child being handed out is in fact owned
by the parent handing it out. It is controlled by a setting
(target.check-vo-ownership) which is currently off by default - since I
can't provide a way to do this correctly for expressions yet. But I turn
it on in the testsuite. That's the real testing strategy for this patch,
since its goal is to ensure that all the children we hand out are
managed by their parents. I didn't put an equivalent check in
ValueObjectVariable because that system really doesn't allow a way to do
this incorrectly.
I also fixed some of the tests that were making children using
CreateValueObjectFromExpression to make their children with
CreateValueObjectFromData instead. I could have opted the tests out of
the check, but using the expression parser to do this work is not best
practice, so I didn't want to leave bad examples around. I left one case
that does do it with expressions, however, so we still have a test for
that not-recommended but people do it anyway path.
2c7b820bc8