View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0024412 | Packages | LCL | public | 2013-05-10 11:55 | 2014-03-06 16:00 |
Reporter | Luca Olivetti | Assigned To | Mattias Gaertner | ||
Priority | normal | Severity | major | Reproducibility | have not tried |
Status | resolved | Resolution | fixed | ||
Platform | i386 | OS | Windows | ||
Product Version | 1.0.8 | ||||
Summary | 0024412: Memory leak in dbgrids.pas with multiselect | ||||
Description | Due 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). | ||||
Tags | No tags attached. | ||||
Fixed in Revision | 42912 | ||||
LazTarget | - | ||||
Widgetset | |||||
Attached Files |
|
|
|
|
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; |
|
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? |
|
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). |
|
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. |
|
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). |
|
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. |
|
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 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. |
|
Still happens with lazarus 1.0.10 |
|
Still valid with lazarus 1.0.12 |
|
Still valid with lazarus 1.1.99pre, the patch still applies (with some offset). |
|
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? |
|
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. |
|
Thank You. |
|
i detected leak also in version #: 1.0.14 FPC Version: 2.6.2 SVN Revision: 43446 i386-win32-win32/win64 |
|
i tried to apply the dbgrids2.patch in my version (see note 0073515), and seems that it eleminates the leak at a first look... |
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 |