View Issue Details

IDProjectCategoryView StatusLast Update
0025258PatchesIDEpublic2014-01-25 15:08
ReporterCyrax Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Summary0025258: [patch] [packageeditor.pas] Clear and reapply active filter when updating TTreeFilterEdit-component.
DescriptionSee summary and attached patch.

This patch will fix memory leak whenever filter is active and package editor is closed.
TagsNo tags attached.
Fixed in Revisionr43482, r43483
LazTarget-
WidgetsetWin32/Win64
Attached Files

Relationships

related to 0025233 closedMattias Gaertner Lazarus Crash, when closing package window 
related to 0025259 closedJuha Manninen Patches [patch] [treefilteredit.pas] Slight update to TTreeFilterBranch.RemoveChildrenData logic. 

Activities

Cyrax

2013-10-30 14:32

reporter  

packageeditor.pas.patch (1,983 bytes)   
Index: packager/packageeditor.pas
===================================================================
--- packager/packageeditor.pas	(revision 43341)
+++ packager/packageeditor.pas	(working copy)
@@ -422,6 +422,9 @@
 
 {$R *.lfm}
 
+Type
+  TTreeFilterEditAccess = class(TTreeFilterEdit);
+
 var
   ImageIndexFiles: integer;
   ImageIndexRemovedFiles: integer;
@@ -1773,9 +1776,14 @@
   FilesBranch, RemovedBranch: TTreeFilterBranch;
   Filename: String;
   NodeData: TPENodeData;
+  OldFilter : String;
 begin
   if LazPackage=nil then exit;
 
+  OldFilter := FilterEdit.Filter;
+  FilterEdit.Filter := '';
+  TTreeFilterEditAccess(FilterEdit).ApplyFilter(True);
+
   // files belonging to package
   FilesBranch:=FilterEdit.GetBranch(FFilesNode);
   FreeNodeData(penFile);
@@ -1816,6 +1824,7 @@
       FreeAndNil(FRemovedFilesNode);
     end;
   end;
+  FilterEdit.Filter := OldFilter;
   FilterEdit.InvalidateFilter;               // Data is shown by FilterEdit.
 end;
 
@@ -1823,11 +1832,15 @@
 var
   CurDependency: TPkgDependency;
   RequiredBranch, RemovedBranch: TTreeFilterBranch;
-  CurNodeText, aFilename: String;
+  CurNodeText, aFilename, OldFilter: String;
   NodeData: TPENodeData;
 begin
   if LazPackage=nil then exit;
 
+  OldFilter := FilterEdit.Filter;
+  FilterEdit.Filter := '';
+  TTreeFilterEditAccess(FilterEdit).ApplyFilter(True);
+
   // required packages
   RequiredBranch:=FilterEdit.GetBranch(FRequiredPackagesNode);
   FreeNodeData(penDependency);
@@ -1870,6 +1883,9 @@
     end;
   end;
   FNextSelectedPart:=nil;
+
+  FilterEdit.Filter := OldFilter;
+  TTreeFilterEditAccess(FilterEdit).ApplyFilter(True);
 end;
 
 procedure TPackageEditorForm.UpdateSelectedFile;
@@ -2406,6 +2422,8 @@
 var
   nt: TPENodeType;
 begin
+  FilterEdit.Filter := '';
+  TTreeFilterEditAccess(FilterEdit).ApplyFilter(True);
   for nt:=Low(TPENodeType) to High(TPENodeType) do
     FreeNodeData(nt);
   if PackageEditorMenuRoot.MenuItem=FilesPopupMenu.Items then
packageeditor.pas.patch (1,983 bytes)   

Mattias Gaertner

2013-10-30 17:34

manager   ~0071044

Why is a private access needed for this task?

Is there a design flaw in TTreeFilterEdit?

Juha Manninen

2013-10-30 23:25

developer   ~0071049

Last edited: 2013-10-30 23:33

View 4 revisions

> Why is a private access needed for this task?

I have the same qiestion. The patch does:
  FilterEdit.Filter := '';
  TTreeFilterEditAccess(FilterEdit).ApplyFilter(True);
but ApplyFilter is already called by Filter's write accessor SetFilter. You don't need to call ApplyFilter explicitly.

InvalidateFilter is called by ApplyFilter. When you set Filter
  FilterEdit.Filter := OldFilter;
then InvalidateFilter is not needed any more.

What caused a memory leak and how this fixes it?

Cyrax

2013-10-31 01:04

reporter   ~0071053

