View Issue Details

IDProjectCategoryView StatusLast Update
0036746PatchesLCLpublic2020-03-12 10:45
ReporterC Western Assigned ToOndrej Pokorny  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx868_64OSLinux 
Product Version2.0.7 (SVN) 
Summary0036746: Removing sub menu causes crash
DescriptionUnder gtk2, if a popup menu is dynamically updated to remove a submenu using Clear on the menu item, a crash occurs (Object reference is nil). Debugging suggests the issue is with the MenuItem.Clear call, which does not fully update the associated menu structures. (I think MergedMenuItems needs to be cleared.) The attached patch fixes this for me, by calling Delete on each menu item, rather than simply emptying the list.
TagsNo tags attached.
Fixed in Revisionr62704, r62715
LazTarget-
WidgetsetGTK 2
Attached Files

Relationships

related to 0036582 resolvedOndrej Pokorny Lazarus MDI support for Windows (patch and demo included) 
related to 0036789 assignedOndrej Pokorny Lazarus MenuItem destructor optimization 
related to 0036776 closedOndrej Pokorny Lazarus Bringing up a popup menu in the IDE sometimes causes Access Violation dialog to occur. 

Activities

C Western

2020-03-01 12:53

reporter  

menu.patch (535 bytes)   
diff -uwNr '--exclude=.svn' '--exclude=Makefile' '--exclude=Makefile.fpc' '--exclude=Makefile.compiled' '--exclude=*.rsj' '--exclude=*.bak' '--exclude=*.po' lazarus/lcl/include/menuitem.inc lazarus.w/lcl/include/menuitem.inc
--- lazarus/lcl/include/menuitem.inc	2020-02-29 08:39:45.785219642 +0000
+++ lazarus.w/lcl/include/menuitem.inc	2020-03-01 11:42:40.061967881 +0000
@@ -1016,7 +1016,7 @@
   I: Integer;
 begin
   for I := Count - 1 downto 0 do
-    Items[I].Free;
+    Delete(I);
 end;
 
 function TMenuItem.HasBitmap: boolean;
menu.patch (535 bytes)   

jamie philbrook

2020-03-01 16:21

reporter   ~0121297

I think you should take a closer look at your issue, you are purposing to change the LCL which effects all other targets

 Also, your change is wrong to start with because you are just using a delete? where is the logic in that?

 I just tested this in the Win32 target and it works fine, also using the HeapTrc..

 If you are having issues with the gkt then please fix it there.

 I looked in the *.inc files and don't see anything alarming and my simple test of adding a submenus at multiple levels at that processes the clear just fine with no faults and no leaks.

 Please look in your gkt widget for the problem..

C Western

2020-03-01 19:25

reporter   ~0121299

Calling Delete (rather than Free) triggers rather more housekeeping to be done, than simply deleting entries from the Items list. I think the key thing is FMergedItems is cleared. I am not sure what this is for, but I suspect it may not be needed by the Win32 widget set, so inconsistencies in its value may not show up in testing in the Win32 widget set. Errors in the value of FMergedItems (which is what seems to trigger the observed crashes) can't be fixed in the widget set. The change should otherwise be neutral.

jamie philbrook

2020-03-01 19:56

reporter   ~0121300

I can't say what your issue is but a simple test here on the 64 and 32 bit targets does not display the problem..

infact, there is no leaks ether..

 I don't know what "DELETE" does in your case but really don't think the INDEX counter should be in the delete call.

 You should produce a minimum test app that displays your issue..

jamie philbrook

2020-03-02 02:15

reporter   ~0121301

After reviewing the existing code for "Delete" procedure you are using, I would say that you have uncovered a bug with the DELETE because DELETE does not call the destructor of the menu! All it is doing is releasing the handles and detaching the events. But the Class is still created and attached to the owner object..

  This may fix your issue but it sure causes another one and that is, this will eat up memory over time if you were to keep deleting and adding menus.

 The currently method of calling FREE on each item is correct because that is calling the destructor along with destroyhandle etc.

 However, I noticed in the destructor it is using the FreeAndNil on some pointers that never got NIL in the constructor!
 For example, The Action Event, if is calling FreeAndNil on that with no regard for a valid address. In otherwords, the constructor should be nilling this to ensure the memory is zeroed out on startup.
  
  Maybe windows Zero's the memory and Linux does not ?

 so it looks like there are issues in both ends the FREE(Destructor) and DELETE..

 Delete will not show its dirty deeds because the class is still attached to the owner component and thus it gets freed when the owner gets released. This will just use up your memory over time if you are inserting and removing items..

 the FREE on the other hand may fault if it attempts to do a FreeAndNil on a wild pointer.. due to them not being NIL in the constructor..

 Maybe someone else would like to look at this.

