All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0024412PackagesLCLpublic2013-05-10 11:552014-03-06 16:00
ReporterLuca Olivetti Assigned ToMattias Gaertner  
PrioritynormalSeveritymajorReproducibilityhave not tried
Status resolvedResolutionfixed 
Platformi386OSWindows 
Product Version1.0.8 
Summary0024412: Memory leak in dbgrids.pas with multiselect
DescriptionDue to the change from TBookmarkStr to TBookmark, using multiselect in a dbgrid causes a memory leak for each new selection.
The attached project shows the problem.
I will also attach a patch to solve the problem (there is still a single leak but it doesn't depend on how many multiple selections are made).
TagsNo tags attached.
Fixed in Revision42912
LazTarget-
Widgetset
Attached Files

Activities

Luca Olivetti

2013-05-10 11:55

reporter  

published.7z (14,090 bytes)

Luca Olivetti

2013-05-10 11:55

reporter  

dbgrids.patch (1,325 bytes)    
--- C:/Documents and Settings/luca/Configuraci�n local/Temp/dbgrids.pas-revBASE.svn000.tmp.pas	Thu Feb 21 03:28:42 2013
+++ C:/laz_1_0/lcl/dbgrids.pas	Fri May 10 11:31:13 2013
@@ -1068,6 +1068,8 @@
 
     if MultiSel and not (dgMultiSelect in FOptions) then begin
       FSelectedRows.Clear;
+      if FKeyBookmark<>nil then
+        FDatalink.DataSet.FreeBookmark(FKeyBookmark);
       FKeyBookmark:=nil;
     end;
 
@@ -2005,7 +2007,8 @@
           exit;
         end;
         FKeySign := 0;
-      end;
+      end else
+        FDatalink.DataSet.FreeBookmark(CurBookmark);
 
       n := 4*Ord(FKeySign>=0) + 2*Ord(ADown) + 1*Ord(AStart);
       case n of
@@ -2322,6 +2325,8 @@
     else
       begin
 
+        if FKeyBookmark<>nil then
+          FDatalink.DataSet.FreeBookmark(FKeyBookmark);
         FKeyBookmark:=nil; // force new keyboard selection start
         SetFocus;
 
@@ -3249,6 +3254,8 @@
     if SelCurrent then
       SelectRecord(true);
   end;
+  if FKeyBookmark<>nil then
+    FDatalink.DataSet.FreeBookmark(FKeyBookmark);
   FKeyBookmark:=nil;
 end;
 
@@ -3885,7 +3892,8 @@
       // store it here as pointer
       FList.Insert(Index, Bookmark);
       FGrid.Invalidate;
-    end;
+    end else
+      FDataset.FreeBookmark(Bookmark);
   end;
 end;
dbgrids.patch (1,325 bytes)   

Jesus Reyes

2013-05-11 07:07

developer   ~0067633

Tried your test project with both Lazarus trunk (1.1) + fpc trunk (2.7.1) and lazarus fixes (1.0.9) + fpc fixes (2.6.3) and I couldn't reproduce the problem.

What versions are you using?

Luca Olivetti

2013-05-12 11:34

reporter   ~0067666

I'm using lazarus 1.0.8 and fpc 2.6.2.
Unless newer fpc versions changed TBookark from pointer to a managed type, the problem should still be there in trunk, since I cannot see the calls to freebookmark that I added in my patch.
Are you sure you're looking at the result of heaptrc? (under linux it's not visible unless you run lazarus from a console, under windows, with no environment redirecting it to a log file, you should see a dialog box with the errors).

Luca Olivetti

2013-05-12 12:59

reporter   ~0067668

I just checked again under linux and I cannot see the output from heaptrc even if I run lazarus from a terminal, I can only see it if I run the executable directly.
The HEAPTRC variable I set in run->run parameters->environment seems to have no effect.

Luca Olivetti

2013-05-12 13:01

reporter   ~0067669

This is the output from heaptrc under linux (same under windows BTW):

Heap dump by heaptrc unit
14029 memory blocks allocated : 732683/769608
14023 memory blocks freed : 732587/769512
6 unfreed memory blocks : 96
True heap size : 196608
True free heap : 195648
Should be : 195744
Call trace for block $00007F71FC791AC0 size 16
  $000000000079647D line 2043 of dbgrids.pas
  $0000000000795DA9 line 2145 of dbgrids.pas
  $0000000000640B23 line 5564 of include/wincontrol.inc
  $0000000000640DE7 line 5695 of include/wincontrol.inc
  $0000000000643DF2 line 6986 of include/wincontrol.inc
  $000000000046F211
  $000000000063FA76 line 5322 of include/wincontrol.inc
  $00000000007404F5 line 4487 of grids.pas