Looking into TCustomControlFilterEdit.SetFilter and TCustomControlFilterEdit.ApplyFilter, it seems that setting only Filter property makes it apply filter only when program/IDE is idle.

We need call ApplyFilter explicitly with parameter True to force update immediately in TPackageEditorForm.UpdateFiles, TPackageEditorForm.UpdateRequiredPkgs and TPackageEditorForm.Destroy because they are removing and creating data/nodes. Because of this, there was/is memory leak happening here.

Cyrax

2013-10-31 01:52

reporter   ~0071056

Apparently patch fixes SIGSEGV when closing package editor with active filter.

Steps to reproduce:

1. Open LCL package.
2. Apply filter.
3. Close package editor.

--
Call stack:

#0 FREENODEDATA(0x1429c800, 0x10784988) at treefilteredit.pas:337
0000001 FREENODEDATA(0x1427f700, PENFILE) at ..\packager\packageeditor.pas:1099
0000002 DESTROY(0x1427f700, 0x1) at ..\packager\packageeditor.pas:2425
0000003 FREE(0x1427f700) at ..\inc\objpas.inc:288
0000004 RELEASECOMPONENTS(0x2397e8) at include\application.inc:2314
0000005 IDLE(0x2397e8, true) at include\application.inc:388
0000006 HANDLEMESSAGE(0x2397e8) at include\application.inc:1269
0000007 RUNLOOP(0x2397e8) at include\application.inc:1401
0000008 APPRUN(0x2917c0, {Proc = {procedure (POINTER)} 0xcfdfeb0, Self = 0x2397e8}) at include\interfacebase.inc:54
0000009 RUN(0x2397e8) at include\application.inc:1389
0000010 main at lazarus.pp:128

---

Target system: win32, Free Pascal trunk 2.7.1-r25835, Lazarus trunk '1.3'-r43345
fpc make options all install sourceinstall UPXPROG=echo OPT="-gw2 -godwarfsets -gl -O- -OoNO -Xs-" COMPILER_OPTIONS="-gw2 -godwarfsets -gl -O- -OoNO -Xs-" INSTALL_PREFIX=i:\free_pascal_and_lazarus\free_pascal_and_lazarus\fpc\trunk\build\trunk_x32 REVSTR=25835 IDE=1
lazarus make options make all UPXPROG=echo OPT="-gw2 -godwarfsets -gh -gl -O- -OoNO -dHEAPTRC_WINDOW -Xs-" USESVN2REVISIONINC=0

Cyrax

2013-10-31 02:05

reporter   ~0071057

Same as in note 0071056, but this time adding and removing file from package triggers it. It is related to bug report 0025233.

---
Call stack:

#0 FREENODEDATA(0x1076f900, 0x10795b28) at treefilteredit.pas:337
0000001 FREENODEDATA(0x11119c50, PENFILE) at ..\packager\packageeditor.pas:1099
0000002 UPDATEFILES(0x11119c50) at ..\packager\packageeditor.pas:1786
0000003 UPDATEALL(0x11119c50, false) at ..\packager\packageeditor.pas:1493
0000004 SETMODIFIED(0x2c02e8, false) at ..\packager\packagedefs.pas:2516
0000005 PACKAGEEDITORFORMCLOSEQUERY(0x11119c50, 0x11119c50, true) at ..\packager\packageeditor.pas:930
0000006 CLOSEQUERY(0x11119c50) at include\customform.inc:2159
0000007 CLOSE(0x11119c50) at include\customform.inc:2069
0000008 WMCLOSEQUERY(0x11119c50, {MSG = 66622, WPARAM = 0, LPARAM = 0, RESULT = 0, WPARAMLO = 0, WPARAMHI = 0, WPARAMFILLER = {}, LPARAMLO = 0, LPARAMHI = 0, LPARAMFILLER = {}, RESULTLO = 0, RESULTHI = 0, RESULTFILLER = {}}) at include\customform.inc:2167
0000009 DISPATCH(0x11119c50, 0) at ..\inc\objpas.inc:602
0000010 WNDPROC(0x11119c50, {MSG = 66622, WPARAM = 0, LPARAM = 0, RESULT = 0, WPARAMLO = 0, WPARAMHI = 0, WPARAMFILLER = {}, LPARAMLO = 0, LPARAMHI = 0, LPARAMFILLER = {}, RESULTLO = 0, RESULTHI = 0, RESULTFILLER = {}}) at include\control.inc:2099
0000011 WNDPROC(0x11119c50, {MSG = 66622, WPARAM = 0, LPARAM = 0, RESULT = 0, WPARAMLO = 0, WPARAMHI = 0, WPARAMFILLER = {}, LPARAMLO = 0, LPARAMHI = 0, LPARAMFILLER = {}, RESULTLO = 0, RESULTHI = 0, RESULTFILLER = {}}) at include\wincontrol.inc:5327
0000012 WNDPROC(0x11119c50, {MSG = 66622, WPARAM = 0, LPARAM = 0, RESULT = 0, WPARAMLO = 0, WPARAMHI = 0, WPARAMFILLER = {}, LPARAMLO = 0, LPARAMHI = 0, LPARAMFILLER = {}, RESULTLO = 0, RESULTHI = 0, RESULTFILLER = {}}) at include\customform.inc:1432
0000013 DELIVERMESSAGE(0x11119c50, 0) at lclmessageglue.pas:112
0000014 WINDOWPROC(3606224, 16, 0, 0) at win32\win32callback.inc:2497
0000015 CUSTOMFORMWNDPROC(3606224, 16, 0, 0) at win32\win32wsforms.pp:395
0000016 gapfnScSendMessage at :0
0000017 ?? at :0
0000018 USER32!GetThreadDesktop at :0
0000019 WIN32WSFORMS_$$_ADJUSTFORMBOUNDS$TCUSTOMFORM$RECT at :0
0000020 USER32!GetThreadDesktop at :0
0000021 ?? at :0

