[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Fix crash in KoMainWindow.cpp
From:       Thomas Zander <zander () kde ! org>
Date:       2010-05-24 21:29:42
Message-ID: 201005242329.42764.zander () kde ! org
[Download RAW message or body]

On Monday 24. May 2010 22.12.27 Johannes Simon wrote:
> I'm a bit too lazy to open a review request for this, as it's a bit
> cumbersome to convert my git diff into a valid one for reviewboard, so
> here's my patch to fix a crash in KoMainWindow.cpp that happened upon
> closing KWord. Basically, a view that was the current active view was
> destroyed, which (after it was destroyed) triggered a slot in
> KoMainWindow.cpp that did some simple operations on the active view, which
> resulted in a crash.
> 
> Here is the fix, I'm putting it up for review:
> 
> diff --git a/libs/main/KoMainWindow.cpp b/libs/main/KoMainWindow.cpp
> index 959ad2e..8559af9 100644
> --- a/libs/main/KoMainWindow.cpp
> +++ b/libs/main/KoMainWindow.cpp
> @@ -482,7 +482,10 @@ void KoMainWindow::setRootDocument(KoDocument 
*doc)
>      emit restoringDone();
> 
>      while(!oldRootViews.isEmpty()) {
> -        delete oldRootViews.takeFirst();
> +        KoView *oldRootView = oldRootViews.takeFirst();
> +        if ( oldRootView == d->activeView )
> +            d->activeView = 0;
> +        delete oldRootView;
>      }
>      if (oldRootDoc && oldRootDoc->viewCount() == 0) {
>          //kDebug(30003) <<"No more views, deleting old doc" << oldRootDoc;
> 
> 
> All this looks highly suspicious to me, especially because d->activeView is
> still one of the oldRootViews when it should actually be something new. I
> attached the crash backtrace for your reference.

Not going to speculate too much over that code, but your solution sounds a 
bit too expensive to me. Setting a new rootDocument deletes all views so 
why not just this patch instead;

--- libs/main/KoMainWindow.cpp
+++ libs/main/KoMainWindow.cpp
@@ -431,6 +431,7 @@ void KoMainWindow::setRootDocument(KoDocument *doc)
     //kDebug(30003) <<"KoMainWindow::setRootDocument this =" << this <<" 
doc =" << doc;
     QList<KoView*> oldRootViews = d->rootViews;
     d->rootViews.clear();
+    d->activeView = 0;
     KoDocument *oldRootDoc = d->rootDoc;
 
     if (oldRootDoc) {

-- 
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]