| Summary: | Random crashes when changing screentone fill layer parameters | ||
|---|---|---|---|
| Product: | [Applications] krita | Reporter: | Deif Lou <ginoba> |
| Component: | General | Assignee: | Dmitry Kazakov <dimula73> |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | CC: | amy, halla, shzam |
| Priority: | NOR | ||
| Version: | 5.0.0 | ||
| Target Milestone: | --- | ||
| Platform: | Android | ||
| OS: | Unspecified | ||
| Latest Commit: | https://invent.kde.org/graphics/krita/commit/f5c9ae3418b8bd81882213a482c452824d68436b | Version Fixed In: | |
| Attachments: |
crash logs
krita 5 android log android crash dump backtrace linux Proposed patch that fixes the issue |
||
Looking at the assert, this might have something to do with the multithreading of fill layers. Amyspark, can you take a look? I've also noticed that when changing rapidly the sliders of a generator, the fill layer is updated several times with the intermediate values emited by the slider instead of discarding them, keeping just the last one and updating the layer only once. This is visible with medium size images (A4-300dpi) using the pattern and multigrid generators, but it seems to happen also with the screentone generator and simplex noise, although the image has to be larger to be noticeable (I think in the case of the simplex noise because it uses a signal compressor and in the case of screentone because it generates the image fast). A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/553 Git commit 195b02123a33ada9c8cd914144116671bd5ea978 by Dmitry Kazakov, on behalf of L. E. Segovia. Committed on 29/10/2020 at 12:29. Pushed by dkazakov into branch 'krita/4.3'. Correctly add barrier jobs for KisGeneratorStrokeStrategy The correct type of job to block Fill Layer updates is BARRIER -- update jobs cannot start unless we are sure the previous set has been cleared and committed to the canvas. Related: bug 427199 M +4 -7 libs/image/generator/kis_generator_stroke_strategy.cpp https://invent.kde.org/graphics/krita/commit/195b02123a33ada9c8cd914144116671bd5ea978 Git commit 7f68229d3bb8580930199a43c7c68921c3f704b7 by Dmitry Kazakov, on behalf of L. E. Segovia. Committed on 29/10/2020 at 12:32. Pushed by dkazakov into branch 'master'. Correctly add barrier jobs for KisGeneratorStrokeStrategy The correct type of job to block Fill Layer updates is BARRIER -- update jobs cannot start unless we are sure the previous set has been cleared and committed to the canvas. Related: bug 427199 M +4 -7 libs/image/generator/kis_generator_stroke_strategy.cpp https://invent.kde.org/graphics/krita/commit/7f68229d3bb8580930199a43c7c68921c3f704b7 Created attachment 144924 [details]
krita 5 android log
After trying Krita on android 5.0 this crash still happens. I attached a log. Can this be related to the screentone generator not using signal compressor? Turning over to Dmitry, this looks like an issue with the canvas lockless storage. Created attachment 145684 [details]
android crash dump
This is what I get with a custom build (RelWithDebInfo), and using adb logcat and ndk-stack. I have to say that with my custom builds I have a harder time to reproduce the crash than using the appstore version, specially with the Debug build. It's very random, with 5.0.2 from appstore I also have a harder time crashing it than I had with 5.0, but easier than with the custom builds.
Created attachment 146435 [details]
backtrace linux
I was able to reproduce the crash on linux. It happened after building with clang ( I don't know if that's relevant).
> it = {m_policy = {m_iter = {d = 0x7fff3c095b70}, m_rawData = 0x7fff46ffc450 "\b", m_oldRawData = 0x7fff46ffc450 "\b"}, m_progressPolicy = {<No data fields>}, m_pixelSize = 32767, m_rowsLeft = 1008384168, m_numConseqPixels = 32767, m_columnsLeft = -1669517436, m_columnOffset = 32767, m_iteratorX = 1191167056, m_iteratorY = 32767, m_isStarted = 146} > #8 0x00007ffff72c0673 in KisDataManager::clear(int, int, int, int, unsigned char const*) (this=0x5555573a2150, x=2048, y=0, w=-208024286, h=0, def=0x7fff46ffc408 "\377\377\377\377\376\352\070'") at /home/deiflou/Development/src/apps/krita/deiflou/sliderspinbox_nojump/libs/image/kis_datamanager.h:188 Not sure what's going on, but that sounds like a out-of-bounds access caused by some overflow there... Hi, Deif Lou! What happens in this crash is the following: 1) The GUI thread calls KisGeneratorLayer::requestUpdateJobsWithStroke(), which calls resetCacheWithoutUpdate(), which, in turn, calls m_d->paintDevice->clear(). 2) At the same time, there is a generator stroke running in the background and writes the result of the filter to the same device. It results in two simultaneous write accesses to the paint device, which is not allowed by the data manager. As far as I remember, this situation should have been guarded by `m_d->updateCookie`, I'm not sure why it doesn't work here. Created attachment 146902 [details]
Proposed patch that fixes the issue
Hi, Deif Lou!
The problem is a regression from stroke-based updates for the generator dialog. Could you please test if this patch fixes the issue?
To make the crash easier to reproduce, please set UPDATE_DELAY in KisDlgGeneratorLayer to 10ms and create "quite big" image, like 7k pixels in size.
Without the patch I could reproduce the issue even in a standard build on Windows with GCC.
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1339 Git commit f5c9ae3418b8bd81882213a482c452824d68436b by Dmitry Kazakov. Committed on 25/02/2022 at 07:21. Pushed by dkazakov into branch 'master'. Possible fix for the Generator Dialog update crash The dialog should also care about the thread safety and use the cookie returned by the updating threads. M +3 -2 libs/image/generator/kis_generator_layer.cpp M +1 -1 libs/image/generator/kis_generator_layer.h M +4 -4 libs/image/generator/kis_generator_stroke_strategy.cpp M +6 -2 libs/ui/dialogs/kis_dlg_generator_layer.cpp M +1 -0 libs/ui/dialogs/kis_dlg_generator_layer.h https://invent.kde.org/graphics/krita/commit/f5c9ae3418b8bd81882213a482c452824d68436b Git commit 7516fe61f6b22ca79215d4aac39ab9a7288730fe by Dmitry Kazakov. Committed on 25/02/2022 at 07:21. Pushed by dkazakov into branch 'master'. Fix thread-safety issues in KoProgressUpdater M +0 -1 libs/image/kis_processing_visitor.cpp M +0 -1 libs/image/kis_processing_visitor.h M +100 -89 libs/widgetutils/KoProgressUpdater.cpp M +3 -0 libs/widgetutils/KoProgressUpdater.h M +3 -0 libs/widgetutils/tests/TestKoProgressUpdater.cpp https://invent.kde.org/graphics/krita/commit/7516fe61f6b22ca79215d4aac39ab9a7288730fe |
Created attachment 132576 [details] crash logs SUMMARY Krita crashes sometimes when updating the parameters of the screentone fill layer. This happens to me both on linux and android, and both in 4.4.0 (tested on linux and android) and master (tested on linux). It seems this is more likely to happen on large images. On my android tablet, it crashes with a A4-300dpi image, but on linux I have to use a larger image to reproduce the crash. In both cases the logs are similar, pointing to "KisTiledExtentManager.cpp". I attach the two relevant logs. STEPS TO REPRODUCE 1. Create a new image (a large size may be needed to reproduce the crash) 2. Create a screentone fill layer 3. Just play with the slides (I silede them very fast to reproduce the crash quickly, but for example on android it can crash with a normal usage) OBSERVED RESULT Krita crashes. EXPECTED RESULT The fill layer is updated correctly with no errors. SOFTWARE/OS VERSIONS Operating System: Manjaro Linux KDE Plasma Version: 5.19.5 KDE Frameworks Version: 5.74.0 Qt Version: 5.15.1 Operating System: Android 10.0 Qt Version: 5.12.9