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

[David Faure]

On Friday 13 December 2013 11:28:01 Frank Reininghaus wrote:
> 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?

Right.

I think the parent for the actions should be the KFileItemActions instance 
instead. That would make all use cases happy, and follow the principle of 
least surprise, right?

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE, in particular KDE Frameworks 5