[Kde-graphics-devel] Review Request 120920: ksnapshot: Fill SendTo menu async to fix Bug 312495

Sat Dec 6 17:13:13 UTC 2014

> On Dec. 4, 2014, 11:02 p.m., Kevin Funk wrote:
> > Two problems:
> > a) Doesn't compile if KIPI_FOUND is FALSE
> > b) When running: ASSERT failure in QCoreApplication::sendEvent: "Cannot send events to objects owned by a different thread. Current thread 2550200. Receiver '' (of type 'KSnapshotSendToActions') was created in thread 23e82d0", file /home/krf/devel/src/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp, line 514
> 
> Dominik Haumann wrote:
>     It seems to be a tougher issues than though: when creating the plugins in the thread, the Kipi PluginManager also wants to set add the plugin's XmlGuiClient to the GuiFactory. This is not a good idea in the thread at all, and currently leads to a crash like this:
>     
>     [KCrash Handler]
>     #5  0x00007f416c81ba6c in QScopedPointer<QObjectData, QScopedPointerDeleter<QObjectData> >::operator-> (this=0x8) at /home/dh/kde/kf5/src/qt5/qtbase/src/corelib/tools/qscopedpointer.h:120
>     #6  0x00007f416c81b6d4 in QObject::parent (this=0x0) at /home/dh/kde/kf5/src/qt5/qtbase/src/corelib/kernel/qobject.h:425
>     #7  0x00007f416c81b37a in KIPI::Plugin::setupXML (this=0x7f4148025a30) at /home/dh/kde/kf5/src/kde/kdegraphics/libs/libkipi/src/plugin.cpp:467
>     #8  0x00007f414392a8e5 in KIPIKXMLHelloWorldPlugin::Plugin_KXMLHelloWorld::Plugin_KXMLHelloWorld (this=0x7f4148025a30, parent=0x2984050) at /home/dh/kde/kf5/src/kde/kdegraphics/libs/libkipi/tests/plugins/kxmlhelloworld/plugin_kxmlhelloworld.cpp:134
>     #9  0x00007f414392cf8b in KPluginFactory::createInstance<KIPIKXMLHelloWorldPlugin::Plugin_KXMLHelloWorld, QObject> (parentWidget=0x0, parent=0x2984050, args=...) at /home/dh/kde/kf5/usr/include/KF5/KCoreAddons/kpluginfactory.h:477
>     #10 0x00007f416eb8afb4 in KPluginFactory::create (this=0x7f4148038990, iface=0x7f416c82a838 <qt_meta_stringdata_KIPI__Plugin+24> "KIPI::Plugin", parentWidget=0x0, parent=0x2984050, args=..., keyword=...) at /home/dh/kde/kf5/src/frameworks/kcoreaddons/src/lib/plugin/kpluginfactory.cpp:145
>     #11 0x00007f416c82635a in KPluginFactory::create<KIPI::Plugin> (this=0x7f4148038990, parentWidget=0x0, parent=0x2984050, keyword=..., args=...) at /home/dh/kde/kf5/usr/include/KF5/KCoreAddons/kpluginfactory.h:519
>     #12 0x00007f416c8258dd in KService::createInstance<KIPI::Plugin> (this=0x7f4148009440, parentWidget=0x0, parent=0x2984050, args=..., error=0x7f4155f90830) at /home/dh/kde/kf5/usr/include/KF5/KService/kservice.h:526
>     #13 0x00007f416c8250c4 in KService::createInstance<KIPI::Plugin> (this=0x7f4148009440, parent=0x2984050, args=..., error=0x7f4155f90830) at /home/dh/kde/kf5/usr/include/KF5/KService/kservice.h:501
>     #14 0x00007f416c823402 in KIPI::PluginLoader::Info::plugin (this=0x7f4148003140) at /home/dh/kde/kf5/src/kde/kdegraphics/libs/libkipi/src/pluginloader.cpp:166
>     #15 0x000000000044d23b in KSnapshotSendToActions::createSendToActions (this=0x29b6de0) at /home/dh/kde/kf5/src/kde/kdegraphics/ksnapshot/ksnapshotsendtoactions.cpp:135
>     #16 0x000000000044ccbb in KSnapshotSendToActions::retrieveActionsFunc (_this=0x29b6de0) at /home/dh/kde/kf5/src/kde/kdegraphics/ksnapshot/ksnapshotsendtoactions.cpp:88
>     #17 0x000000000045005d in QtConcurrent::StoredFunctorCall1<bool, bool (*)(KSnapshotSendToActions*), KSnapshotSendToActions*>::runFunctor (this=0x2b16560) at /home/dh/kde/kf5/src/qt5/qtbase/src/concurrent/qtconcurrentstoredfunctioncall.h:265
>     #18 0x0000000000450006 in QtConcurrent::RunFunctionTask<bool>::run (this=0x2b16560) at /home/dh/kde/kf5/src/qt5/qtbase/src/concurrent/qtconcurrentrunbase.h:104
>     #19 0x00007f4166adb1ad in QThreadPoolThread::run (this=0x2905330) at thread/qthreadpool.cpp:101
>     #20 0x00007f4166ae15fb in QThreadPrivate::start (arg=0x2905330) at thread/qthread_unix.cpp:345
>     #21 0x00007f4165c3f0db in start_thread () from /lib64/libpthread.so.0
>     
>     I'm not sure whether it's possible to create the plugins at all in the thread, given the current Kipi architecture.
>     
>     What would be a straight forward and safe solution to only create the actions once in the main thread, and then reuse these when opening the menu the second time etc. But this would imply removing the threading thingy again.
>     
>     Kevin/Aron: Do you maybe have more comments on this?

