[SOLVED] Access violation when freeing a dynamically created Form

[wp]

as long as I don't forget to check 'memoopen' before accessing FormMM

It 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.

I am attaching another demo, this time with two instances of TFormMM which are allowed to exist simultaneously. The main form runs a timer which checks which of the two forms is currently open. In this case the main form needs to know the instances of TFormMM and it must know whether they are closed or not. In the current approach of destroying a form when it is closed the latter information is provided by the form variable being nil or not. For this to happen the program must be able to set the form variables to nil. In this demo the main form sets the OnClose handler of TFormMM and thus is able to set the related form variable to nil. If such a precaution would not have been made the program would crash when the timer event handler accesses the form variable although the form already might have been closed and destroyed.

I understand that you refrain from changing the code used by many projects. Of course, you can keep everything as it is and only empty the Free_Memo procedure. But it is a time-bomb, and since you plan to use this also in future projects you should fix it. You could create a new version of your LCL_lib and put the improved code in there. If you name the new version LCL_lib2 and use it only in the new projects you can keep the old projects untouched. Slowly, over time, you could adapt the old projects one by one by replacing unit LCL_lib by LCL_lib2. After the last old project has been migrated you can delete the outdated LCL_lib version from your code base.

[Hartmut]

Hello wp, thanks again for your new reply and your new demo and the time you spent to help me.

Again I had a deep look into your demo and ran it a couple of times. Again I am impressed, that you found a solution for the "only 1 global variable" problem. It works.

Please let us compare the advantages of this new solution with the "old solution" = only deleting the FreeAndNil() in free_Memo():

• in both cases I must check a global variable before accessing FormMM (either if FormMM <> nil or if memoopen is True). I see there no difference.
• of course you are right: I may not e.g. call FormMM.Show() without adjusting memoopen. But this would be a bug, like forgetting to check the global variable before accessing FormMM. And if I ever would need things like this, my philosophy would always be, to place those things - especially if they are dangerous - always inside the class, where it should be handled correctly, and where I had to implement (and test) it only once, and then I had it ready-to-use for all other programs, instead of to implement (and test) such things outside the class, again and again for every new program. As I wrote, this common class can do much more than you see in this minimalized demo, because of this philosophy. So in my eyes of course some advantage of your new solution exists, but please see the price:
• in every program, after every call of init_Memo(), I must set the FormMM.OnClose-Event
• later added: later I realized, that therefor the FormClose-Event of the main program must be accessible. But what if the init_Memo() resides in another unit? And as we see later, the FormClose-Event of the main program must access all FormMM-variables... I had a couple of "circular references errors" in my life and am not keen on to support the next one (now or later in the future).
• later added: and what is, if this other unit which calls init_Memo() is a common unit and not only a part of the current program? In a common unit I cannot include all main programs with 'uses'... I beleave, that someone like you could invent a solution for this too, but again we have an increase of effort and complexity and the risk for bugs and less flexibility.
• in every program I have to add a FormClose-Event with code for every possible Memo-Form
• if the main program already has a FormClose-Event - or if I need to add one later - I have to separate both
• 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?
• the FormClose-Event must be able to access all FormMM-variables. Therefor they must be global (until now the most programs use local variables) and what if they reside in another unit? Yes, should be solvable, but again extra effort.
• I prefer, that the usage of a common class or procedure is as simple as possible and as safe as possible (e.g. things that could be forgotten) and that it adds as less restrictions as prossible. For 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.
• 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 program
• and to have 2 classes in use (LCL_lib and LCL_lib2) with nearly the same code would be in my eyes one big disadvantage more: as long as you live with both, you must always keep in mind, which one works how, and where are exactly the differences in how to handle them correctly. And adapting old projects to the new class "slowly, over time" would mean, that I had to dive deep enough into both classes multiple times, once for each program, to find all the things, which have to be adapted. This way would increase the sum of total effort (and increase the risk for inserting new bugs).

If we look at this list, please understand, that the only benefit, that FormMM would be nil after usage, in my eyes is a "nice to have", yes, but much smaller than all the disadvantages, which I see. I don't want to pay this high price for this small benefit.

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.

So I decided to choose now the easy solution, to only delete FreeAndNil() in free_Memo(), because no one contradicted, that this would solve my Access violation problem.
 
Again many, many thanks to all who helped me with this difficult problem. Especially to wp, who was in the 1st line and who found out, where and why this problem was caused in the first place. Again I learned a lot from you.
 
This is a great forum.

Hartmut

[RAW]

hmmm funny:

Code: Pascal  [Select][+][-]

1. // OnClose:
2. CloseAction:= caFree;
3. Form:= Nil;
4.  
5. // looks the same to me ...
6. Form.Release;
7. Form:= Nil;
8.  

As I said before, that's the best way to close any form ... 

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 both

The 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 ... 

...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 program

Yeah, 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).

Again: your program/problem -  your choice ...

[Hartmut]

Code: Pascal  [Select][+][-]

1. // OnClose:
2. CloseAction:= caFree;
3. Form:= Nil;
4.  
5. // looks the same to me ...
6. Form.Release;
7. Form:= Nil;
8.  

As I said before, that's the best way to close any form ... 

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?

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.

Where is a real difference between
   if FormMM <> nil then FormMM.MM.Lines.Add(...);