Juha Manninen

2013-10-31 07:54

developer   ~0071059

Hmmm... How there are so many bugs now? It used to work after the filter was first implemented. It must be a cumulative effect from some other change.

Anyway, your note about ApplyFilter is correct. The Immediate parameter gets False when the Filter property is changed.
Still, the hack-accessor class is just plain wrong. It may be needed with classes that you cannot change but we have full control over the FilterEdits.
The right solution is to change visibility of ApplyFilter or (even better) make a new method that sets Filter and calls ApplyFilter.
If you make such patch, I promise to apply it today later.

I removed the hack also from your earlier code committed by Mattias. I changed visibility of the method. See r43345.
And, "if o is TFileNameItem then" works even if o is Nil. I removed the extra check.

Juha Manninen

2013-11-02 11:28

developer   ~0071100

Last edited: 2013-11-03 11:47

View 3 revisions

Cyrax, what do you think of my last comment?

[Edit] There was a crash caused by your other patch for issue 0025259. Mattias has fixed it. I cannot reproduce are crashes now.
I think this issue is obsolete.

Juha Manninen

2013-11-22 00:37

developer   ~0071473

Resolving. It apparently works now.

Cyrax

2013-11-26 20:30

reporter   ~0071624

Last edited: 2013-11-26 21:12

View 2 revisions

Nope, it still crashes whenever there is active filter on. Lazarus trunk '1.3'-r43481.

Juha, your suggestion for solution sounds correct. I have been bit busy, so I will try to make new, correct patch in following days.

EDIT:

File "0000 - editbtn.pas.patch" introduces new method ForceFilter which by default clears active filter and forces update to happen.
File "0001 - packageeditor.pas.patch" makes use of this new method.

Cyrax

2013-11-26 21:09

reporter  

0000 - editbtn.pas.patch (815 bytes)   
Index: lcl/editbtn.pas
===================================================================
--- lcl/editbtn.pas	(revision 43481)
+++ lcl/editbtn.pas	(working copy)
@@ -365,6 +365,7 @@
     constructor Create(AOwner: TComponent); override;
     destructor Destroy; override;
     procedure InvalidateFilter;
+    function ForceFilter(AFilter : String = '') : String;
     procedure StoreSelection; virtual; abstract;
     procedure RestoreSelection; virtual; abstract;
   public
@@ -1834,6 +1835,13 @@
   IdleConnected:=true;
 end;
 
+function TCustomControlFilterEdit.ForceFilter(AFilter: String): String;
+begin
+  Result := FFilter;
+  FFilter := AFilter;
+  ApplyFilter(True);
+end;
+
 function TCustomControlFilterEdit.GetDefaultGlyphName: String;
 begin
   Result := ResBtnListFilter;
0000 - editbtn.pas.patch (815 bytes)   

Cyrax

2013-11-26 21:09

reporter  

0001 - packageeditor.pas.patch (1,597 bytes)   
Index: packager/packageeditor.pas
===================================================================
--- packager/packageeditor.pas	(revision 43481)
+++ packager/packageeditor.pas	(working copy)
@@ -1770,9 +1770,12 @@
   FilesBranch, RemovedBranch: TTreeFilterBranch;
   Filename: String;
   NodeData: TPENodeData;
