[scribus-dev] Destruction of removed items

[Elvis Stansvik]

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