jamie philbrook

2020-03-02 03:01

reporter   ~0121302

Please look at the Constructor and Destructor of the TMenuItem and notice there are 2 or 3 pointer objects that are not getting set to NIL in the constructor but are being free'd using the FreeAndNil call.

  Maybe you can experiment by setting these items to NIL in the constructor, compile and see what happens...

  Because in your case it's possible the memory isn't being Zeroed when the class get's created.

 I can see this happening if for example you are creating these items dynamically (runtime) on the fly and later when you release them you are attempting to free wild memory.

 Just a thought.

Juha Manninen

2020-03-02 13:16

developer   ~0121313

Last edited: 2020-03-02 13:17

View 2 revisions

> Please look at the Constructor and Destructor of the TMenuItem and notice there are 2 or 3 pointer objects
> that are not getting set to NIL in the constructor but are being free'd using the FreeAndNil call.

They are private variables of TMenuItem class. Their memory is always zeroed by the compiler, unlike variables of a function or method.
The only question is this inside a loop:
  FreeAndNil(FMenuItemHandlers[HandlerType]);
but I believe it is OK, too.

To me the patch looks valid. I was already planning to apply it.
jamie philbrook, can you please test it on Windows. Does it cause problems.
If there are memory leaks or similar, how to fix them?

jamie philbrook

2020-03-02 23:19

reporter   ~0121328

I think its best to see an actually failing test case here from @C Western

from what he states It's ether a problem in the gkt widget or his code accessing menu classes that have been deleted.

The DELETE procedure does not call the constructor of the Tmenuitem, all it does is releases the OS handle of the item and unhooks any event calls.

 The Destructor on the other hand does free it up..

 So in short what is happening in C Western's case is the TmenuItem Class is still sitting in memory alive and well using the DELETE which would explain the reason why he can get past his problem, most likely due to some other code accessing the menuitem after the fact.

  Personally I think we need to verify a simple test case, because over here if you use the DELETE, the classes are still sitting there alive..

 So what does this mean? Using the existing method will delete the class and thus attempts to access them afterwards will results in errors,
otherwise using DELETE will simple clear the handles but leave Classes in memory unattended until the owner terminates..


 You won't see a leak with heap trace because these menus are owned and when the app terminates then the class gets free'd or the owner terminates.

 Changing the Clear operation for this reason does nothing more then hide/coverup a problem that seems to only be taking place on their end.
  This could also explain some unknown faults at app termination which I bet is also brewing.

 We need an EXample that fails..

Juha Manninen

2020-03-03 11:41

developer   ~0121337

Yes, a demo application and/or steps to reproduce the problem are needed.

C Western

2020-03-03 22:05

reporter   ~0121342

In fact I can easily reproduce the crash (and fix) in the Win32 widget set also. See attached project. To crash:

right click on form;
click off menu;
Turn on crash check box;
right click on form
project1.zip (2,839 bytes)

jamie philbrook

2020-03-03 23:20

reporter   ~0121344

Please provide a project that actually opens....

 ALl that did was give me a blank project with no forms..

 But don't worry I can look at the UNIT1.pas file././.

wp

2020-03-03 23:40

developer   ~0121345

> blank project with no forms.

This happens when you create a project with Laz-trunk due to the new file structure of lpi files. When you open the project in Laz trunk, the forms will be back. If you want to open the project in an older version you must check the box "Maximize compatibility of project files" in "Project options" > "Miscellaneous" and re-save.

jamie philbrook

2020-03-03 23:54

reporter   ~0121346

Last edited: 2020-03-04 00:08

View 2 revisions

back, I just copy and pasted the complete unit1 into a new project , duplicated your project so that the popupmenu opens with the right click on the form..
procedure TForm1.PopupMenu1Popup(Sender: TObject);
var
  MenuItem: TMenuItem;
  i: Integer;
