Re: [Flightgear-devel] Segmentation fault acquiring SGPropertyLock

[Florent R. <f.r...@fr...>]

Hi,

Julian Smith <ju...@op...> wrote:

> This diff seems to fix things for me:

Many thanks, same here!

> The problem was that tiedProperties.Tie() was calling
> FGProperties::setFreeze(true) before it returned, but we were only setting
> _simFreezeNode *after* tiedProperties.Tie() had returned.

Indeed, TiedPropertyList::Tie() calls SGPropertyNode::tie()
(simgear/simgear/props/props.cxx:4217) which, when the “useDefault” arg
is true which is the case here, does setValue(exclusive, old_val) which,
in the case at hand, calls the setter method FGProperties::setFreeze().
As you explained, this executes _simFreezeNode->fireValueChanged() but
_simFreezeNode hasn't been set at this point yet.

> tiedProperties.Tie() simply returns the node that it was given as its
> first arg, so the diff simply sets _simFreezeNode to the value it would
> have taken later, before calling tiedProperties.Tie().

> Please note that it's entirely possible that i've missed something
> subtle here. I haven't considered what's going on in any great detail,
> and i've never used tied properties myself.

I've read the code of SGRawValueMethods, TiedPropertyList and
SGPropertyNode::tie() to understand this but that's about what I know on
the subject matter — and nothing about the freeze process per se. Your
analysis looks good to me and your patch fixes the crash I reported
yesterday.

Maybe James will want to double-check, but since he seems to be busy
with RL, given the non-invasiveness of your patch and our understanding
of this issue, I believe it's pretty safe to commit.

Thanks & regards

P.S.: your patch (the attachment) had all explanations plus your
      signature inside (yeah, I know the patch tool tolerates
      “trailing, uh... garbage”). :)

-- 
Florent