| Summary: | Kate crashes when project plugin autoloads GIT repos | ||
|---|---|---|---|
| Product: | [Applications] kate | Reporter: | jm.ouwerkerk |
| Component: | application | Assignee: | KWrite Developers <kwrite-bugs-null> |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | CC: | christoph |
| Priority: | NOR | ||
| Version: | 5.0.0 | ||
| Target Milestone: | --- | ||
| Platform: | Debian unstable | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/kate/df08e724d8d83d821a2d6ceb0bb41f231dbdd451 | Version Fixed In: | |
| Sentry Crash Report: | |||
| Attachments: | Working test case in C (does not crash) | ||
|
Description
jm.ouwerkerk
2015-10-16 03:39:40 UTC
> Please review whether or not Kate's projects plugin uses the libgit2 API correctly or whether this is a regression in libgit proper.
Hi, I reviewed our code, I really don't see any error there :/
It turns out the same crash can also be provoked by merely having the Project plugin enabled and trying to open a document from a git project. This seems to happen when the document is not already in session. That is: I now have a session in which I'm happily editing away at my own GIT project which originally triggered the crash. I can save the session, open the session and open new documents from the same GIT repo inside this session. But when I try to open, say, CMakeLists.txt from a recently updated copy of the Kate source repo itself inside a fresh new session (another Kate instance) then I get the crash. Disabling the Project plugin entirely circumvents this. Scratch that last bit: the crash in Kate repos seems directly linked to the .kateproject file (which I hadn't noticed because hidden file). So the bug manifests itself when loading a file from a git repo which generated either an auto-loaded a KateProject instance or is explicitly configured with a .kateproject file. Created attachment 95017 [details]
Working test case in C (does not crash)
It seems libgit2 is not to blame, since the attached simple test case in C does not produce the same crash.
The test case simply opens a GIT repo using the ***_open_ext() call (same as the projects plugin) and closes it again with added printf() messages to track each step.
Maybe the bug is caused by the Kate project plugin's use of threading (ThreadWeaver) ?
You need to keep in mind: we use libgit2 in different threads, that might lead to different race conditions in libgit2 than the singled threaded test case ;=) Yeah, the test case merely verifies whether the problem is one for the libgit2 devs to fix or for Kate to work around. It seems to be the latter. Note that threading support in libgit2 is not exactly... out of beta? Also, what exactly are the guarantees made by ThreadWeaver? Is it merely a frontend to a thread pool which dispatches jobs to the worker/slave threads? In which case: what is the guarantee that a git_libgit2_init() call executed in Thread A is valid for a GIT job running in the context of Thread B? I'm not too sure how one would go about implementing this, but would it be possible for the Kate project plugin to 'pin' all its GIT operations to a single specific QThread? I think you point to the right issue: we miss to call the init functions in the threads :/ The good thing: KTextEditor uses it only in the main thread an calls it there. Kate can call it in the plugin inside the right thread, as it is "OK" to stack that calls: https://libgit2.github.com/libgit2/#HEAD/group/libgit2/git_libgit2_init Git commit 4de75478b35d65b597742cc5192de14648bc1e47 by Christoph Cullmann. Committed on 17/10/2015 at 14:09. Pushed by cullmann into branch 'master'. init libgit2 in right thread avoid too much configure magic, we require recent enough libgit anyways M +2 -4 addons/project/CMakeLists.txt M +0 -26 addons/project/kateprojectplugin.cpp M +9 -2 addons/project/kateprojectworker.cpp D +0 -3 addons/project/libgit2_config.h.cmake http://commits.kde.org/kate/4de75478b35d65b597742cc5192de14648bc1e47 Actually I hit on a different way to fix the issue. I'll try and post a patch through reviewboard. It's a less invasive change and I tested it (i.e. works for me). Git commit 567128d28249969e57c0ec770073b3f703e491a1 by Christoph Cullmann. Committed on 17/10/2015 at 14:22. Pushed by cullmann into branch 'master'. simplify git2 init, given we require recent enough version M +1 -1 CMakeLists.txt M +2 -6 src/CMakeLists.txt M +0 -2 src/document/katedocument.cpp D +0 -3 src/libgit2_config.h.cmake M +3 -14 src/utils/kateglobal.cpp http://commits.kde.org/ktexteditor/567128d28249969e57c0ec770073b3f703e491a1 > Actually I hit on a different way to fix the issue. I'll try and post a patch through reviewboard. It's a less invasive change and I tested it (i.e. works for me).
Hi, if you have some other fix, happy to see it ;=)
I still think the last two changes I made make it easier, given we no longer check for functions the library version we require no longer has and we init the stuff in the right thread now, always,
Git commit 1a56451ea0d7d7cbc8d8e06ddfa033063d0e825b by Christoph Cullmann. Committed on 17/10/2015 at 14:34. Pushed by cullmann into branch 'master'. use constData M +2 -1 src/document/katedocument.cpp http://commits.kde.org/ktexteditor/1a56451ea0d7d7cbc8d8e06ddfa033063d0e825b Git commit df08e724d8d83d821a2d6ceb0bb41f231dbdd451 by Christoph Cullmann. Committed on 17/10/2015 at 14:40. Pushed by cullmann into branch 'master'. use constData and cleanup the code a bit REVIEW: 125674 M +10 -10 addons/project/kateprojectworker.cpp http://commits.kde.org/kate/df08e724d8d83d821a2d6ceb0bb41f231dbdd451 Please try out the latest project plugin patch ;=) I think the object on the stack should not actually help, but it makes the code a bit cleaner. constData() is more nice, too. I still think the missing _init stuff was the main problem and all other things just "good luck" ;) I've recompiled my Kate using kdesrc-build from current master. The issue appears to be fixed now. Yeah! Thanks btw. for: a) the report b) the help to resolve this I only all bugs would work out that way ;=) |