Fix crash deep inside VcsOverlayProxyModel::data()
ClosedPublic

Authored by kfunk on Nov 16 2017, 4:54 PM.

Details

Summary

VcsOverlayProxyModel::data() calls ProjectController::changesModel()
which currently is a non-const method which might create an instance
of ProjectChangesModel which in turn triggers some KJobs.

Avoid that by instantiating ProjectChangesModel() immediately during
startup. I don't think that has a real performance impact.

BUG: 384162
FIXED-IN: 5.2.1

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kfunk created this revision.Nov 16 2017, 4:54 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptNov 16 2017, 4:54 PM
kfunk added a reviewer: apol.Nov 16 2017, 4:55 PM
kfunk added a subscriber: apol.

@apol May I ask you to review 6 years old code of yours? :D -- why was ProjectChangesModel lazy-initialized?

From kdevplatform.git:

commit 6ff350bb63f4a6617bf4599711303297ec7e3fc8
Author: Aleix Pol <aleixpol@kde.org>
Date: Sat Jun 4 15:07:58 2011 +0200

Add the projectchangesmodel to the project controller so that we just have one instance of it.
brauch added a subscriber: brauch.Nov 16 2017, 5:01 PM

+1 from me, but let apol review it ;)

kfunk added a subscriber: dfaure.Nov 16 2017, 5:06 PM
mwolff added a subscriber: mwolff.Nov 16 2017, 8:54 PM

lgtm too, does it noticeably influence startup time maybe?

apol added a comment.Nov 17 2017, 12:06 PM

What's the crash? This was done so that we weren't monitoring changes if nobody was listening to them.

In D8852#168856, @apol wrote:

What's the crash?

For the crash report, see: https://bugs.kde.org/show_bug.cgi?id=384162

This was done so that we weren't monitoring changes if nobody was listening to them.

IIUC then the changes model will *always* be instantiated, as the project tree view tracks VCS branches to get the branch name of the project(s). Thus the optimization probably no longer makes sense.

apol added a comment.Nov 17 2017, 12:18 PM

IIUC then the changes model will *always* be instantiated, as the project tree view tracks VCS branches to get the branch name of the project(s). Thus the optimization probably no longer makes sense.

Oh, right.
On the other hand, David Faure just told you what the bug is... :P

Are you sure this fixes the crash anyway?

I think this should help as we won't synchronously run the kjob exec anymore since we don't defer the construction of the changes model until first access from paint/data methods. That said, we should also remove the synchronous jobs...

I confirm that this fixes the reproducible crash I was seeing.

apol accepted this revision.Nov 20 2017, 6:20 PM

Okay, let's get this in, maybe we will get to fix the main issue down the line and restore the optimization at some point.

This revision is now accepted and ready to land.Nov 20 2017, 6:20 PM
This revision was automatically updated to reflect the committed changes.