begin
   { To crash; right click on form; click off menu; Turn on crash check box; right click on form }
  {$IFNDEF FIXCRASH}
    MenuItem1.Clear;
  {$ELSE}
    with MenuItem1 do begin
      for i := Count - 1 downto 0 do
        Delete(i);
    end;
  {$ENDIF}
  if not CheckBox1.Checked then begin
    MenuItem1.Caption := 'Several Items';
    for i := 0 to 1 do begin
      MenuItem := TMenuItem.Create(MenuItem1);
      MenuItem.Caption := 'Item '+IntToStr(i);
      MenuItem1.Add(MenuItem);
    end;
  end else begin
    MenuItem1.Caption := 'Single Item';
  end;
end;

I didn't even bother testing the DELETE because the CLEAR method works perfectly fine over here., No crashes and no leaks with HeapTrc.
No matter what key strokes I use or combination of interaction with the menu it does not crash using the CLEAR method.
May I suggest you have some other issue, since that I couldn't even open your project.

 I tried this on 2.0.6, 2.0.4, in both 32 and 64 window EXEs, I am also using 3.0.4 fpc… if that makes any difference..

Update,
   I just tried this on another Wnidows 10, this one is a part Server machine and it works fine there too...

C Western

2020-03-04 10:03

reporter   ~0121352

The error occurs with trunk on the current SVN version (of both Lazarus and FPC). Jamie - as you couldn't open my project, this implies you are not using the trunk version of Lazarus?

jamie philbrook

2020-03-04 23:57

reporter   ~0121386

I want to present to you a problem that you are creating on your own due to the fact the DELETE does not actually call the destructor of the menuitems you are creating and adding to the list, and later deleting, which actually isn't deleting the class code.. This to me is a bug that you
have uncovered which I thank you for that however, if and when it gets fixed your problem will come back..
Please drop a TPopupMenu on the form but don't add anything to it but perform this Tbutton1 click action and observe the accumulated
component counts that do not get freed up.

procedure TForm1.Button1Click(Sender: TObject);
Var
  M:TMenuItem;
begin
 M := TMenuItem.Create(PopupMenu1);
 M.Caption := 'Item '+PopupMenu1.componentCount.ToString;
 PopupMenu1.Items.Add(M); // Add to list;
 PopupMenu1.Items.Delete(0); // Now take it away, using the DELETE method instead.
 Caption := PopupMenu1.ComponentCount.Tostring; // Show owned component counts
end;

As you can see, you are now accumulating memory that will never get released until the PopupMenu1 it self gets released, nor are they accessible at this point.

 DELETE should be calling FREE, not doing what it is now otherwise a heavy app doing menu manipulation is going to tank out on heap memory.

C Western

2020-03-05 00:34

reporter   ~0121387

I agree my fix introduces a memory leak, though it wouldn't show with heaptrc as all the redundant memory will be freed when the parent popup menu is frred. The Clear loop should presumably be:
      for i := Count - 1 downto 0 do begin
        MenuItem := Items[i];
        Delete(i);
        MenuItem.Free
      end;
Delete does not call Free; this seems to be part of it's specification, but Clear does have Free as part of its specification.

C Western

2020-03-05 00:40

reporter  

menu.updated.patch (667 bytes)   
diff -uwNr '--exclude=.svn' '--exclude=Makefile' '--exclude=Makefile.fpc' '--exclude=Makefile.compiled' '--exclude=*.rsj' '--exclude=*.bak' '--exclude=*.po' lazarus/lcl/include/menuitem.inc lazarus.w/lcl/include/menuitem.inc
--- lazarus/lcl/include/menuitem.inc	2020-02-29 08:39:45.785219642 +0000
+++ lazarus.w/lcl/include/menuitem.inc	2020-03-04 23:36:17.204592280 +0000
@@ -1014,9 +1014,13 @@
 procedure TMenuItem.Clear;
 var
   I: Integer;
+  M: TMenuItem;
 begin
-  for I := Count - 1 downto 0 do
-    Items[I].Free;
+  for I := Count - 1 downto 0 do begin
+    M := Items[I];
+    Delete(I);
+    M.Free;
+  end;
 end;
 
 function TMenuItem.HasBitmap: boolean;
menu.updated.patch (667 bytes)   