Call trace for block $00007F71FC791A20 size 16
  $00000000007963F2 line 2037 of dbgrids.pas
  $0000000000795DA9 line 2145 of dbgrids.pas
  $0000000000640B23 line 5564 of include/wincontrol.inc
  $0000000000640DE7 line 5695 of include/wincontrol.inc
  $0000000000643DF2 line 6986 of include/wincontrol.inc
  $000000000046F211
  $000000000063FA76 line 5322 of include/wincontrol.inc
  $00000000007404F5 line 4487 of grids.pas

There are more but are the same 2 alternating.
Line 2037 and 2043 are calls to SelectNext, but the leak happens inside the procedure (that's why in the original report I said that heaptrc doesn't show the correct line, compounded by the fact that I was using an external debug file, so I could only see the line numbers by using the "info line" command in gdb).

Luca Olivetti

2013-05-12 22:45

reporter   ~0067676

I looked at svn and, while in fpc trunk TBookmark=TBytes (so that could mask the problem), in fpc fixes is still a pointer, so I don't understand why you cannot reproduce it.

Luca Olivetti

2013-05-13 09:37

reporter  

dbgrids2.patch (2,194 bytes)    
--- C:/Documents and Settings/luca/Configuraci�n local/Temp/dbgrids.pas-revBASE.svn000.tmp.pas	Thu Feb 21 03:28:42 2013
+++ C:/laz_1_0/lcl/dbgrids.pas	Mon May 13 09:34:48 2013
@@ -1066,10 +1066,12 @@
 
     inherited Options := OldOptions;
 
     if MultiSel and not (dgMultiSelect in FOptions) then begin
       FSelectedRows.Clear;
+      if FKeyBookmark<>nil then
+        FDatalink.DataSet.FreeBookmark(FKeyBookmark);
       FKeyBookmark:=nil;
     end;
 
     EndLayout;
   end;
@@ -1724,10 +1726,12 @@
 
 procedure TCustomDBGrid.LinkActive(Value: Boolean);
 begin
   if not Value then begin
     FSelectedRows.Clear;
+    if FKeyBookmark<>nil then
+      FDatalink.DataSet.FreeBookmark(FKeyBookmark);
     RemoveAutomaticColumns;
   end;
   LayoutChanged;
 end;
 
@@ -2003,11 +2007,12 @@
           else
             FKeySign := -1;
           exit;
         end;
         FKeySign := 0;
-      end;
+      end else
+        FDatalink.DataSet.FreeBookmark(CurBookmark);
 
       n := 4*Ord(FKeySign>=0) + 2*Ord(ADown) + 1*Ord(AStart);
       case n of
         0,6,8..11:
           begin
@@ -2320,10 +2325,12 @@
     gzFixedCells, gzFixedCols:
       doInherited;
     else
       begin
 
+        if FKeyBookmark<>nil then
+          FDatalink.DataSet.FreeBookmark(FKeyBookmark);
         FKeyBookmark:=nil; // force new keyboard selection start
         SetFocus;
 
         P:=MouseToCell(Point(X,Y));
         if Gz=gzFixedRows then
@@ -3247,10 +3254,12 @@
     if SelectedRows.Count>0 then
       SelectedRows.Clear;
     if SelCurrent then
       SelectRecord(true);
   end;
+  if FKeyBookmark<>nil then
+    FDatalink.DataSet.FreeBookmark(FKeyBookmark);
   FKeyBookmark:=nil;
 end;
 
 function TCustomDBGrid.NeedAutoSizeColumns: boolean;
 begin
@@ -3883,11 +3892,12 @@
     if AValue then begin
       // the reference count of Bookmark was increased above, so it is save to
       // store it here as pointer
       FList.Insert(Index, Bookmark);
       FGrid.Invalidate;
-    end;
+    end else
+      FDataset.FreeBookmark(Bookmark);
   end;
 end;
 
 procedure TBookmarkList.CheckActive;
 begin
dbgrids2.patch (2,194 bytes)   

Luca Olivetti

2013-05-13 09:47

reporter   ~0067678

