[scribus-dev] Destruction of removed items

[Elvis Stansvik]

So after some discussion with Craig on IRC we arrived at the attached
fix, which could possibly be an OK solution to the problem.

The patch makes sure that StyleContext::invalidate() and the resulting
update() is never called during destruction of a StyleSet. It does so
by introducing a new bool argument to StyleSet::clear() to control
this. It doesn't really make sense to notify listeners when the entire
style set is actually being destroyed, does it?

Also, to avoid a crash when trying to disconnect from a destroyed
object, the table no longer disconnects from the style contexts in its
destructor. As a side note, this last problem has apparently been hit
before in the story text code, which also connects to the
document-wide style contexts. It has this outcommented snippet in its
destructor:

StoryText::~StoryText()
{
/* Code below is not needed imho, as all connections will be
disconnected automatically
	by the QObject destructor. At least removing that code fixes a crash
when closing
	documents */
/*	if (doc)
	{
		doc->paragraphStyles().disconnect(this, SLOT(invalidateAll()));
		doc->charStyles().disconnect(this, SLOT(invalidateAll()));
	} */
...

Anyway, the attached patch fixes the crash and is quite minimal. Is it
a good enough fix though?

What do you think avox?

Cheers,
Elvis

2011/8/15 Elvis Stansvik <elvstone at gmail.com>:
> An ugly fix for this problem is attached, but this is a hack at best
> and I don't favor this solution... It could have side effects.
>
> It simply makes the doc keep track of deleted objects and deletes them
> when the document is destroyed.
>
> I'd love some input on how you would like this to be handled. In my
> mind it really is the undo states currently on the undo stack that are
> the owners of the deleted objects, since they are the only ones
> holding pointers to them. So on application exit, I would think it
> would be up to them to delete it.
>
> Craig said he wasn't able to reproduce, but he had some local
> modifications and had to go to work before trying with a clean copy of
> my repo.
>
> If anyone else has a recent copy of my repo, could you please verify that
>
> 1) Start app.
> 2) Create a table.
> 3) Delete the table.
> 4) Quit app.
>
> will crash the application?
>
> Would be much appreciated!
>
> Elvis
>
> 2011/8/15 Elvis Stansvik <elvstone at gmail.com>:
>> Good morning. A more thorough analysis and possible solution of this
>> problem follows.
>>
>> 2011/8/14 Elvis Stansvik <elvstone at gmail.com>:
>>> Hi again,
>>>
>>> The tables listens to changes in the document-wide cell and table
>>> style contexts. They do this by connecting to them like this:
>>>
>>> // Listen to changes in the document-wide cell/table style contexts.
>>> m_Doc->tableStyles().connect(this, SLOT(handleStyleChanged()));
>>> m_Doc->cellStyles().connect(this, SLOT(handleStyleChanged()));
>>>
>>> during construction, and disconnecting like this:
>>>
>>> m_Doc->tableStyles().disconnect(this, SLOT(handleStyleChanged()));
>>> m_Doc->cellStyles().disconnect(this, SLOT(handleStyleChanged()));
>>>
>>> on destruction.
>>>
>>> In the handleStyleChanged() handler I perform things like updating the
>>> cell text frames. This involves accessing the cells and their styles
>>> (to get border/padding width).
>>>
>>> Now, this wouldn't be a problem if it wasn't for the fact that a table
>>> which has been deleted by selecting it and pressing Delete seems to
>>> receive these signals on application exit. And when that happens, it
>>> seems that the internal style objects used by the cells have been
>>> destroyed (I'm guessing maybe by the style context themselves)? At
>>> least that's my understanding. I'll have to do some debugging.
>>>
>>> This means that currently we get a crash if tables have been deleted
>>> and the application is exited.
>>
>> The following happens when a user creates a table, then deletes it,
>> then exits the application:
>>
>> 1) The table is constructed. On construction, it connects to the
>> StyleContexts for cell/table styles on the document to listen for
>> changes in them.
>> 2) The table is removed. This means that it is removed from the list
>> of items on the document and then put in a QList<PageItem*> of deleted
>> items, which is then put in a ItemState<QList<PageItem*>> which is
>> finally put into the undo manager. Note that the destructor of the
>> table never runs (Naturally! Otherwise undo/redo wouldn't work).
>> 3) The application exits. Now things happen in the following order:
>> 4) ScribusMainWindow::DoFileClose() runs.
>> 5) UndoManager::removeStack(...) is called for the undo stack of the document.
>> 6) The ItemState created in step 2 is destroyed, but the items in its
>> list of deleted items are _not_ destroyed (their destructors never
>> run). All references/pointers to these deleted items are now gone;
>> they have been leaked. The table object is still alive though, and
>> still listening to changes in the document-wide style contexts.
>> 7) Now the destructor of the document runs. First the body of its
>> destructor will run. This will delete all the items in the document
>> (the table is not among them, so its destructor will not run). After
>> that, destructors of non-static member are called in the reverse order
>> of construction, this means that the document-wide style contexts are
>> destroyed.
>> 8) During destruction, all styles in the style context will be
>> destroyed and then the invalidate() function of the style contexts
>> will be called, which means that Observable::update() will be called.
>> 9) The removed table will be notified of this change in the style
>> context, and it's handler for this signal will be called. This handler
>> will try to access the styles of the cells to get their border/padding
>> width.
>> 10) A crash occurs since it is trying to access a destroyed style.
>>
>> In my opinion, the correct fix for this would be for the UndoState
>> that handles item deletion to take care of deleting the items which it
>> is managing pointers to. It is the one responsible for them after they
>> have been put there. I think this might require a new ItemState class
>> specialized for item deletion.
>>
>> Do you agree? Should I try to make a fix for it this way?
>>
>> Elvis
>>
>