jamie philbrook

2020-03-05 04:01

reporter   ~0121390

Last edited: 2020-03-05 04:03

View 2 revisions

That code is getting twisted between the Destroy and Delete. You can see where the destructor calls the REMOVE which in turn calls the Delete.
 
Delphi seems to actually delete the classes when using DELETE and REMOVE, REMOVE is to locate one via POINTER and get the index then call the DELETE via the index it found. But in the end DELPHI Deletes the complete reference using DELETE..

 Since the destructor is making a call to DELETE via REMOVE, you would need to prevent DELETE from calling the destructor in that case otherwise DELETE should be calling the destructor at the end...

 That's just my opinion but it would make it work that way, I don't see any spec's other than what my old DElphi 3 help file says about the action of DELETE and REMOVE, the help states it actually deletes the class after the needed cleanup .

Juha Manninen

2020-03-07 23:44

developer   ~0121443

Please test with r62704.
It frees the sub menuitem in Delete() and uses Delete() in TMenuItem.Clear.

jamie philbrook

2020-03-08 03:29

reporter   ~0121445

did you try to call the Free on each item directly too, to ensure there is no recursive loop taking place ?

C Western

2020-03-08 10:15

reporter   ~0121452

While I think this will fix my specific issue, looking at the on-line Delphi documentation I note that it specifically states:

TMenuItem.Delete does not free the menu item removed from the menu

TMemuItem.Clear does free the items

I suspect this means this fix could break things elsewhere. I think my updated patch provides the required behaviour

C Western

2020-03-08 10:24

reporter   ~0121453

Update: Delete must not not call Free.

TMenuItem.SetMenuIndex() essentially calls Delete, then Insert

Juha Manninen

2020-03-08 13:48

developer   ~0121457

Ok, I reverted my earlier changes and applied the modified patch.
Please test again.

C Western

2020-03-08 14:42

reporter   ~0121458

Works for me - thanks

Juha Manninen

2020-03-08 15:04

developer   ~0121460

Good! Resolving.

jamie philbrook

2020-03-08 16:11

reporter   ~0121462

Last edited: 2020-03-08 16:30

View 2 revisions

So what happens when using DELETE directly ? It leaks resources and that is the way you want to leave it?
My old Delphi docs indicates it deletes the item not simply remove it from the list and only have delete it..
Just a band aid to cover up a real problem elsewhere..

With 2.0.6 and down this problem does not exists.. So what changed ?

why don't we simply hide the current existing DELETE and implement a public one that does just that, Call the actual .FREE on it..
mean while the Clear and REMOVE can use the existing one.

EDIT:

  May I make a suggestion, could be ether have A DeleteAndFree method or make the DELETE a function instead so that it returns the class being deleted.
otherwise this makes absolutely no since at all.
 Using REMOVE would also leak memory..

 I was just reading the latest docs for Delphi and I guess their position is to just document the issue instead of fixing it... but at least what they suggest is call the FREE of the item from the list without using Remove or DElete… That's fine however, has anyone tested this to work after this change has been done ?

Juha Manninen

2020-03-08 21:47

developer   ~0121474

jamie philbrook, there is conflicting information now between you and C Western.
If you know how to fix it properly then please provide a patch.
Let's continue the discussion in forum or mailing list. Start a thread and I will join it, too, although I am not an expert on this particular case.

jamie philbrook

2020-03-08 23:00

reporter   ~0121476

I looked at the most recent help on Delphi and it seems they have taken the position of leaving it trash memory and have the coder first get the selection into a external TmenuItem, then call delete and then Free the external Item, as it is being done now with the fix that was put in place.
  I wasn't aware the DELETE was in this kind of working order. My last comment was maybe we should change the Method of DELETE to a function instead of a procedure so that it can return the currently allocated TMenuItem so one could basically free it on the fly..

 Changing it to a function should still work as a procedure too so there shouldn't be an issue..
 Example
  AMenuITme.Delete(WHichOne?).Free;
 This would ensure the Object would be freed after the fact.
 or one could simple use as a Procedure and ignore the return..

I guess I could do some mailings, I'll get back to you on this one.

Juha Manninen

2020-03-09 22:33

developer   ~0121508

> Changing it to a function should still work as a procedure too so there shouldn't be an issue..

