Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#16231 closed defect (fixed)

wxDataViewCtrl sortDescriptors memory leak wxOSX-Cocoa

Reported by: johnr Owned by: VZ
Priority: normal Milestone:
Component: wxOSX-Cocoa Version: dev-latest
Keywords: wxDataViewCtrl sortDescriptors leak wxOSX-Cocoa Cc:
Blocked By: Blocking:
Patch: yes

Description

The fix for #16210 comment 9 exposes a memory leak when setting an NSArray of sortDescriptors in dataview.mm outlineView:sortDescriptorsDidChange via setSortDescriptors.

Previous to the patch in #16210 the outlineView was released twice and rebuilt once hence the leak was not seen.

To reproduce:
1.Apply the patch in #16210 comment 9
2.Build the dataview sample
3.Click the column 0 "Title" header to cause a resort or two
4.Close the sample and check your leaks

Below is the leak report.

Leaked Object	#Address	Size	Responsible Library	Responsible Frame
OS_dispatch_source	1	0x102d30210	160 Bytes	AppKit	-[NSApplication _installMemoryPressureDispatchSources]
OS_dispatch_source	1	0x102d30090	160 Bytes	AppKit	-[NSApplication _installMemoryStatusDispatchSources]
__NSCFString	1	0x102cd1490	48 Bytes	Foundation	-[NSPlaceholderString initWithBytes:length:encoding:]
Malloc 16 Bytes	1	0x102cb8640	16 Bytes	minimal_cocoa	-[wxCocoaOutlineDataSource outlineView:sortDescriptorsDidChange:]
wxSortDescriptorObject	1	0x102cd8390	64 Bytes	minimal_cocoa	-[wxCocoaOutlineDataSource outlineView:sortDescriptorsDidChange:]
NSMutableArray	1	0x102cbbfb0	48 Bytes	minimal_cocoa	-[wxCocoaOutlineDataSource outlineView:sortDescriptorsDidChange:]

Change History (8)

comment:1 Changed 4 months ago by johnr

  • Patch set

(That should have been comment 10 above)
The diff below fixes the leak in the sortDescriptors

Index: src/osx/cocoa/dataview.mm
===================================================================
--- src/osx/cocoa/dataview.mm	(revision 76432)
+++ src/osx/cocoa/dataview.mm	(working copy)
@@ -1080,8 +1080,6 @@
 
 -(void) setSortDescriptors:(NSArray*)newSortDescriptors
 {
-    [newSortDescriptors retain];
-    [sortDescriptors release];
     sortDescriptors = newSortDescriptors;
 }
Last edited 4 months ago by johnr (previous) (diff)

comment:2 Changed 4 months ago by johnr

  • Resolution set to invalid
  • Status changed from new to closed

I have looked at this one again and can't replicate it anymore so I will close it for now.

comment:3 Changed 4 months ago by johnr

  • Resolution invalid deleted
  • Status changed from closed to reopened

I closed this ticket after checking it again yesterday because on testing the leaks sometimes didn't appear. However, it was a combination of circumstances where I had sorting an outlineview column, edited a cell and sorted another column not necessarily in that order which seemed to remove the leaks.

I have spent more time understanding the code. On sorting just the outlineview column then the leaks asre consistent. To reproduce this leak nothing needs to be changed in sample or the wxDVC code and it just needs a click or two on the "title" column header in the sample.

I then reverted the patch in #16210 comment 9 that removes ClearColumns() but still had leaks as documented above.

outlineView:sortDescriptorsDidChange contans the line

[(wxCocoaOutlineDataSource*)[outlineView dataSource] setSortDescriptors:wxSortDescriptors];

and we retain the sort descriptors while releasing the previous descriptor array as in the original code in comment 1. This means the last wxSortDescriptors retained in setSortDescriptors:(NSArray*)newSortDescriptors remain retained when we destroy the DVC.

Seems obvious now after looking at it for some time that the above code setSortDescriptors for the data source rather than the outlineview and they were not being released at destruction so hence a replacement patch. I have tested the diff below but maybe someone else should check it because I have wxDataViewFatigue.

Index: src/osx/cocoa/dataview.mm
===================================================================
--- src/osx/cocoa/dataview.mm	(revision 76497)
+++ src/osx/cocoa/dataview.mm	(working copy)
@@ -1989,7 +1989,13 @@
 
 wxCocoaDataViewControl::~wxCocoaDataViewControl()
 {
-    [m_DataSource  release];
+    if( m_DataSource )
+    {
+        [m_DataSource setSortDescriptors:nil];
+        [m_DataSource release];
+        m_DataSource = NULL;
+    }
+
     [m_OutlineView release];
 }

