In the close action you free the form, then you free it again in another place.
So I have those questions left:Just use FormMM.Free;
- is 'FormMM:=nil' a save way to 'free' a Form and all of its memory? In my real program the dynamic Memo-Form is opened and closed many times and I want to avoid memory leaks.
- why does 'FormMM:=nil' work and FreeAndNil() cause an Access violation?
- in which other cases should I use FreeAndNil() and in which cases 'FormMM:=nil'? Is one of them generally good and the other not?
Just use FormMM.Free;I tried FormMM.Free() before I wrote this Topic, it makes no difference to FreeAndNil(FormMM), in both my small demo and my real world program, so unfortunately it does not help.
You could do FormMM := nil after the .Free but it shouldn't be necessary (if you don't check for unassigned).Because FormMM.Free can cause the Access violation, I must avoid it and having a FormMM:=nil after it I can see no improvement.
Don't use only FormMM := nil because that will lead to massive memory leaks.That's exactly what I was afraid of...
In Lazarus set Project > Project Options > Debugging and check the box for "Use heaptrc unit" to make sure you don't have leaks.Here comes the result (with the original FreeAndNil(FormMM) in free_Memo()):
If you do, you get a dialog at the end of your program of which components are not freed.
Memo_Button_Click() called
CloseMM(): ComponentCount=2
1) memoopen=FALSE ComponentCount=1
2) memoopen=FALSE ComponentCount=1
TApplication.HandleException Access violation
Stack trace:
$000000000043078D
$00000000004627F6
$0000000000542CC2
$00000000005A5CEA
$00000000005A6533
$00000000005A5BD6
$0000000000430CBF
$000000000053442D
$0000000000709722
$000000000071B781
$00007F5C36CA010D
WARNING: TButton.Destroy with LCLRefCount>0. Hint: Maybe the component is processing an event?
Heap dump by heaptrc unit
1174 memory blocks allocated : 123681/125696
1168 memory blocks freed : 123293/125288
6 unfreed memory blocks : 388
True heap size : 196608
True free heap : 195264
Should be : 195432
Call trace for block $00007F5C3827C540 size 33
$000000000045B1BD
$000000000045C6B8
$000000000045B6AD
$000000000070979C
$000000000071B781
$000000000071B781
$000000000071B781
$00000000005A5CEA
Call trace for block $00007F5C3835B2C0 size 121
$00000000004C7A56
$000000000045C67F
$000000000045B6AD
$000000000070979C
$000000000071B781
$000000000071B781
$00000000005387A0
$0000000000539F78
Call trace for block $00007F5C3827BE80 size 42
$000000000045B6AD
$000000000070979C
$000000000071B781
$000000000070979C
$000000000071B781
$000000000070979C
$000000000071B781
$000000000071B781
Call trace for block $00007F5C3835B3E0 size 128
$0000000000431B81
$00000000004627F6
$0000000000542CC2
$00000000005A5CEA
$00000000005A6533
$00000000005A5BD6
$0000000000430CBF
$000000000053442D
Call trace for block $00007F5C3827DD40 size 40
$0000000000431B81
$00000000004627F6
$0000000000542CC2
$00000000005A5CEA
$00000000005A6533
$00000000005A5BD6
$0000000000430CBF
$000000000053442D
Call trace for block $00007F5C3827DE00 size 24
$00000000004627F6
This is the 1st time I used unit Heaptrc. Does this give you more informations?Yes, it tells you there is a leak.
The problem is the Action := caFree in the OnClose handler of TFormMM. It destroys the form but does not set the formvariable to nil. You close the form in the OnClick handler ABORT button, and, because of caFree, this destroys the form but leaves FormMM non-nil which allows your free_memo to try to destry it again.I think now I understand the problem. As often I'm impressed how much details you know.
Since everything happens in a single routine, I remove the caFree and rely on the Free_memo alone.If I understand you correctly, you suggest to remove the 'CloseAction:=caFree' completely. But if I remember correctly, this command was essential neccessary to implement the feature, that if the main program terminates and the dynamic Memo-Form is still open, that this Memo-Form is automatically closed. I use this feature in a couple of programs and don't want to loose it, if possible.
Alternatively the OnClose handler of FormMM must be provided by the calling program which can simultaneously set FormMM to nil.As rvk wrotes (see below), FormMM:=nil seems not to be neccessary?
...Thanks a lot again for that infos rvk.
When using CloseAction := caFree you shouldn't free anything yourself.
So either completely remove the freeandnil or remove the caFree.
There shouldn't be any need for setting FormMM to nil.
You'r not using it anymore after doing that so it has no use.
...
An unhandled exception occurred at $0000000000437117:
EAccessViolation:
$0000000000437117
An unhandled exception occurred at $0000000000437117:
EAccessViolation:
$0000000000437117
An unhandled exception occurred at $0000000000437117:
EAccessViolation:
Speicherzugriffsfehler (Speicherabzug geschrieben)
I tried different Lazarus versions, but no difference. This was on Linux (Ubuntu 18.04).In the close action you free the form, then you free it again in another place.
Hmmm, I can't find the source lines which you mean. And from the console output (see above) for me it seems to be sure, that CloseMM(), the OnClose-Event, is always only called once, not twice.
Can you please be more detailed? Thanks.
Hi, what I meant in the first post was "use both together".
The best way to free a form is this (in my eyes):Maybe you don't need FORM:= Nil right now, but maybe in the future and FORM:= Nil isn't that much additional typing, is it? :)
I would always use RELEASE if you don't need the form anymore...
by the way:If you setup an owner for the form (SELF, APPLICATION) then let the owner free it... :)
In the close action you free the form, then you free it again in another place.Hmmm, I can't find the source lines which you mean. And from the console output (see above) for me it seems to be sure, that CloseMM(), the OnClose-Event, is always only called once, not twice. Can you please be more detailed? Thanks.
In Close_MM you do(Line 67)
I played with the program again and found two solutions.Thanks a lot for this very detailed answer. It was hard stuff for me trying to understand all. But I will start to answer beginning with your end:
I must say, however, that I do not fully understand what you want to achieve. Therefore, I think there may be simpler solutions to this problem.This common class has grown historically over the years (in reality it can do much more, I minimized it for this demo). I use this class in various programs (one of them uses 2 different Instances, which can be open at the same time). So every change, which is not only inside this class, could cause very much work to adapt (and test) it in all programs, which use this class.
Yes, from my understanding this Line 67 is the only one, where the dynamic Memo-Form is closed and from the showed Console output (see original post) this line seems to be called only once. But I understood your "In the close action you free the form, then you free it again in another place" that you mean, that there are either 2 different source lines or 1 sorce line called twice? Did I misunderstand you?
So my question to you is: if I stay with using 'CreateNew(Application)' in init_Memo() and stay with 'CloseAction:=caFree' in CloseMM() and only make free_Memo() empty: would this be a correct way, if I want to call above "main procedure" multiple times, before the program terminates?Basically ok, but not very nice. Because having an empty procedure will confuse you next year when you have forgotten why you did this. What is the problem to delete it? The compiler will tell you where it is called.
Your Abort Button (name: BTN I think) closes the form.OK, now I understand what you mean. Thanks for clarification.
At that point the form gets freed (because you set ClosAction to caFree), but not nilled.
You then break the loop in the main form and call free_Memo(FormMM);
This will call Free on an already destroyed form, hence the AV.
This is as far as I cab understand the code flow, without an actual compilable example.I attached a compilable demo at my 1st post, did you see it?
Add a debug statement in the destructor of this form, so you can see when it is called, my guess is before you call free_memo(FormMM).That sounds interesting. I was not sure, what you meant exactly: add a destructor to 'TFormMM' and hope that it's called automatically or add an OnDestroy-Event? I tried the latest:
But if the Abort-Button was not pressed, I call FormMM.Close() instead in line 46 =When ABORT is not pressed the code goes into the FormMM.Close that you mention as cited. Using the debugger to trace into the LCL code I see that Form.Close calls ReleaseComponent which does not destroy the form immediately but drops the request for it in the message queue.
if memoopen then FormMM.Close; {only if not already closed by ABORT-Button}
and in this case the AV does not occur later in free_Memo() in line 48...
I don't understand the difference. In both cases FormMM.Close(), which calls CloseMM(), is exactly called once (as we see in the Console output). Why does the variant without Abort-Button work (where FormMM.Close() is called once in line 46 of Unit1) and the variant with Abort-Button does not (where FormMM.Close() is called once in line 57 of unit LCL_lib)?
can we delete free_Memo() completely, because of 'CloseAction:=caFree', as rvk suggested in reply #8 and wp in reply #13? Would this be safe and clean, even if the dynamic Memo-Form is opened and closed multiple times, before the main program terminates? Do you agree to this?Again, yes. But you must be sure that the FormMM variable is set to nil afterwards. If you do not want to follow the idea with the "foreign" OnClose handler that I presented several posts above you could also replace the declaration of FormMM as a variable by a function which queries the forms of the screen and returns the (only) TFormMM instance or nil:
"Basically ok" sounds a bit like there are any functional restrictions... do you see any or would it be a clean and safe way? Because this solution would be much easier (see reply #16) than all other solutions, I would like very much to choose it, if it is clean and safe. Is it?So my question to you is: if I stay with using 'CreateNew(Application)' in init_Memo() and stay with 'CloseAction:=caFree' in CloseMM() and only make free_Memo() empty: would this be a correct way, if I want to call above "main procedure" multiple times, before the program terminates?Basically ok...
...but not very nice. Because having an empty procedure will confuse you next year when you have forgotten why you did this. What is the problem to delete it? The compiler will tell you where it is called.Of course I would add some lines of remarks in this empty procedure, where I had so much trouble with, including the URL of this Topic. And please allow that I contradict in this single point for 2 reasons:
When you need the form again and again there is a third option: Do not destroy it at all. Remove CloseAction := caFree and you will only hide the form, the form variable will always be valid - perfect.This is an interesting approach, thanks for that idea. If I would need this dynamic Memo-Form only in my new program, I think this would be the best (simpliest) way. But because this common class is used in various programs (and can do much more than I showed in this minimalized demo) it looks much easier to only empty the free_memo(), if this would be a clean and safe way. Is it?
But note that in this case your Show_Memo will be a problem because it will create the button and the memo again and again. Why don't you do this in the Init_Memo?I thought it would be better, to put as much code as possible in a method of a class (which could be replaced virtually, if needed) than in a global procedure. But you are right, for your third option this had to be changed.
"Basically ok" sounds a bit like there are any functional restrictions... do you see any or would it be a clean and safe way? Because this solution would be much easier (see reply #16) than all other solutions, I would like very much to choose it, if it is clean and safe. Is it?If you want an absolute statement: NO because you always can access the form variable FormMM after closing the form and you cannot even check for nil to verify that FormMM does not exist any more. But when you take measures to make sure that FormMM cannot be accessed or can be checked against NIL before access, it is safe.
When ABORT is not pressed the code goes into the FormMM.Close that you mention as cited. Using the debugger to trace into the LCL code I see that Form.Close calls ReleaseComponent which does not destroy the form immediately but drops the request for it in the message queue.Ahhh, I think now the whole oracle is completely solved :-) Now we know, where and why the problem comes in the world and why the "same" FormMM.Close() produces those different results. Thanks a lot for finding this out. I will have to write a lot of remarks in my sources after this Topic is closed, because I learned so much and don't want to loose (forget) it.
The same happens essentially when you call FormMM.Close in the MemoButtonClick procedure. But in the calling procedure (TForm1.ButtonClick) there is an Application.ProcessMessages which handles all commands accumulated in the message queue, among them the request to destroy FormMM.
Hmm, I have some difficulties to understand what you mean exactly.can we delete free_Memo() completely, because of 'CloseAction:=caFree'...?Again, yes. But you must be sure that the FormMM variable is set to nil afterwards.
If you do not want to follow the idea with the "foreign" OnClose handler that I presented several posts above you could also replace the declaration of FormMM as a variable by a function which queries the forms of the screen and returns the (only) TFormMM instance or nil:
Why not reconsider the whole structure ??? :)Thank you for that suggestion. I will have a look at your code to see how it works and what I can learn from it.
Main Form
...
Memo Window
...
Hmm, I have some difficulties to understand what you mean exactly.The key question is: Is FormMM a global variable and can it be accessed at any time? It is not in your demo program, but you said that it is in the general case. If it is not global then you may be right to control the validity of the form by status variables like "Active" or "MemoOpen" - but this must be fixed as your demo shows that this mechanism is not working correctly.
Why is it so important to set FormMM to nil?
You don't need FormMM:= Nil at all if you don't like to use ASSIGNED. Instead you can use a global variable of course.I'm happy that you see no reason, why checking my global variable wouldn't work. I hope, that wp will say the same...
The whole structure is not optimal if you ask me, but it's your program....I agree. So I will not recommend to print this class in a book for "Best programming examples" ;-)
There is only one rule you have to follow: Don't free things more than once ! :DOf course. And thanks to wp we now know, why CloseMM() does react so different and how we can react.
by the way: the easiest way would be: create every window with (Application) and use Form.SHOW and Form.HIDE and when the program ends then let the APP free everything! That's very safe!Yes, wp suggested quasi the same in reply #18 and I answered:
If I would need this dynamic Memo-Form only in my new program, I think this would be the best (simpliest) way. But because this common class is used in various programs (and can do much more than I showed in this minimalized demo) it looks much easier to only empty the free_memo()
as long as I don't forget to check 'memoopen' before accessing FormMMIt may even be that your controlling mechanism with "memoOpen" will not even notice that a new form has been opened. If I understand your code correctly you show FormMM by calling its method "Show_Memo()" which changes the internal "pActive" parameter and which, together with "memoOpen", is the inidcator that the form is still alive. What do you think will happen when you open the form with "Show" - this does not touch "pActive" and bypasses your mechanism? In essence, you can achieve the same with FormMM, but in a more reliable way - you only must make sure that it becomes nil when the form is destroyed; this is the same as resetting "memoOpen" but the latter is less reliable because the related mechanism can be bypassed.
in both cases I must check a gloabl variable before accessing FormMM (either if FormMM <> nil or if memoopen is True). I see there no difference.That's very wrong: your variable is only good for one thing and FormMM can do alot more... for example: If you like to access the TMemo you need the public variable FormMM to do that. With FormMM you can check if the form is Nil and access buttons, memos .. or whatever.
if the main program already has a FormClose-Event - or if I need to add one later - I have to separate bothThe only thing you have to do is to add three lines of code to the existing OnClose event or create one for the main form.
as I wrote above, my new program will use a couple of dynamic Memo-Forms. For each Memo-Form which I add, peace by peace, I must additionally add it in the FormClose-Event. And what if I forget this?This sounds alot like a design problem, choose another different design ... :D
...this the current class in my eyes does a good job, while the new solution adds a couple of additional effort and risks in every program, which wants to use this class.an excellent "Access-Violation"-job ... :)
I would have to adapt all existing programs, at multiple places (either now or later - the sum wouldn't become smaller) including a higher effort for testing - for each programYeah, that's the way it is, if the structure is bad, but after that, all programs are running safe...
As I said two times before (see reply #3 and #14): with 'Form.Release' I still got an Access Violation Error. It did not help. Did you read them?As I said before, that's the best way to close any form ... :)
Where is a real difference betweenQuotein both cases I must check a gloabl variable before accessing FormMM (either if FormMM <> nil or if memoopen is True). I see there no difference.That's very wrong: your variable is only good for one thing and FormMM can do alot more... for example: If you like to access the TMemo you need the public variable FormMM to do that. With FormMM you can check if the form is Nil and access buttons, memos .. or whatever.
This is a disadvantage of the new solution. If you think, this is a design problem, then it's one in the new solution, not in my "old" class (because there this extra effort and extra risk is not neccessary).Quoteas I wrote above, my new program will use a couple of dynamic Memo-Forms. For each Memo-Form which I add, peace by peace, I must additionally add it in the FormClose-Event. And what if I forget this?This sounds alot like a design problem, choose another different design ... :D
There was one bug. Yes. Because I did not know about the peril of Form.Close() which wp found out (and seems that even he didn't know before). Now this bug is solved. Why throw the child out with the bath? (don't know if this German slogan is understandable after translating it into English)Quote...this the current class in my eyes does a good job, while the new solution adds a couple of additional effort and risks in every program, which wants to use this class.an excellent "Access-Violation"-job ... :)
As said many times before: I see no need for such a huge effort (and such a long list of disadvantages, see reply #31) now.QuoteI would have to adapt all existing programs, at multiple places (either now or later - the sum wouldn't become smaller) including a higher effort for testing - for each programYeah, that's the way it is, if the structure is bad, but after that, all programs are running safe...
New program - new (better) structure is a very normal thing in the programming world, because every programmer gets better (or at least it should be like that).
Of course might come the day, where I want to add a feature to this class, which is not possible with the current structure. At this day (if it comes) I must bite in this apple and must change those things, which are then neccessary - or which make sense at this time, because I already have to adapt (and test) all using programs.
Again: your program/problem - your choice ...Yes. Please respect this.
Where is a real difference betweenEasy: the second way needs 2 global variables and the first way only one, so the first way is less complex and easier and safer ... !!!
if FormMM <> nil then FormMM.MM.Lines.Add(...);
and
if memoopen then FormMM.MM.Lines.Add(...);
I don't see one.
if pActiv <> nil then pActiv^:=false; {reset a flag that the Form is closed}
The difference is one variable, which needs 1 byte of memory, in a program of 10 or 20 MB or more. In my eyes this is nothing. I would not talk of "more complexity" because of something like this.QuoteWhere is a real difference betweenEasy: the second way needs 2 global variables and the first way only one, so the first way is less complex and easier and safer ... !!!
if FormMM <> nil then FormMM.MM.Lines.Add(...);
and
if memoopen then FormMM.MM.Lines.Add(...);
I don't see one.
Also the boolean "memoopen" needs to be set to false or true exactly as the NIL-way and so that's a bad design!But the big difference is:
by the way: the demo program is working very fine if you choose to delete "CloseAction:=caFree;" or "FreeAndNil(FormMM);"!!!Did you read nothing of this Topic?
The whole thing is all about destroying a form that is already destroyed...
Why throw the child out with the bath? (don't know if this German slogan is understandable after translating it into English)
Again many thanks to all who helped me with this difficult problem. I learned a lot.You made a careless mistake. You destroyed a already destroyed form. That is not a problem or a difficult problem at all. It's just a careless mistake. It's not hard to close or destroy a window and if you do it twice, then of course it won't work. It has nothing to do with RELEASE or FREE or FREEANDNIL.
And from my programming experience (42 years next month, with some gaps)You declared that you have 42 years programming "experience" and then you talk about things that are totally wrong, so there must be someone who put this right, especially in a forum where a lot of new programmers are around and maybe like to copy your structure because of your declared "experience"...
The difference is one variable, which needs 1 byte of memory, in a program of 10 or 20 MB or more. In my eyes this is nothing. I would not talk of "more complexity" because of something like this.A variable that is not necessary is a bad variable and can cause to further careless mistakes.
- memoopen is set automatically inside the class and it is absolute easy to do thisIt doesn't matter if it's easy, it's not necessary. Even this simple and funny FUNCTION can handle it without any global variable:
- setting FormMM to nil requires a huge effort and additional it increases the complexity and reduces the flexibility, how this common class could be used everywhere.Setting FormMM:= Nil inside OnClose isn't a huge effort and that it would reduce the flexibility is wrong too. The opposite is true, the flexibility is extended because now you can use ASSIGNED if you want and if not it changes nothing...