+  OldFilter : String;
 begin
   if LazPackage=nil then exit;
 
+  OldFilter := FilterEdit.ForceFilter;
+
   // files belonging to package
   FilesBranch:=FilterEdit.GetBranch(FFilesNode);
   FreeNodeData(penFile);
@@ -1813,6 +1816,7 @@
       FreeAndNil(FRemovedFilesNode);
     end;
   end;
+  FilterEdit.Filter := OldFilter;
   FilterEdit.InvalidateFilter;               // Data is shown by FilterEdit.
 end;
 
@@ -1820,11 +1824,13 @@
 var
   CurDependency: TPkgDependency;
   RequiredBranch, RemovedBranch: TTreeFilterBranch;
-  CurNodeText, aFilename: String;
+  CurNodeText, aFilename, OldFilter: String;
   NodeData: TPENodeData;
 begin
   if LazPackage=nil then exit;
 
+  OldFilter := FilterEdit.ForceFilter;
+
   // required packages
   RequiredBranch:=FilterEdit.GetBranch(FRequiredPackagesNode);
   FreeNodeData(penDependency);
@@ -1867,6 +1873,8 @@
     end;
   end;
   FNextSelectedPart:=nil;
+
+  FilterEdit.ForceFilter(OldFilter);
 end;
 
 procedure TPackageEditorForm.UpdateSelectedFile;
@@ -2403,6 +2411,7 @@
 var
   nt: TPENodeType;
 begin
+  FilterEdit.ForceFilter;
   for nt:=Low(TPENodeType) to High(TPENodeType) do
     FreeNodeData(nt);
   if PackageEditorMenuRoot.MenuItem=FilesPopupMenu.Items then
0001 - packageeditor.pas.patch (1,597 bytes)   

Juha Manninen

2013-11-26 23:14

developer   ~0071630

Thanks, I applied your patches.
I still did not get the crash on Windows but now I was able to reproduce on Linux. Indeed there was a bug.

Issue History

Date Modified Username Field Change
2013-10-30 14:32 Cyrax New Issue
2013-10-30 14:32 Cyrax File Added: packageeditor.pas.patch
2013-10-30 17:34 Mattias Gaertner Note Added: 0071044
2013-10-30 18:54 Juha Manninen Relationship added related to 0025233
2013-10-30 18:55 Juha Manninen Relationship added related to 0025259
2013-10-30 23:25 Juha Manninen Note Added: 0071049
2013-10-30 23:28 Juha Manninen Note Edited: 0071049 View Revisions
2013-10-30 23:29 Juha Manninen Note Edited: 0071049 View Revisions
2013-10-30 23:33 Juha Manninen Note Edited: 0071049 View Revisions
2013-10-30 23:33 Juha Manninen Assigned To => Juha Manninen
2013-10-30 23:33 Juha Manninen Status new => assigned
2013-10-31 01:04 Cyrax Note Added: 0071053
2013-10-31 01:52 Cyrax Note Added: 0071056
2013-10-31 02:05 Cyrax Note Added: 0071057
2013-10-31 07:54 Juha Manninen Note Added: 0071059
2013-11-02 11:28 Juha Manninen LazTarget => -
2013-11-02 11:28 Juha Manninen Note Added: 0071100
2013-11-02 11:28 Juha Manninen Status assigned => feedback
2013-11-03 11:28 Juha Manninen Note Edited: 0071100 View Revisions
2013-11-03 11:47 Juha Manninen Note Edited: 0071100 View Revisions
2013-11-22 00:37 Juha Manninen Note Added: 0071473
2013-11-22 00:37 Juha Manninen Status feedback => resolved
2013-11-22 00:37 Juha Manninen Resolution open => unable to reproduce
2013-11-26 20:30 Cyrax Note Added: 0071624
2013-11-26 20:30 Cyrax Status resolved => assigned
2013-11-26 20:30 Cyrax Resolution unable to reproduce => reopened
2013-11-26 21:09 Cyrax File Added: 0000 - editbtn.pas.patch
2013-11-26 21:09 Cyrax File Added: 0001 - packageeditor.pas.patch
2013-11-26 21:12 Cyrax Note Edited: 0071624 View Revisions
2013-11-26 23:14 Juha Manninen Fixed in Revision => r43482, r43483
2013-11-26 23:14 Juha Manninen Note Added: 0071630
2013-11-26 23:14 Juha Manninen Status assigned => resolved
2013-11-26 23:14 Juha Manninen Resolution reopened => fixed
2014-01-25 15:08 Cyrax Status resolved => closed