Re: Review for a lists patch

On Sat, Nov 21, 2009 at 2:46 AM, J.M. Maurer <uwog@uwog.net> wrote:
> Say you have some hypothetical list definitions like this:
>
> <lists>
>    <l id="1001"/>
>    <l id="1002" parentid="1001"/>
>    <l id="1003" parentid="1001"/>
> </lists>
>
> And also say you never ever use list 1001 in your document. This is
> silly of course, but in theory it could be valid. AbiWord itself will
> never write that out, but I actually came across generated documents
> like that :)
>
> Abiword will crash on this construct (valgrind trace attached):
>
> 1. PD_Document::updateDirtyLists will be called
> 2. It will see the first list is empty:
>
>       if(pAutoNum->isEmpty())
>
>   and it will delete it.
> 3. Then it will updat the other dirty lists a few lines below:
>
>       pAutoNum->update(0);
>
> => This will crash in fl_AutoNum::update() on the following line:
>
>       if (m_pParent && !m_pParent->isUpdating())
>

A better solution would be to remove the m_pParent pointer and instead
record the
parent ID in fl_AutoNum. Call it m_iParentID.

Then whenever need the parent fl_AutoNum from the looking up the
parent ID in the Document.

fl_AutoNum * pParent = m_pDoc->getListByID(m_iParentID);

We could wrap all this into a const method

fl_AutoNum::getParent(void) const
{
   return m_pDoc->getListByID(m_iParentID);
}

so there is hardly any more typing needed to get a parent pointer.

It was a mistake to hold pointers to unowned transient classes.

What do you think?

Cheers

Martin

>   as this list's parent list has just been deleted.
>
> The attached proposed patch fixes this by simply not deleting the list,
> and not updating a list if it is empty. Since I'm always scared touching
> list code, I'd appreciate a review ;)
>
> Cheers!
>  Marc
>
> Note: Sometimes AbiWord will NOT crash on this and actually do what you
> expect (valgrind will complain loudly though: ). The attached patch will
> then look like a regression, since you will now see something like
> ".6." (the parent has no list number, since it is empty). I find this
> behavior way better than crashing, and the "regression" can be easily
> fixed by simple setting the parentid to 0.
>
>
Received on Sat Nov 21 01:51:20 2009