[scribus-dev] Destruction of removed items

Elvis Stansvik elvstone at gmail.com
Mon Aug 15 12:56:49 UTC 2011


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
>>
>



More information about the scribus-dev mailing list