I think this completes the ticket suite beginning with #16210 and including #16223 #16226 and this ticket #16231

Perhaps it is unnecessary but I needed the diff below for the sample in order to use XCode's instruments leak tool. Snapshoot after sorting and again after closing the dvc window before closing frameMain for those not familiar with the tool.

Index: samples/dataview/dataview.cpp
===================================================================
--- samples/dataview/dataview.cpp	(revision 76497)
+++ samples/dataview/dataview.cpp	(working copy)
@@ -239,10 +239,13 @@
 {
     if ( !wxApp::OnInit() )
         return false;
-
+    
+    wxFrame* frameMain = new wxFrame( NULL , wxID_ANY, "wxDataViewCtrl master" );
+    
     MyFrame *frame =
-        new MyFrame(NULL, "wxDataViewCtrl sample", 40, 40, 1000, 540);
-
+    new MyFrame(frameMain, "wxDataViewCtrl sample", 40, 40, 1000, 540);
+    
+    frameMain->Show( true );
     frame->Show(true);
     return true;
 }

comment:4 Changed 4 months ago by johnr

Yet another update on this patch but the sample does not test rebuilding the wxDVC via AssociateModel(NULL) + ClearColumns and then rebuilding columns and using AssociateModel(modelNew) but I have code that does.

Using ClearColumns() or a second AssociateModel( model ) also causes a leak so wherever we release the data source for the outlineview or release the outlineview itself then we have to do similar with the sort descriptors or there is a leak, hence an updated patch. I am hoping I will not wake at night with another "insight".

I can't find anyway to change outlineView sortDescriptorsDidChange where we create these sortDescriptors to avoid the leaks and not cause a crash after editing a cell or doing other sorting actions.

Not sure whether this will apply cleanly either because some of my other changes may change line numbers to be off.

Index: src/osx/cocoa/dataview.mm
===================================================================
--- src/osx/cocoa/dataview.mm	(revision 76497)
+++ src/osx/cocoa/dataview.mm	(working copy)
@@ -1989,7 +1989,8 @@
 
 wxCocoaDataViewControl::~wxCocoaDataViewControl()
 {
-    [m_DataSource  release];
+    [m_DataSource setSortDescriptors:nil];
+    [m_DataSource release];
     [m_OutlineView release];
 }
 
@@ -2001,6 +2002,7 @@
     // as there is a bug in NSOutlineView version (OSX 10.5.6 #6555162) the
     // columns cannot be deleted if there is an outline column in the view;
     // therefore, the whole view is deleted and newly constructed:
+    [m_DataSource setSortDescriptors:nil];
     [m_OutlineView release];
     m_OutlineView = [[wxCocoaOutlineView alloc] init];
     [((NSScrollView*) GetWXWidget()) setDocumentView:m_OutlineView];
@@ -2280,6 +2284,7 @@
 //
 bool wxCocoaDataViewControl::AssociateModel(wxDataViewModel* model)
 {
+    [m_DataSource setSortDescriptors:nil];
     [m_DataSource release];
     if (model)
     {

comment:5 Changed 3 months ago by vadz

I think a simpler and more fool proof fix is to just release sortDescriptors in dealloc of wxCocoaOutlineDataSource itself.

I do wonder how does the current code work when it doesn't initialize sortDescriptors to nil in init however, shouldn't it simply crash when releasing it for the first time in setSortDescriptors?

In any case, thanks a lot for looking, debugging and fixing this and other memory leaks in wxDVC!

comment:6 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from reopened to closed

In 76638:

Don't leak sort descriptors array in wxDataViewCtrl in wxOSX.

wxCocoaOutlineDataSource owns its sortDescriptors field, so it must release it
in its dealloc() (and also initialize it in its init(), no idea how did it
work without this being done before).

Closes #16231.

comment:7 Changed 3 months ago by VZ

In 76642:

Don't leak sort descriptors array in wxDataViewCtrl in wxOSX.

wxCocoaOutlineDataSource owns its sortDescriptors field, so it must release it
in its dealloc() (and also initialize it in its init(), no idea how did it
work without this being done before).

Closes #16231.

comment:8 Changed 3 months ago by johnr

A nicer solution than mine. Thanks.

I did think it was worth re-setting the sortDescriptors in ClearColumns() but I think the way we release and rebuild the outlineView in ClearColumns probably does this anyway.

Note: See TracTickets for help on using tickets.