On 05/13/15 21:34, Georg Baum wrote:
Hi,
after a lot of debugging I understand now what happens in
http://www.lyx.org/trac/ticket/9490. As you may recall, the last math macro
crash I fixed (9418) was caused by a broken ArgumentProxy::mathMacro_
pointer. At that time, I assumed that the only place where an ArgumentProxy
can occur is as an element of MathMacro::d->expanded_ (which is also
indicated by the comment in ArgumentProxy::draw(). Under this assumption,
the fix for 9418 would have fixed all cases of invalid mathMacro_ pointers.
However, this assumption is not true. In MathData::detachMacroParameters(),
elements of MathMacro::d->expanded_ can be extracted and put into the same
vector as the macro itself. Unfortunately, if you insert an element into a
vector, the existing elements may be copied => pointers may become invalid.
The fix for 9419 does not work for this case, since the ArgumentProxy
instances in question are already extracted from the macro and stored in the
local variable detachedArgs.
It would be relatively easy to do a local fix in
MathData::detachMacroParameters(). However, since the detached ArgumentProxy
instances stay alive after leaving MathData::detachMacroParameters(), this
is not enough.
I tried to keep a record of each ArgumentProxy child in a new member of the
MathMacro itself. Then ArgumentProxy::setOwner() can be called reliably
whenever a copy of a MathMacro is made. This works fine, but exposes another
problem: When following the recipe of bug 9490, a MathMacro instance is
deleted in Undo::Private::doTextUndoOrRedo() while an ArgumentProxy pointing
to it is still alive.
What to do now? I don't know. I see the following options:
- Change the base class of MathData to RandomAccessList as proposed by Abdel
in the discussion of 9419. This would solve the invalid pointer problems,
but not the problem showed by the undo stack, and I fear that more hidden
problems like this one exist. To me it is now more clear than during the
discussion of 9418 that the "self-contained" property of a MathInset is
really really important and assumed implicitly all over the place. The
ArgumentProxy is not self-contained, and this is the root of all evil.
- Get rid of MathData::detachMacroParameters(). This would be a feature
regression: Unfolding a math macro would no longer be possible. Is this an
important feature?
- Rewrite ArgumentProxy to get rid of the mathMacro_ pointer (or even get
rid of the class ArgumentProxy). This is a big task.
Any other ideas? Preferences? Unless somebody has a brilliant idea I tend
towards the second item, since the first does not solve all problems, and
the last is too much work for me ATM.
Georg
Dear Georg,
Thank you for looking at these bugs. I reported 9418 and 9490, and I use
macro unfolding quite often. Before discovering this feature, I would
occasionally edit the lyx file externally (!) to change certain macros.
Given that the original bug was about a crash occurring during
unfolding, the second solution is a bit like curing a pain by amputation...
I would really appreciate a long-term solution to the math macro bugs.
It would be useful to know whether many other people had segmentation
faults because of macros, because I suspect that these crashes have been
under-reported due to how hard they were to reproduce.
Thank you again for the hard work.
Best,
Guillaume