jamie philbrook, I fulfilled your wishes in r62728!
True, it remains Delphi compatible. It is an extension.
It helps with the related issue. Please take alook and test.

jamie philbrook

2020-03-09 23:31

reporter   ~0121513

Last edited: 2020-03-09 23:33

View 2 revisions

I tested the block of code here that you put in the r62728 and it seems to work for the Delete(x).Free;

But I noticed the REMOVE method is no longer there in the mentitem.inc file unless you put it elsewhere ?

In any case I really don't think it matters since you would need the instance to reference it so that would mean that you already have it ;0

A real test would be to modified the CLEAR method to use the now modified DELETE(X).Free in the loop.

unless someone else can find an issue with that I would call it good..

Juha Manninen

2020-03-10 09:15

developer   ~0121523

CLEAR method now uses the enhanced syntax, too.
Resolving.

Ondrej Pokorny

2020-03-11 06:25

developer   ~0121546

You may NOT auto-free a TMenuItem in TMenuItem.Delete(). This is a HUGE change, don't you see? I revert it. Furthermore, it is Delphi-incompatible.

Ondrej Pokorny

2020-03-11 06:30

developer   ~0121547

Check for example r62731 - you forgot to delete Free there so it is called 2x. And this change affects all user code as well!

jamie philbrook

2020-03-11 22:20

reporter   ~0121558

Ondrej you need to find another way to merge your menus. DELETE does not automatically free the Class, its optional and only when the now a, function returns. The Return is the Class object that just got removed and handled free'd from the list. For Backwards compatibility this works as intended.
  
  For the Clear method, its states that it removes it from the list like DELETE does and then free's it.

 so this explains why earlier versions of Lazarus (Release versions) are working as designed and failing in the trunk.

 But simply put, if all you are doing is changing the parent to view the menu elsewhere, then you need to be careful about how that operates because the menu you are changing to view to does not own the Class.

 That is my point on the whole matter and btw, before the change to the CLEAR method, there was still a generated fault so I guess the menu merge was causing that too.

jamie philbrook

2020-03-11 22:43

reporter   ~0121559

I see you reverted the file back, I request that you fix the DELETE as it was fixed when finalized and put it back to a Function and have it return the class object..

This is just out of left field here..

The original problem wasn't caused here however, a bug that is unrelated to what you did was found and I wish it to be put back as I have already changed my files over here and works great..

 The DELETE should be a function that returns the object and optionally can be deleted on the fly, otherwise it leaves garbage in the memory pool. This is just to make it keep Delphi compatible but yet add an enhancement to code here..

I open another report if need be..

Ondrej Pokorny

2020-03-12 07:06

developer   ~0121564

Calling
MenuItem.Delete(ItemIndex).Free;
and
MenuItem.Items[ItemIndex].Free;
do exactly the same thing.

But I admit that the former is faster because TMenuItem.Destroy doesn't need to search through the parent's list.

Ondrej Pokorny

2020-03-12 10:45

developer   ~0121570

Last edited: 2020-03-12 10:45

View 2 revisions

Sub menus should not crash anymore. Please retest.

I reported the optimization as a new issue: 0036789

Issue History