and   
   if memoopen then FormMM.MM.Lines.Add(...);
I don't see one.

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 ... 

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).

...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 ... 

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)

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 program

Yeah, 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).

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.

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.

[Hartmut]

Because this Topic got very long here is a short summary:

How / where did the problem (which leaded to an Access violation) occur in the first place?
 - when ABORT-Button is not pressed, FormMM.Close() is called in line 46 of Unit1. FormMM.Close calls ReleaseComponent, which does not destroy the form immediately, but drops the request for it in the message queue.
 - when ABORT-Button is pressed, FormMM.Close() is called in line 57 of unit LCL_lib. Again FormMM.Close does the same as before. But now in the calling procedure (TForm1.ButtonClick) there is in line 40 of Unit1 an Application.ProcessMessages, which handles all commands accumulated in the message queue - among them the request to destroy FormMM.
So in the 1st case, when free_Memo() is called in line 48 of Unit1, the request to destroy FormMM has not yet executed and no Access violation occurs.
But in the 2nd case, when the same free_Memo() is called, the request to destroy FormMM has been already executed (because of Application.ProcessMessages) and the Access violation occurs.
This was found out by wp. Thanks to him again.

How does the solution work, which I choosed?
Because of 'CloseAction:=caFree' in line 67 of unit LCL_lib the FreeAndNil() in free_Memo() = line 80 of unit LCL_lib is not neccessary, because "the form will be destroyed automatically whenever the user closes the form or when FormMM.Close is called". So I only deleted this one line - that's all :-)

Addendum: meanwhile I could solve the 2 Heaptrc-problems (see end of reply #9) and made a couple of tests and the results were always "everything ok" :-)

What about an improvement and alternative solutions?
This solution has a (in my eyes not very big) disadvantage, because FormMM is not set to nil after usage. This is not very "clean". And you cannot do things like 
   if FormMM <> nil then FormMM.MM.Lines.Add(...);
But fortunately my old class 'TFormMM' already provides a global control variable (in this demo called 'memoopen'), which can do exactly the same job by
   if memoopen then FormMM.MM.Lines.Add(...);
   
There have been a couple of different attempts by wp, to set automatically FormMM to nil after usage. But this was quite difficult to achieve and had (in my eyes) a long list of disadvantages (see reply #31) like much additional effort and nameable more complexity and less flexibility, how this common class could be used everywhere.

So I decided to choose now the much easier solution.

Again many thanks to all who helped me with this difficult problem. I learned a lot.

[RAW]

Where is a real difference between
   if FormMM <> nil then FormMM.MM.Lines.Add(...);
and   
   if memoopen then FormMM.MM.Lines.Add(...);
I don't see one.

Easy: the second way needs 2 global variables and the first way only one, so the first way is less complex and easier and safer ... !!!
Also the boolean "memoopen" needs to be set to false or true exactly as the NIL-way and so that's a  bad design!

if pActiv <> nil then pActiv^:=false; {reset a flag that the Form is closed}

by the way: the demo program is working very fine if you choose to delete "CloseAction:=caFree;" or "FreeAndNil(FormMM);"!!!
The whole thing is all about destroying a form that is already destroyed... that's it... very easy to see... no big deal!

[Hartmut]

Hello RAW,
why do you continue to argue, after the decision has fallen? Can't you accept / respect this?

Where is a real difference between
   if FormMM <> nil then FormMM.MM.Lines.Add(...);
and   
   if memoopen then FormMM.MM.Lines.Add(...);
I don't see one.

Easy: the second way needs 2 global variables and the first way only one, so the first way is less complex and easier and safer ... !!!

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.

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:
 - memoopen is set automatically inside the class and it is absolute easy to do this
 - 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.
For this reasons, I prefer the "design" of my old class.

Obviously we have different opinions about what counts more in the question, what - in sum - is a "better" design or not. This should not be a problem... Fortunately it's my job to decide, which solution I'll use.

by the way: the demo program is working very fine if you choose to delete "CloseAction:=caFree;" or "FreeAndNil(FormMM);"!!!
The whole thing is all about destroying a form that is already destroyed...

Did you read nothing of this Topic?
1) Why "CloseAction:=caFree" is neccessary was written multiple times!
2) And many people have already written uncountable times that to delete FreeAndNil(FormMM) is a solution! Why dou you repeat things, that we all know for long?

I don't want to continue this discussion, it makes no sense.

[ASBzone]

Why throw the child out with the bath? (don't know if this German slogan is understandable after translating it into English)

The English version of this idiom is "Why throw out the child/baby with the bath water?"

Close enough. 

[Hartmut]

Thanks, ASBzone.

[RAW]

I don't argue at all, I put things right! A bad structure is not an opinion ...

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 this

It doesn't matter if it's easy, it's not necessary. Even this simple and funny FUNCTION can handle it without any global variable:

Code: Pascal  [Select][+][-]

1. Function WndExists(Wnd:TForm):Boolean;
2. Begin
3.   Result:= False;
4.   Try
5.    Wnd.Caption;
6.    Result:= True;
7.   Except
8.   End;
9. End;
10.  
11. // outside the IDE this works fine
12. // inside it produces an exception anyway if Wnd is destroyed

- 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...


Just to make things clear !