Patch for crash on closing

Hi,

after my patch from yesterday all KOffice apps are now crashing on closing. The problem is that the registry deletes the colospaces in the destructor and the colorspace use the registry to get the cache.
The patch change the behavior so that colorspaces owned by the registry don't delete the cached conversion themself. Instead this is done with the destruction of the cache.

Can you have a look at it?



Sven

[patch.diff]

---

Index: KoColorSpaceRegistry.cpp
===================================================================
--- KoColorSpaceRegistry.cpp (Revision 1039072)
+++ KoColorSpaceRegistry.cpp (Arbeitskopie)
@@ -160,15 +160,12 @@
 
 KoColorSpaceRegistry::~KoColorSpaceRegistry()
 {
-    d->alphaCs->d->ownedByRegistry = false;
-
     delete d->colorConversionSystem;
     foreach( KoColorProfile* profile, d->profileMap) {
         delete profile;
     }
     d->profileMap.clear();
     foreach( const KoColorSpace * cs, d->csMap) {
-        cs->d->ownedByRegistry = false;
         delete cs;
     }
     d->csMap.clear();
Index: KoColorSpace.cpp
===================================================================
--- KoColorSpace.cpp (Revision 1039113)
+++ KoColorSpace.cpp (Arbeitskopie)
@@ -60,15 +60,17 @@
 
 KoColorSpace::~KoColorSpace()
 {
-    Q_ASSERT(!d->ownedByRegistry);
     qDeleteAll(d->compositeOps);
     foreach(KoChannelInfo * channel, d->channels)
     {
         delete channel;
     }
-    KoColorConversionCache* cache = KoColorSpaceRegistry::instance()->colorConversionCache();
-    if (cache) {
-        cache->colorSpaceIsDestroyed(this);
+    // conversions of colorspaces owned by the registry will be deleted once the cache gets deleted
+    if(!d->ownedByRegistry) {
+        KoColorConversionCache* cache = KoColorSpaceRegistry::instance()->colorConversionCache();
+        if (cache) {
+            cache->colorSpaceIsDestroyed(this);
+        }
     }
     delete d->mixColorsOp;
     delete d->convolutionOp;

---

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Friday 23 October 2009, Sven Langkamp wrote:

> Hi,
>
> after my patch from yesterday all KOffice apps are now crashing on closing.
> The problem is that the registry deletes the colospaces in the destructor
> and the colorspace use the registry to get the cache.
> The patch change the behavior so that colorspaces owned by the registry
> don't delete the cached conversion themself. Instead this is done with the
> destruction of the cache.
>
> Can you have a look at it?

That look ok. I think it was for the same reason that I added the check
in the first place.
--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Cyrille Berger]

Hi,

On Friday 23 October 2009, Sven Langkamp wrote:
> Can you have a look at it?

I disagree with the removal of the Q_ASSERT(!d->ownedByRegistry);
It's there for a reason. So we can either add an other boolean, replaced
ownedByRegistry by an enum or use a permanent color space in the cache, which
would remove the need to call colorSpaceIsDestroyed.
--
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Friday 23 October 2009, Cyrille Berger wrote:

> Hi,
>
> On Friday 23 October 2009, Sven Langkamp wrote:
> > Can you have a look at it?
>
> I disagree with the removal of the Q_ASSERT(!d->ownedByRegistry);
> It's there for a reason. So we can either add an other boolean, replaced
> ownedByRegistry by an enum or use a permanent color space in the cache,
>  which would remove the need to call colorSpaceIsDestroyed.
>

I've got the following variant:

diff --git a/libs/pigment/KoColorSpace.cpp b/libs/pigment/KoColorSpace.cpp
index c8a33ab..670be3a 100644
--- a/libs/pigment/KoColorSpace.cpp
+++ b/libs/pigment/KoColorSpace.cpp
 -56,19 +56,23 @@ KoColorSpace::KoColorSpace(const QString &id, const QString
&name, KoMixColorsOp
     d->transfoFromRGBA16 = 0;
     d->transfoToLABA16 = 0;
     d->transfoFromLABA16 = 0;
+    d->deletability = NotOwnedByRegistry;
 }
 
 KoColorSpace::~KoColorSpace()
 {
-    Q_ASSERT(!d->ownedByRegistry);
+    Q_ASSERT(d->deletability != OwnedByRegistryDoNotDelete);
+
     qDeleteAll(d->compositeOps);
     foreach(KoChannelInfo * channel, d->channels)
     {
         delete channel;
     }
-    KoColorConversionCache* cache = KoColorSpaceRegistry::instance()-
>colorConversionCache();
-    if (cache) {
-        cache->colorSpaceIsDestroyed(this);
+    if (d->deletability == NotOwnedByRegistry) {
+        KoColorConversionCache* cache = KoColorSpaceRegistry::instance()-
>colorConversionCache();
+        if (cache) {
+            cache->colorSpaceIsDestroyed(this);
+        }
     }
     delete d->mixColorsOp;
     delete d->convolutionOp;
diff --git a/libs/pigment/KoColorSpace.h b/libs/pigment/KoColorSpace.h
index a1d0c4d..b4d357c 100644
--- a/libs/pigment/KoColorSpace.h
+++ b/libs/pigment/KoColorSpace.h
 -42,6 +42,12 @@ class KoColorTransformation;
 class KoColorConversionTransformationFactory;
 class QBitArray;
 
+enum Deletability {
+    OwnedByRegistryDoNotDelete,
+    OwnedByRegistryRegistyDeletes,
+    NotOwnedByRegistry
+};
+
 enum ColorSpaceIndependence {
     FULLY_INDEPENDENT,
     TO_LAB16,
diff --git a/libs/pigment/KoColorSpaceRegistry.cpp
b/libs/pigment/KoColorSpaceRegistry.cpp
index 7cdf69f..349bf5a 100644
--- a/libs/pigment/KoColorSpaceRegistry.cpp
+++ b/libs/pigment/KoColorSpaceRegistry.cpp
 -137,7 +137,7 @@ void KoColorSpaceRegistry::init()
 
     // Create the built-in colorspaces
     d->alphaCs = new KoAlphaColorSpace();
-    d->alphaCs->d->ownedByRegistry = true;
+    d->alphaCs->d->deletability = OwnedByRegistryDoNotDelete;
 
     KoPluginLoader::PluginsConfig config;
     config.whiteList = "ColorSpacePlugins";
 -160,7 +160,6 @@ KoColorSpaceRegistry::KoColorSpaceRegistry() : d(new
Private())
 
 KoColorSpaceRegistry::~KoColorSpaceRegistry()
 {
-    d->alphaCs->d->ownedByRegistry = false;
 
     delete d->colorConversionSystem;
     foreach( KoColorProfile* profile, d->profileMap) {
 -168,7 +167,7 @@ KoColorSpaceRegistry::~KoColorSpaceRegistry()
     }
     d->profileMap.clear();
     foreach( const KoColorSpace * cs, d->csMap) {
-        cs->d->ownedByRegistry = false;
+        cs->d->deletability = OwnedByRegistryRegistyDeletes;
         delete cs;
     }
     d->csMap.clear();
 -176,6 +175,7 @@ KoColorSpaceRegistry::~KoColorSpaceRegistry()
     delete d->colorConversionCache;
     d->colorConversionCache = 0;
 
+    d->alphaCs->d->deletability = OwnedByRegistryRegistyDeletes;
     delete d->alphaCs;
     // Do not explicitly delete d->rgbU8sRGB and d->lab16sLAB, since they are
contained in the d->csMap
     delete d;
 -319,7 +319,7 @@ const KoColorSpace * KoColorSpaceRegistry::colorSpace(const
QString &csID, const
         }
 
         d->csMap[name] = cs;
-        cs->d->ownedByRegistry = true;
+        cs->d->deletability = OwnedByRegistryDoNotDelete;
         dbgPigmentCSRegistry << "colorspace count: " << d->csMap.count() <<
", adding name: " << name;
     }
 
 -361,7 +361,7 @@ const KoColorSpace * KoColorSpaceRegistry::colorSpace(const
QString &csID, const
 
             QString name = csID + "<comb>" + profile->name();
             d->csMap[name] = cs;
-            cs->d->ownedByRegistry = true;
+            cs->d->deletability = OwnedByRegistryDoNotDelete;
             dbgPigmentCSRegistry << "colorspace count: " << d->csMap.count()
<< ", adding name: " << name;
         }
 
 -508,7 +508,7 @@ KoColorConversionCache*
KoColorSpaceRegistry::colorConversionCache() const
 
 const KoColorSpace* KoColorSpaceRegistry::permanentColorspace( const
KoColorSpace* _colorSpace )
 {
-    if(_colorSpace->d->ownedByRegistry) return _colorSpace;
+    if(_colorSpace->d->deletability != NotOwnedByRegistry) return
_colorSpace;
     else if(*_colorSpace == *d->alphaCs) return d->alphaCs;
     else {
         const KoColorSpace* cs = colorSpace(_colorSpace->id(), _colorSpace-
>profile());
diff --git a/libs/pigment/KoColorSpace_p.h b/libs/pigment/KoColorSpace_p.h
index 2491ef9..3c58c79 100644
--- a/libs/pigment/KoColorSpace_p.h
+++ b/libs/pigment/KoColorSpace_p.h
 -23,8 +23,6 @@
 #include "KoColorSpace.h"
 
 struct KoColorSpace::Private {
-    Private() : ownedByRegistry(false)
-    {}
 
     QString id;
     quint32 idNumber;
 -35,13 +33,12 @@ struct KoColorSpace::Private {
     KoConvolutionOp* convolutionOp;
     QThreadStorage< QVector<quint8>* > conversionCache;
 
-
     mutable KoColorConversionTransformation* transfoToRGBA16;
     mutable KoColorConversionTransformation* transfoFromRGBA16;
     mutable KoColorConversionTransformation* transfoToLABA16;
     mutable KoColorConversionTransformation* transfoFromLABA16;
 
-    bool ownedByRegistry;
+    Deletability deletability;
 };
 
 #endif

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop