[Kde-pim] Review Request 121017: don't update KMMainWidget's folderMenu when shutting down
René J.V. Bertin
rjvbertin at gmail.com
Fri Nov 7 16:11:36 GMT 2014
> On Nov. 7, 2014, 7:42 a.m., Laurent Montel wrote:
> > Perhaps there is an other method that add it for each call no ?
>
> René J.V. Bertin wrote:
> Other than a manual method where someone does if and when a crash has identified an unprotected call, I really wouldn't know how to do that. Mopping with the tap open, as we say in the Netherlands.
>
> Someone who knows KMail's event handling skeleton well (as well as undoubtedly of the other PIM components) should be able to add those few lines to the relevant SLOT functions quite easily.
>
> I've checked `KJob::exec`: it calls a QEventLoop `eventLoop->exec(QEventLoop::ExcludeUserInputEvents)` which means that if we get those UI events in error, it's an upstream error in Qt. I've asked on Qt's interest ML about that.
>
> René J.V. Bertin wrote:
> Oh, and in the meantime I haven't yet had the occasion to check whether this patch does its work... (not that there's any reason to expect it wouldn't, except if `kmkernel` evaluates to a NULL pointer)
>
> René J.V. Bertin wrote:
> Here's a first reply to my query on the Qt interest ML, raising some valid points that I don't think are up to me to answer:
>
> On Friday November 07 2014 12:44:27 Andreas Pakulat wrote:
>
> >> I'm using KDE's KMail on Linux and Mac OS X, and I've been dealing with
> >> crashes on both platform when UI events "come through" during the shutdown
> >> phase.
> ...
> >> client code, but reality is clearly different; see for instance
> >> https://bugs.kde.org/show_bug.cgi?id=340680 for a backtrace.
> >>
> >
> >Looking at that backtrace I don't see UserInput events. At least
> >QTimerInfoList::activateTimers suggests a QTimer fires, thats not a
> >UserInput nor a UserInterface event.
> ...
> >> What am I missing (or rather, what have the people maintaining KJob missed
> >> :) )?
> >>
> >
> >I'd say at least the above backtrace is an application bug. I can't say
> >wether the timer that triggers the update of the folder menu should have
> >been stopped already or just the FolderTreeWidget should double-check its
> >(selection)model instead of assuming its always ok. To me this looks like
> >the shutdown order of things in this application is not properly defined,
> >i.e. something deletes the selection model and/or the itemmodel but does
> >not clean up other things connected to that.
> >
> >I'd also question the usage of KJob::exec (I assume thats where the
> >processEvents comes from, as everything else in the API does not sound like
> >it would need that). Whats the point of putting things into a KJob if you
> >then essentially do a synchronous run? Just put the functionality thats
> >inside the job's run function into a utility function and call that
> >directly. Or does that cleanup really need a Qt event loop running?
> >
> >Andreas
On Friday November 07 2014, Andreas Pakulat wrote regarding "Re: [Interest] [Qt4.8] UI events coming through despite QEventLoop::ExcludeUserInputEvents"
>> On Friday November 07 2014 14:16:07 Andreas Pakulat wrote:
>> > even if the dbus message is triggered through a user event (like clicking
>> > in the menubar or so) thats not possible for Qt to see anymore since a
>> dbus
>> > event may also come from a commandline app running in the background or
>>
>> Sure, but isn't the outgoing event a UI event, or should I say, isn't the
>> event being treated as if it were a user input event?
>>
>
>Let me clarify something since you're always talking about UI events, but
>thats not what the processEvents call does. UI events at least in my books
>are any events related to the user interface. This includes interaction
>events like clicks, keyboard input etc. but also includes redraw, resize or
>move events. The latter can afaik not be stopped from being delivered. What
>can be 'delayed' is the delivery of keyboard or mouse events by using
>ExcludeUserInput. However this only affects events in the Qt application
>that you're looking at, but due to the usage of libdbusmenu-qt some 'parts'
>of your application (in particular the application menu bar and apparently
>also the systray and its menu) are not living in your application process
>but are in another process. So you can't block the user input events
>triggering menu items in that separate process. You can of course ensure
>that your menu actions are disconnected before cleaning up, or at least
>most of them are disconnected in case there are some still working. Since
>the separate process needs a way to trigger the original action's connected
>slots when the menu in the separate process is being triggered it uses Qt's
>metaobject system to call the slots directly when the corresponding IPC
>call is received in your application. Stopping that IPC call from happening
>would require ExcluseSocketNotifiers I guess in QApplication::processEvents
>and would shut down all sockets the app may use. Might be not a good idea
>for KMail if it talks to IMAP...
>
>> something else. So if libdbusmenu-qt just invoke's qactions or their
>> > connected slots directly when the user selects something in the menu (I'm
>> > assuming this is the hack that (K)Ubuntu uses since some releases to get
>> a
>> > cross-application menubar like on MacOSX) instead of posting a user-input
>> > event to the event loop you could argue they're doing that wrong.
>>
>> Who's they here? Sounds like the libdbusmenu-qt devs?
>>
>
>Yes, the libdbusmenu-qt devs should probably consider to emulate userinput
>events instead of invoking code directly to avoid breaking assumptions such
>as 'my menu bar runs in the same process as my main application'.
>
>> One could
>> > also argue that the menu shouldn't be accessible anymore during this
>> > 'shutdown' procedure...
>>
>> Yes, that's what the fix does. It was discussed to remove the systray icon
>> (the "source" of the event) early when shutting down, but I voted against
>> that because part of its context menu remains functional and valid (and
>> useful, as the last "visual handle" allowing to quit the app if it gets
>> stuck too long in the cleanup code).
>>
>
>Oh, systray is using that too? Ok, wasn't aware of that. Another option for
>solving the 'useful' part you mention would be to let a timer fire after a
>'reasonable timeout' and ask the user wether KMail should be killed since
>it seems to be hanging in its cleanup phase.
>
>Andreas
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121017/#review69963
-----------------------------------------------------------
On Nov. 6, 2014, 8:19 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121017/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2014, 8:19 p.m.)
>
>
> Review request for KDEPIM.
>
>
> Repository: kdepim
>
>
> Description
> -------
>
> Under certain conditions UI events come through when shutting down Kontact or KMail, for instance during `jobStatistics->exec()` in `KMKernel::cleanup`.
> When that leads to an attempted update of the folder menu, the process will crash because it accesses already released resources.
> See bug report https://bugs.kde.org/show_bug.cgi?id=340680
>
> This patch prevents the illegal access highlighted in the above bugreport.
>
>
> Diffs
> -----
>
> kmail/kmmainwidget.cpp 8296b4a
>
> Diff: https://git.reviewboard.kde.org/r/121017/diff/
>
>
> Testing
> -------
>
> On Kubuntu 14.04 with kdelibs 4.14.2 and kdepim git/4.14.3 .
> On OS X Mac OS 10.6.8 with kdelibs git/4.14 and kdepim 4.14.2 .
>
>
> Thanks,
>
> René J.V. Bertin
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list