Hm. Ouch. I didn't look at KIPI yet, but can we fix their plugin manager?

At any rate: We'll have to remove the threading for now. And I agree with Dominik, at least make sure we cache the action list.

So, in summary, I'd suggest: Keep most of the changes (the refactoring is fine), but remove the use of QtConcurrent and friends.

- Kevin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120920/#review71392
-----------------------------------------------------------

On Dec. 6, 2014, 1:55 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120920/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2014, 1:55 p.m.)
> 
> 
> Review request for KDE Graphics, KSnapshot, Aaron J. Seigo, Gilles Caulier, and Dominik Haumann.
> 
> 
> Bugs: 312495
>     http://bugs.kde.org/show_bug.cgi?id=312495
> 
> 
> Repository: ksnapshot
> 
> 
> Description
> -------
> 
> Hi, I also feel slightly impaired by Bug 312495 - Very slow "Send to" menu. So here is a fix.
> 
> It consists of the following changes:
>     1. move long-running retrieval of SendTo items to separate method
>     2. fillSendToActionsCache at startup but not yet async
>     3. move code related to SendTo actions to own class and file
>          (this includes the kipi code but KIPI_FOUND was not set in my dev env so thats not tested)
>     4. SendTo menu is built on a separate thread to speed up populating
>     5. delete actions in dtor
>     6. Create main window on stack instead of heap
> 
> The SendTo actions are now created in QThread which is started on startup of ksnapshot.
> If the user opens the SendTo menu too soon there is a message (as single menu item) that says
> that the menu is still being constructed and that he/she should try a moment later.
> 
> 
> Diffs
> -----
> 
>   ksnapshot.h 07100a85bbb146501ee77a9cbb9d2655edda4595 
>   ksnapshot.cpp 192a0910553acf9180155ba0f409d7b15d125753 
>   ksnapshotsendtoactions.h PRE-CREATION 
>   ksnapshotsendtoactions.cpp PRE-CREATION 
>   main.cpp e7a1efb2fd8398492b96e6f21f90363d99358e12 
>   .reviewboardrc PRE-CREATION 
>   CMakeLists.txt b8e665610284ae9db644e6925acae3d60e207245 
> 
> Diff: https://git.reviewboard.kde.org/r/120920/diff/
> 
> 
> Testing
> -------
> 
> 1)
> - Start ksnapshot (notice that is as fast as before)
> - Immediately open SendTo menu => there is a message that the menu is not ready yet.
> - Try again after 1 second: menu is populated.
> - Close ksnapshot.
> 
> 2)
> - Start ksnapshot. Wait two seconds and then click SendTo menu
>    => menu is populated right away
> - Close it.
> 
> 3)
> Start ksnapshot. Hit the SendTo menu with insanely high button click speed.
>    => No crash observed.
>    
> 4)
> Start ksnapshot and close it immediately.
>    => No crash observed.
> 
> 5) [pending]
> Could no test the kipi code because KIPI_FOUND is not set yet on my environment.
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-graphics-devel/attachments/20141206/4435b8d3/attachment-0001.html>