Date Modified Username Field Change
2020-03-01 12:53 C Western New Issue
2020-03-01 12:53 C Western File Added: menu.patch
2020-03-01 16:21 jamie philbrook Note Added: 0121297
2020-03-01 19:25 C Western Note Added: 0121299
2020-03-01 19:56 jamie philbrook Note Added: 0121300
2020-03-02 02:15 jamie philbrook Note Added: 0121301
2020-03-02 03:01 jamie philbrook Note Added: 0121302
2020-03-02 12:26 Juha Manninen Assigned To => Juha Manninen
2020-03-02 12:26 Juha Manninen Status new => assigned
2020-03-02 13:16 Juha Manninen Note Added: 0121313
2020-03-02 13:17 Juha Manninen Note Edited: 0121313 View Revisions
2020-03-02 13:19 Juha Manninen Status assigned => feedback
2020-03-02 13:19 Juha Manninen LazTarget => -
2020-03-02 23:19 jamie philbrook Note Added: 0121328
2020-03-03 11:41 Juha Manninen Note Added: 0121337
2020-03-03 22:05 C Western File Added: project1.zip
2020-03-03 22:05 C Western Note Added: 0121342
2020-03-03 22:05 C Western Status feedback => assigned
2020-03-03 23:20 jamie philbrook Note Added: 0121344
2020-03-03 23:40 wp Note Added: 0121345
2020-03-03 23:54 jamie philbrook Note Added: 0121346
2020-03-04 00:08 jamie philbrook Note Edited: 0121346 View Revisions
2020-03-04 10:03 C Western Note Added: 0121352
2020-03-04 23:57 jamie philbrook Note Added: 0121386
2020-03-05 00:34 C Western Note Added: 0121387
2020-03-05 00:40 C Western File Added: menu.updated.patch
2020-03-05 04:01 jamie philbrook Note Added: 0121390
2020-03-05 04:03 jamie philbrook Note Edited: 0121390 View Revisions
2020-03-07 23:44 Juha Manninen Status assigned => feedback
2020-03-07 23:44 Juha Manninen Note Added: 0121443
2020-03-08 03:29 jamie philbrook Note Added: 0121445
2020-03-08 10:15 C Western Note Added: 0121452
2020-03-08 10:15 C Western Status feedback => assigned
2020-03-08 10:24 C Western Note Added: 0121453
2020-03-08 13:48 Juha Manninen Status assigned => feedback
2020-03-08 13:48 Juha Manninen Note Added: 0121457
2020-03-08 13:49 Juha Manninen Fixed in Revision => r62704, r62715
2020-03-08 13:49 Juha Manninen Widgetset GTK 2 => GTK 2
2020-03-08 14:42 C Western Note Added: 0121458
2020-03-08 14:42 C Western Status feedback => assigned
2020-03-08 15:04 Juha Manninen Status assigned => resolved
2020-03-08 15:04 Juha Manninen Resolution open => fixed
2020-03-08 15:04 Juha Manninen Widgetset GTK 2 => GTK 2
2020-03-08 15:04 Juha Manninen Note Added: 0121460
2020-03-08 16:11 jamie philbrook Note Added: 0121462
2020-03-08 16:30 jamie philbrook Note Edited: 0121462 View Revisions
2020-03-08 21:47 Juha Manninen Note Added: 0121474
2020-03-08 23:00 jamie philbrook Note Added: 0121476
2020-03-09 15:23 Juha Manninen Relationship added related to 0036776
2020-03-09 20:35 Juha Manninen Relationship deleted related to 0036776
2020-03-09 22:28 Juha Manninen Relationship added related to 0036776
2020-03-09 22:33 Juha Manninen Status resolved => assigned
2020-03-09 22:33 Juha Manninen Resolution fixed => reopened
2020-03-09 22:33 Juha Manninen Note Added: 0121508
2020-03-09 23:31 jamie philbrook Note Added: 0121513
2020-03-09 23:33 jamie philbrook Note Edited: 0121513 View Revisions
2020-03-10 09:15 Juha Manninen Status assigned => resolved
2020-03-10 09:15 Juha Manninen Widgetset GTK 2 => GTK 2
2020-03-10 09:15 Juha Manninen Note Added: 0121523
2020-03-11 06:25 Ondrej Pokorny Note Added: 0121546
2020-03-11 06:25 Ondrej Pokorny Assigned To Juha Manninen => Ondrej Pokorny
2020-03-11 06:25 Ondrej Pokorny Status resolved => assigned
2020-03-11 06:26 Ondrej Pokorny Relationship added related to 0036582
2020-03-11 06:30 Ondrej Pokorny Note Added: 0121547
2020-03-11 22:20 jamie philbrook Note Added: 0121558
2020-03-11 22:43 jamie philbrook Note Added: 0121559
2020-03-12 07:06 Ondrej Pokorny Note Added: 0121564
2020-03-12 10:44 Ondrej Pokorny Relationship added related to 0036789
2020-03-12 10:45 Ondrej Pokorny Status assigned => resolved
2020-03-12 10:45 Ondrej Pokorny Resolution reopened => fixed
2020-03-12 10:45 Ondrej Pokorny Widgetset GTK 2 => GTK 2
2020-03-12 10:45 Ondrej Pokorny Note Added: 0121570
2020-03-12 10:45 Ondrej Pokorny Note Edited: 0121570 View Revisions