Crash after requesting context menu (after reopening the project)

[Manuel Breugelmans]

On Thursday 31 July 2008 08:39:29 Andreas Pakulat wrote:
> On 31.07.08 01:59:24, Manuel Breugelmans wrote:
> > On Wednesday 30 July 2008 00:43:17 Andreas Pakulat wrote:
> > > Hmmm... I can't see how this could happen. Granted the project pointer
> > > itself isn't deleted until the event loop runs again after closing it,
> > > but the project items are being deleted properly. So the project tree
> > > has to have the new items and their project() pointer needs to be
> > > "proper".
> > >
> > > You should be able to debug this by simply printing out the pointer of
> > > the project in projectcontroller::closeProject() before its deleted and
> > > then also in veritas coverageplugin before the crash line. Those two
> > > pointers should be different. If they are, there must be a problem
> > > elsehwere.
> >
> > I've given this a long hard look and spotted the culprit. Not dvcs nor
> > veritas but some obscure plugin loading code in projectcontroller [ the
> > person that introduced this now owes +1 beer to Evgeniy & me :) ]
> >
> > I deduced this with the following minimal test cases:
> > */ start kdev4 >> load project >> reopen project [no close, just recent-
> >
> > >project] >> context menu >> crash
> >
> > */  start kdev4 >> load project >> close project >> load project >>
> > context menu >> no crash
> > */ start kdev4 >> load cmake project >> load other cmake project >>
> > reload cmake project >> context on reloaded >> no crash
> > */ start kdev4 >> load cmake project >> load make project >> reload cmake
> > project >> context menu reloaded >> crash
> >
> > On a reopen project ProjectController unloads the KDevCmakeBuilder plugin
> > (IF no other open projects use it) then immediatly loads it again, which
> > results in that segfault.
>
> Did you find out why?

I'm not 100% but I think it's the plugin doing a deleteLater() when unloading. 
Someone with a bit more experience might have to look at that. Maybe the 
proper fix for this is to make the closeProject and openProject 'non-blocking' 
and connect to the deleted signal of the plugin to emit projectClosed. That 
would change the semantics of these members though...

> > Attached is a patch which:
> > 1. Fixes this bug, by not unloading plugins when reloading a project
> > which is redundant anyway
>
> I don't like that. It duplicates code, reopen should be the same as
> close+open so there's no reason to change that - IMHO.

agreed, either by using a flag or change the semantics of the interface as I 
said above.

> > 2. Refactor the ProjectController->closeProject() member as this had
> > started to rot
>
> No it didn't. How do you know it rots? I'm not seeing much sense behind
> factoring out code from closeProject, unless its used in more than one
> place - which isn't the case (taking into account the above comment).

Hmm that's common practice to increase readability, or so I was taught ;) 
Whenever you feel like a chunk of code is non trivial enough to comment it, 
use an ExtractMethod refactoring instead. I would say keep that.

> Besides: "getRidOfProject" is not a proper method name.

Yes probably inline that again.

> > 3. Remove faulty signals from ProjectController, which are also present
> > in IProjectController
> > 4. Add a unit test for ProjectController
>
> These two are ok.
>
> > Index: shell/projectcontroller.h
> > ===================================================================
> > --- shell/projectcontroller.h	(revision 839851)
> > +++ shell/projectcontroller.h	(working copy)
> > @@ -55,18 +55,13 @@
> >  //     IProject* currentProject() const;
> >
> >  public Q_SLOTS:
> > -    bool openProject( const KUrl &KDev4ProjectFile = KUrl() );
> > -    bool projectImportingFinished( IProject* );
> > -    bool closeProject( IProject* );
> > -    void closeAllProjects();
> > -    bool configureProject( IProject* );
> > +    virtual bool openProject( const KUrl &KDev4ProjectFile = KUrl() );
> > +    virtual bool projectImportingFinished( IProject* );
> > +    virtual bool closeProject( IProject* );
> > +    virtual void closeAllProjects();
> > +    virtual bool configureProject( IProject* );
>
> There's not really a need to add the virtual there, but I don't care
> either way.

No real need but slight benifit when reading, as it suggests they are part of 
the public interface of IProjectController.

Btw powerfox with that patch it will still crash on a 
openProject()
closeProject().


Manuel