Is it a bug to free a form that has MainForm as the owner?

Question

In our existing code we have a bunch of these where a form is created with MainForm as the owner (say instead of nil) yet we free it explicitly.

function SomeFunc(): Boolean;
var
  form: TMyForm; // subclasses TForm
begin
  with TMyForm.Create(Application.MainForm) do
    try
      ShowModal;
      Exit(True);
    finally
      Free;
    end
end;

Can this cause any form of bug or crash, or is it safe?

I can't seem to sort it out by reading the doc:

http://docwiki.embarcadero.com/Libraries/Berlin/en/System.Classes.TComponent.Owner

http://docwiki.embarcadero.com/Libraries/Berlin/en/System.Classes.TComponent.Create

Erdrich

What is the purpose of the exit ? Is it really needed here ? – Bradberry

@Bradberry I am not sure what to answer... is it making my question less clear? Less readable? Harder to grasp? Is it like noise? – Erdrich

You should FreeAndNil() any objects you create your self, it does not matter who its parent is. So yes this is safe and even recommended because if you dont than the memory only gets release when your application closes – Bradberry

I dont know what the exit should do there. Why do you think you need to call exit there ? – Bradberry

@GuidoG: why are you asking and why is it so important that I give you an answer? It's there, that is all. – Erdrich

It's not a bug. You can save a few CPU cycles by passing nil as owner though. – Mayorga

If you pass an owner at construction, the owner will take care of the lifecycle of the form (ie it will be freed at owner destruction). But if you are controlling the lifecycle, so you should pass nil in the constructor. – Stinger

@GuidoG: Exit(True) is just another way to say Result := True; or SomeFunc := True; Rember that the finally block will always be executed. – Stinger

@hellyea, small observation, you don't initialize the function return variable, I hope you are aware of that :) – Stinger

@Bradberry FreeAndNil() of locally created & destroyed object is just as unnecessary as passing an owner. More importantly, bearing in mind OP code, you should never FreeAndNil() an uninitialised local variable. – Belcher

@Stinger Function's return variable doesn't need to be initialised. OP function always does one of: either raises exception (i.e. doesn't return any result) or returns True; (Since only one result is possible, this shouldn't be a function.) – Belcher

bullcrap. this is exactly why I gave up on this site two times already. too little reward for enduring the trolling of others. my question is as clear as it can get regardless of Exit(True) or the unused local variable. – Erdrich

I don't see problems with Exit(True) here??? On a side note, does my answer addresses your question? – Stinger

@Stinger I know, SO is painful. this will once again not work out. too much annoyances for the reward. it had been over 2 yrs since I quit the last time. perhaps It ll take 3 yrs before I get back on this. – Erdrich

@hellyea I'm taking my time to offer some helpful general advice and this is how you show your gratitude????? – Belcher

gratitude for what? I know all that stuff already. you just presume I could use your unrequested advice. get over yourself. – Erdrich

@Stinger & hellyea The function always returns the same result*. So should the caller check the result? What does the result mean? I made the point the code doesn't have any bugs, but it's confusing because there are a bunch of unanswered questions for using a particular template that avoids thinking about the consequences to the caller. – Belcher

@hellyea If you know to avoid those pitfalls, then how are they relevant to the question? If you don't want my help: fine, ignore it - there's no need to be rude! – Belcher

@hellyea Regardless if you use exit(true) or Result := true or if your result is initialized or not, to answer your question I would say this. If you want to free objects yourself than do not pass an owner but pass nil. Its a simple rule. Your code will not crash though. Alexandre's answer is perfect although I always do Free in stead of Release; – Bradberry

Answer 1

Look at the sourcecode and you can answer this question yourself!

TForm inherits deep down from TComponent, if we look at the destructor of TComponent, we see this (in DelphiXE7 at least):

destructor TComponent.Destroy;
begin
  Destroying;
  RemoveFreeNotifications;
  DestroyComponents;
  if FOwner <> nil then FOwner.RemoveComponent(Self);
  FObservers.Free;
  inherited Destroy;
end;

There are 2 important lines here:

• DestroyComponents

This will Destroy all owned components at destruction before the owner itself is destroyed.

• if FOwner <> nil then FOwner.RemoveComponent(Self);

This notifies the owner that the object he owns no longer exists and that it must be removed from the owners componentlist.

so in your case Application.MainForm will own your TMyForm instance, but at destruction it will be gone from the Main Form componentlist.

To make a long story short, Your code is perfectly fine and will not crash. But to make it clear that you are controlling the lifecycle of the component, you should pass nil as the Owner in the constructor. And as Sertac Akyuz already mentioned in the comments, you will avoid the call to FOwner.RemoveComponent(Self); which will save some CPU cycles...

Stinger

Answer 2

It is not a bug, but it is definitely a code smell. The preferred way of doing this (modal form's create-show-destroy cycle) is:

  function SomeFunc(): Boolean;
  var
    form: TMyForm;
  begin
    form := TMyForm.Create(nil);
    try
      Result := form.ShowModal = mrOk; // this can vary, of course
    finally
      form.Release;
    end
  end;

Please notice that I'm not calling TForm.Free directly. Instead I'm calling TForm.Release method which is safer. From RAD Studio help file:

Release does not destroy the form until all event handlers of the form and event handlers of components on the form have finished executing. Release also guarantees that all messages in the form's event queue are processed before the form is released. Any event handlers for the form or its children should use Release instead of Free (Delphi) or delete (C++). Failing to do so can cause a memory access error.

Communicable

The documentation quote you included recommends 'Release' to be used from "any event handlers for the form or its children". The place you call it is neither. – Mayorga

@SertacAkyuz: Read it again. It basically says: "from within an event handler in that form, you must use Release, not Free". It doesn't say that you are forbidden to use it in other scenarios. I think it is pretty clear. – Communicable

Re3lease can also be potentially dangerous. If someone adds some code that will pump windows messages - such as a message dialog, the form may be freed before you are finished with it. I have had to debug quite a bit of code like this. Use Release when it is required, not out of reflex. Remember Release posts a message that will be handled as soon as the messages are pumped. – Gally

There is nothing wrong posting a message to a window in order to close/destroy it. This is a standard way of doing it in many scenarios, outside the Delphi world. If you have problems with Release, it's because you have a bug somewhere else. It is not a problem with this pattern. – Communicable

@GerryColl - So, to simplify, Release is potentially dangerous only if you use Application.ProcessMessages (I don't mention other more "brutal"/direct ways of pumping the queue) in your code. Right? – Connie

Just as an addendum, note that TForm.Close will also call Release in the end. – Connie

Answer 3

As far as I know, this is perfectly safe to do if you ensure that your main form isn't freed while your child form is shown, that would get messy. I'd expect an "access violation" or "Invalid pointer operation" on the ChildForm.Free, that is, if it makes it that far.

In a small test case, the application actually remained stuck in the modal loop of the (now destroyed) second form.

But since freeing the main form is a job better left to TApplication, I think we can conclude using the main form as the owner of your child form is perfectly fine.

For the lifetime management aspect of the question, see whosrdaddy's answer.

Fontainebleau