Bug 428014

Summary: Random crashes when changing screentone fill layer parameters
Product: krita Reporter: Deif Lou <ginoba>
Component: GeneralAssignee: 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: Version Fixed In:
Attachments: crash logs
krita 5 android log
android crash dump
backtrace linux
Proposed patch that fixes the issue

Description Deif Lou 2020-10-20 09:44:24 UTC
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
Comment 1 Halla Rempt 2020-10-20 09:48:18 UTC
Looking at the assert, this might have something to do with the multithreading of fill layers. 

Amyspark, can you take a look?
Comment 2 Deif Lou 2020-10-20 10:11:41 UTC
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).
Comment 3 Bug Janitor Service 2020-10-26 16:46:56 UTC
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/553
Comment 4 Dmitry Kazakov 2020-10-29 12:29:25 UTC
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
Comment 5 Dmitry Kazakov 2020-10-29 12:32:59 UTC
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
Comment 6 Deif Lou 2021-12-29 10:14:23 UTC
Created attachment 144924 [details]
krita 5 android log
Comment 7 Deif Lou 2021-12-29 10:17:36 UTC
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?
Comment 8 amyspark 2021-12-29 19:26:51 UTC
Turning over to Dmitry, this looks like an issue with the canvas lockless storage.
Comment 9 Deif Lou 2022-01-20 22:13:53 UTC
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.
Comment 10 Deif Lou 2022-02-08 12:02:02 UTC
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).
Comment 11 amyspark 2022-02-08 12:13:36 UTC
>         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...
Comment 12 Dmitry Kazakov 2022-02-18 07:00:32 UTC
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.
Comment 13 Dmitry Kazakov 2022-02-18 07:48:16 UTC
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.
Comment 14 Bug Janitor Service 2022-02-18 07:52:38 UTC
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1339
Comment 15 Dmitry Kazakov 2022-02-25 07:21:44 UTC
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
Comment 16 Dmitry Kazakov 2022-02-25 07:21:52 UTC
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