Differential D10157

Fix Crashing when Changing Smoothing Option in a Stroke (Bug 386726)
ClosedPublic
Actions

Authored by mzhou on Jan 28 2018, 9:53 AM.

Details

Reviewers

dkazakov

Group Reviewers

Krita

Commits

R37:2ec26e2f50f2: Fix Crashing when Changing Smoothing Option in a Stroke (Bug 386726)

Summary

Fix for Bug 386726 (https://bugs.kde.org/show_bug.cgi?id=386726).

The symptom is that Krita crashes when the smoothing option is changed from the stabilizer to something else or from something else to the stabilizer during a stroke.

The first case happens because the stabilizer draws lines even after the user has removed his pen from the canvas, but this isn't what other smoothing options do: they end the stroke and remove it from the memory immediately after the user removes his pen. If the smoothing option is changed from the stabilizer to something else in the middle of a stroke, after the user removes his pen, because the smoothing option is no longer the stabilizer, the stroke and the painting action is removed from the memory; but because the stroke started as a stabilizer stroke, the stabilizer tries to finish the stroke and the action, which no longer exist. In this process, KisRecodingAdapter::addLine(), which doesn't check the m_pathPaintAction pointer before dereferencing it, is called, and there' a segfault.

I added pointer checks to KisRecodingAdapter::addLine(), and made the stabilizer finish the stroke only when the smoothing option is set to it.

For the second case, I haven't looked into it, but the code written to solve the first one seems also able to solve it.

Changing smoothing option in the middle of a stroke still causes unexpected behavior, the stroke would be a mess, but at least that's a behavior that can be cancelled by ctrl + Z and doesn't cause artists to lose their work.

Diff Detail

Repository

R37 Krita

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

mzhou created this revision.Jan 28 2018, 9:53 AM

Restricted Application added a subscriber: woltherav. · View Herald TranscriptJan 28 2018, 9:53 AM

mzhou requested review of this revision.Jan 28 2018, 9:53 AM

mzhou edited the summary of this revision. (Show Details)Jan 28 2018, 10:02 AM

Hi, @mzhou!

Thank you for your patch! There seem to be a problem with setFinishStabilizedCurve(true), which might cause a regression. Can you just keep the check of the timer activity to avoid the crash?

Ideally, to really fix the problem, the stabilizer info should be moved to KisResourcesSnapshot structure and used by all the methods of KisToolFreehandHelper from there. This snapshot class is exactly the thing we use for detaching the current state of GUI controls from state of the currently running asynchronous stroke. Then there will be no race conditions at all.

PS:
Please choose any way you like: either continue with the current approach (workaround the crash) or make a real fix with the snapshot :)

libs/ui/tool/kis_tool_freehand_helper.cpp
605	Finishing the curve is not just a technical thing, it really draws some extra portion of the line on the canvas when the user lifts the pen, so this change will just disable the ability to paint "unfinished" strokes and will introduce a regression
642	From the code it seems that the if can be substituted with something like that: if (m_d->stabilizerPollTimer.isActive()) { stabilizerEnd(); } though it needs a bit of testing.

This revision now requires changes to proceed.Jan 29 2018, 7:51 AM

Alright, working on it.

The regression is caused because even after the stabilizer's timer keeps running after the smoothing type is changed, and it keeps emitting timeout signals to cause the brush, which is no longer using the stabilizer, to perform stabilizer operations.

I let the KisSmoothingOptions class emit a signal when it's smoothing type gets changed and connected it to the KisToolFreeHandHelper, which does most of the painting jobs. When the signal is emitted and the freehandhelper is running, it starts or ends the stabilizer depending on the situation.

Also, I'm kind of curious why the KisToolBrush handles most signals related to smoothing options. Wouldn't it be better to let KisToolFreeHand or KisSmoothingOptions handle them? As it looks like the freehand class does most operations that are related to smoothing options, and KisSmoothingOptions is just the options themselves.

Changed the new slot to be a private one as it's unlikely to be used by other classes.

I would ask: Why do we even want to allow the smoothing options to be changed mid-stroke? Surely the smoothing option is supposed to be applied on the whole stroke. Are there any real use cases that need a stroke with different segments using different smoothing options?

If we do want to support switching to different smoothing options mid-stroke, then I would suggest checking whether the current code can really restart the stroke cleanly. My guess is that it doesn't.

I think one problem is that we can't prevent people from changing it using e.g. a shortcut during the stroke.

In D10157#209422, @rempt wrote:

I think one problem is that we can't prevent people from changing it using e.g. a shortcut during the stroke.

Yeah. I mean we can allow users to change the options, but not actually change the current ongoing stroke. Doesn't the suggestion from @dkazakov aim to handle it this way?

This patch actually was originally created only to solve the crashing problem...
I have to say there's no concrete reasoning behind not choosing the method of taking the snapshot. I just chose the way that seemed more obvious to me. Also, for now the snapshot class doesn't support recording the smoothing options. I'm still getting familiar with Krita and don't want to make changes to a class that is as widely used as the snapshot.
Though I didn't really think much about it, I would say if an artist changes the smoothing option mid-stroke, she probably wants to see the change immediately.

The patch looks fine now. Yes, it still doesn't use snapshots, but it doesn't crash when the user switches the smoothing type right during the stroke (which can be done using shortcuts) and I cannot see any obvious regressions.

@mzhou, do you have commit access or I should push the patch for your? In the latter case, please send me your email to dimula73 at gmail.com, so I could add you as an author of a commit.

This revision is now accepted and ready to land.Feb 20 2018, 9:49 AM

Closed by commit R37:2ec26e2f50f2: Fix Crashing when Changing Smoothing Option in a Stroke (Bug 386726) (authored by mzhou). · Explain WhyFeb 22 2018, 7:07 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			libs/ui/tool/kis_recording_adapter.cpp (3 lines)
M			libs/ui/tool/kis_smoothing_options.h (3 lines)
M			libs/ui/tool/kis_smoothing_options.cpp (1 line)
M			libs/ui/tool/kis_tool_freehand_helper.h (2 lines)
M			libs/ui/tool/kis_tool_freehand_helper.cpp (20 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	26104			Jan 28 2018, 9:35 AM	★	★
Diff 2	27530			Feb 19 2018, 12:03 PM	★	★
Diff 3	27532			Feb 19 2018, 12:19 PM	★	★
Diff 4	27753	f6f83a2	R37:2ec26e2f50f2ce76e69f5738536123d6431d2403	Feb 21 2018, 7:05 AM	★	★