OgrFeatureDefn Reference Counting affecting SWIG bindings

[cfis@…]

In the SWIG bindings I'm running into problems with features existing longer
than their layers.

def ogr_seg_fault():
   layer = ds.CreateLayer( 'pm1' )

   ogrtest.quick_create_layer_def( layer,
                                   [ ('PRIME_MERIDIAN_CODE', ogr.OFTInteger),
                                     ('INFORMATION_SOURCE', ogr.OFTString) ] )

   dst_feat = ogr.Feature( feature_def = layer.GetLayerDefn() )
   ds.DeleteLayer(0)

The dst_feat is not deleted until after the layer is deleted.  This causes
problems (segmentation faults in Ruby, for some reason Python does not do that
but I think that's just pure luck) because, as I'm sure you know, Feature
references a OgrFeatureDefn that is deleted by the layer. In fact, if you set
CPL_DEBUG you'll see:

D:\msys\1.0\src\gdalautotest\ogr>python ogr_seg_fault.py
 TEST: ogr_seg_fault ... OGRFeatureDefn: OGRFeatureDefn pm1 with a ref count of
1 deleted!


I see that OgrFeatureDefn is already reference counted.  What would you think of
the idea that when a layer is deleted it decrements the reference count on
OgrFeatureDefn instead of destroying it.  Of course it would also be necessary
to update OgrFeatureDefn::Dereference to destroy itself when the reference count
goes to zero.  Doing this would mean that features could be destroyed after
layers without any ill-effects.  Of course if you did something stupid like this:

   dst_feat = ogr.Feature( feature_def = layer.GetLayerDefn() )
   ds.DeleteLayer(0)
   dst_feat.some_method

Then that's your fault.

I've done a quick implementation for the CVS layer to make sure this works (it
seems to), I've attached the patch for you to look at below.  Note if you like
this idea, I would probably implement it slightly differently.  In the
constructor of OgrFeatureDefn I would probably set the reference count to 1,
thus who ever creates a OgrFeatureDefn (I'm guessing just layers, but don't know
OGR that well) would not have to immediately call Reference.


Thanks,

Charlie

---------------------------------

Test code:

import os
import sys
import string

sys.path.append( '../pymod' )

import gdaltest
import ogrtest
import ogr
import gdal

def ogr_seg_fault():
    tempDir = 'c:/temp/csvwrk'
    driver = ogr.GetDriverByName('CSV')
    driver.DeleteDataSource( tempDir )

    ds = driver.CreateDataSource( tempDir )

    layer = ds.CreateLayer( 'pm1' )

    ogrtest.quick_create_layer_def( layer,
                                    [ ('PRIME_MERIDIAN_CODE', ogr.OFTInteger),
                                      ('INFORMATION_SOURCE', ogr.OFTString) ] )

    dst_feat = ogr.Feature( feature_def = layer.GetLayerDefn() )
    ds.DeleteLayer(0)

gdaltest_list = [
    ogr_seg_fault ]

if __name__ == '__main__':

    gdaltest.setup_run( 'ogr_seg_fault' )

    gdaltest.run_tests( gdaltest_list )

    gdaltest.summarize()



Patch:



Index: ogrsf_frmts/csv/ogrcsvlayer.cpp
===================================================================
RCS file: /cvs/maptools/cvsroot/gdal/ogr/ogrsf_frmts/csv/ogrcsvlayer.cpp,v
retrieving revision 1.5
diff -u -w -b -r1.5 ogrcsvlayer.cpp
--- ogrsf_frmts/csv/ogrcsvlayer.cpp    20 Jun 2005 17:54:04 -0000    1.5
+++ ogrsf_frmts/csv/ogrcsvlayer.cpp    21 Aug 2005 22:58:05 -0000
@@ -74,6 +74,7 @@
    nNextFID = 1;

    poFeatureDefn = new OGRFeatureDefn( pszLayerNameIn );
+     poFeatureDefn->Reference();
    poFeatureDefn->SetGeomType( wkbNone );

/* -------------------------------------------------------------------- */
@@ -208,7 +209,8 @@
                  poFeatureDefn->GetName() );
    }

-    delete poFeatureDefn;
+    //delete poFeatureDefn;
+     poFeatureDefn->Dereference();
       VSIFClose( fpCSV );
}

Index: ogr_feature.h
===================================================================
RCS file: /cvs/maptools/cvsroot/gdal/ogr/ogr_feature.h,v
retrieving revision 1.29
diff -u -w -b -r1.29 ogr_feature.h
--- ogr_feature.h    23 Feb 2004 21:47:23 -0000    1.29
+++ ogr_feature.h    21 Aug 2005 23:11:46 -0000
@@ -232,7 +232,11 @@
    OGRFeatureDefn *Clone();

    int         Reference() { return ++nRefCount; }
-    int         Dereference() { return --nRefCount; }
+    int         Dereference() { --nRefCount;
+                                            if (nRefCount==0) {
+                                                delete this;
+                                            }
+                                            return nRefCount;}
    int         GetReferenceCount() { return nRefCount; }

    static OGRFeatureDefn  *CreateFeatureDefn( const char *pszName = NULL );

[warmerdam]

Charlie, 

I have made a comprehensive pass through OGR fixing reference counting for
OGRFeatureDefn as discussed earlier so I think this is all fixed now.  Let me 
know if that is not the case.