dbgrids2.patch supersedes the previous one and (hopefully) gets rid of the last remaining leak.
Since it's against 1.0.8 and it probably doesn't apply to trunk/fixes, here's a description of what it does:

1) calls Freebookmark before each line setting FKeyBookmark to nil
2) calls Freebookmark(FKeyBookmark) in TCustomDBGrid.LinkActive(false) if FKeyBookmark<>nil
3) calls Freebookmark(CurBookmark) in SelectNext if it's not the same as FKeyBookmark (i.e., it's not been stored in FKeyBookmark) [*]
4) calls Freebookmark(Bookmark) in TBookmarkList.SetCurrentRowSelected if the bookmark has not been found and Value is false (i.e. the bookmark is not going to be stored in the list)

[*]I don't completely understand the purpose of FKeyBookmark (yes, it's to extend the selection, but I don't think it's working properly), but FKeySign is always going to be 0.

Luca Olivetti

2013-06-13 15:02

reporter   ~0068276

Still happens with lazarus 1.0.10

Luca Olivetti

2013-08-26 18:37

reporter   ~0069544

Still valid with lazarus 1.0.12

Luca Olivetti

2013-09-23 11:08

reporter   ~0070269

Still valid with lazarus 1.1.99pre, the patch still applies (with some offset).

Mattias Gaertner

2013-09-23 11:23

manager   ~0070273

About the patch:
Why do you sometimes check if the freed bookmark is nil and sometimes not?
Why do you sometimes free a bookmark without setting the variable to nil?

Luca Olivetti

2013-09-23 11:46

reporter   ~0070274

It's explained in comment #67678, anyway:

Regarding the first question, the only time I don't set FKeyBookmark to nil is in LinkActive(false), when the FKeyBookmark isn't going to be used anymore (I think)). It would do no harm to set it to nil even there though.

Regarding the second question, in both cases it's a local variable that's not used after the call to FreeBookmark.

Mattias Gaertner

2013-09-23 12:08

manager   ~0070275

Thank You.

Stratis Aravias

2014-03-06 15:13

reporter   ~0073515

i detected leak also in
version #: 1.0.14
FPC Version: 2.6.2
SVN Revision: 43446
i386-win32-win32/win64

Stratis Aravias

2014-03-06 16:00

reporter   ~0073517

i tried to apply the dbgrids2.patch in my version (see note 0073515), and seems that it eleminates the leak at a first look...

Issue History

Date Modified Username Field Change
2013-05-10 11:55 Luca Olivetti New Issue
2013-05-10 11:55 Luca Olivetti File Added: published.7z
2013-05-10 11:55 Luca Olivetti File Added: dbgrids.patch
2013-05-11 02:09 Jesus Reyes Assigned To => Jesus Reyes
2013-05-11 02:09 Jesus Reyes Status new => assigned
2013-05-11 07:07 Jesus Reyes LazTarget => -
2013-05-11 07:07 Jesus Reyes Note Added: 0067633
2013-05-11 07:07 Jesus Reyes Status assigned => feedback
2013-05-12 11:34 Luca Olivetti Note Added: 0067666
2013-05-12 11:34 Luca Olivetti Status feedback => assigned
2013-05-12 12:59 Luca Olivetti Note Added: 0067668
2013-05-12 13:01 Luca Olivetti Note Added: 0067669
2013-05-12 22:45 Luca Olivetti Note Added: 0067676
2013-05-13 09:37 Luca Olivetti File Added: dbgrids2.patch
2013-05-13 09:47 Luca Olivetti Note Added: 0067678
2013-06-13 15:02 Luca Olivetti Note Added: 0068276
2013-08-26 18:37 Luca Olivetti Note Added: 0069544
2013-09-23 11:08 Luca Olivetti Note Added: 0070269
2013-09-23 11:23 Mattias Gaertner Note Added: 0070273
2013-09-23 11:46 Luca Olivetti Note Added: 0070274
2013-09-23 12:08 Mattias Gaertner Fixed in Revision => 42912
2013-09-23 12:08 Mattias Gaertner Note Added: 0070275
2013-09-23 12:08 Mattias Gaertner Status assigned => resolved
2013-09-23 12:08 Mattias Gaertner Resolution open => fixed
2013-09-23 12:08 Mattias Gaertner Assigned To Jesus Reyes => Mattias Gaertner
2014-03-06 15:13 Stratis Aravias Note Added: 0073515
2014-03-06 16:00 Stratis Aravias Note Added: 0073517