KFileItemActions - why does it remember the actions it created, and deletes them?

[Frank Reininghaus]

Hi,

2013/12/13 Frank Reininghaus:
> Hi,
>
> thanks David for the detailed reply, and sorry for not reading the
> documentation before asking stupid questions ;-)
>
> 2013/12/12 David Faure:
>> On Thursday 12 December 2013 18:38:23 Frank Reininghaus wrote:
>>> 1. In the openItemContextMenu() member of DolphinContextMenu, which
>>> inherits KMenu, we create a KFileItemActions instance on the stack,
>>> and use it to add actions to "this". The actions thus become children
>>> of "this".
>>
>> Let me quote from the KFileItemActions documentation:
>>
>>     /**
>>      * Set the parent widget for any dialogs being shown.
>>      *
>>      * This should normally be your mainwindow, not a popup menu,
>>      * so that it still exists even after the popup is closed
>>      * (e.g. error message from KRun) and so that QAction::setStatusTip
>>      * can find a statusbar, too.
>>      */
>>     void setParentWidget(QWidget* widget);
>>
>> *not a popup menu* :-)
>>
>> The documentation is incomplete though - what it forgets to say (oops), is
>> that this parent widget will also be the parent of the actions.
>>
>> I can't remember why it works this way, but at least I think you have an easy
>> solution: call setParentWidget(mainwindow).
>
> Actually, we do call
>
> fileItemActions.setParentWidget(m_mainWindow);
>
> already in DolphinContextMenu::addServiceActions(KFileItemActions&
> fileItemActions).
>
> Hm. The user said that he closed the Dolphin window by clicking the
> "X" in the top-right corner of the window. So it seems that both the
> main window and its context menu child somehow were deleted inside the
> menu's exec() method with the nested event loop. Now I see two ways to
> explain the crash:
>
> 1. The mainwindow, which is the owner of the QActions created by the
> KFIleItemActions instance, somehow gets deleted, and thus deletes its
> children, including the actions. Therefore, the KFileItemActions
> destructor tries to delete dangling pointers (I see nothing in
> kfileitemactions.cpp that prevents appending the created actions to
> m_ownActions, and deleting them in the destructor, if a parent widget
> has been set?).
>
> 2. Another problem might be that after returning from the event loop,
> we are inside DolphinContextMenu::openItemContextMenu(), the method of
> a deleted object. Therefore, accessing local variables of that
> function, living on the stack, might not be really safe, right? Just
> calling KFileItemAction's destructor might be enough to cause trouble
> then. I think that this could be resolved by allocating the
> KFileItemActions instance on the heap and setting the context menu as
> the parent.
>
> Maybe I'll try to do some experiments. If I could only reproduce the crash...

quick update: I can reproduce the crash now, see

https://bugs.kde.org/show_bug.cgi?id=259089#c21

Closing Dolphin with a 5 second delay using

sleep 5; qdbus `qdbus | grep dolphin`
/dolphin/Dolphin_1/actions/file_quit trigger

and then opening the context menu did the trick.

It seems that solution 2 (allocating the KFileItemActions on the heap
with the context menu as the parent) solves the problem.

BTW, the DolphinPart in Konqueror, which uses KonqPopupMenu, does not
suffer from this problem, because the KFileItemActions instance is a
member of KonqPopupMenuPrivate (which is destructed before we return
from the nested event loop when trying to reproduce the bug with the
"quit via D-Bus" method).

Still, I'm wondering if KFileItemActions should really delete all
actions it created, even if they belong to a widget that has been set
with setParentWidget(QWidget*). It works fine if KFileItemActions is
destroyed *before* the widget, because parent QObjects listen to the
'destroyed' signal, but if the destruction order is different for some
reason, then we might get a crash because we try to delete dangling
pointers, right?

Best regards,
Frank