SIGSEGV in TStringGrid.Free when focused

[JuhaManninen]

I would like to repeat: You should never free a component that is owned by another component :

Code: [Select]

Grid := TStringGrid.Create(ScrollBox1);
...
Grid.Free;  // <- Bad!  Owned by ScrollBox1.

Maybe it did not cause the crash. Maybe the focus did it. Don't know.
Anyway, freeing a focused control feels like begging for problems. Why would you do such thing?
Maybe deleting the control first from its parent's collection first would do it cleanly.
Often the GUIs are altered by changing the visibility of controls.

mmxngg, your "bug" report was very different. There you freed a component added at design time.

Juha

[taazz]

I would like to repeat: You should never free a component that is owned by another component :

Code: [Select]

Grid := TStringGrid.Create(ScrollBox1);
...
Grid.Free;  // <- Bad!  Owned by ScrollBox1.


Although I agree that the owner is responsible to handle the clean up and life time of the objects it owns
it is desirable to be able to free those components outside the owner mechanism and it works as expected. 

Maybe it did not cause the crash. Maybe the focus did it. Don't know.
Anyway, freeing a focused control feels like begging for problems. Why would you do such thing?
Maybe deleting the control first from its parent's collection first would do it cleanly.
Often the GUIs are altered by changing the visibility of controls.

mmxngg, your "bug" report was very different. There you freed a component added at design time.

Juha

In my 20 min attempt to pin point the culprit I came to the conclusion that the focus is the problem. I do not know what or where exactly but as far as I can understand there is probably a variabe or a list somewhere that keeps a reference and does free it when it should. The search goes on this is a major problem in my book.

[mmxngg]

I used 10 minutes to figure out where the problem was and :

- FreeThenNil does not work
- The owner is not the problem because the bug remain also with Create(nil)
- Created at design or runtime...nothing changes...still does not work

In the destructor, the code searches the next control of the parent for set the new focus and takes an handle that is already freed. Moving the TCustomGrid inherited destroy at the beginning of the procedure the problem disappears but i don't think that is the correct solution.

[User137]

- FreeThenNil does not work

Code: [Select]

FreeAndNil(yourObjectName);

[mmxngg]

I have not tried but it's useless for release the grid within application. Maybe can works in the destructor of TCustomGrid for release all object.

[JuhaManninen]

Maybe can works in the destructor of TCustomGrid for release all object.

Hmmm... Do you mean the Grid somehow owns the objects?
You should separate data/logic and GUI as much as possible. Copy the data to/from the GUI controls. The life-time of a control should not depend so much on the life-time of data.

I recommend that you let the Owner of the Grid handle its life-time. You can show/hide it with Visible := True/False, and reuse them as needed.
This is the best way resource-wise, too. Creating a GUI control takes longer that just showing an existing control.

Juha

[mmxngg]

Hmmm... Do you mean the Grid somehow owns the objects?

TCustomGrid allocates some resources for its internal management and releases them with FreeThenNil.

From destructor TCustomGrid.Destroy in grids.pas

Code: [Select]

  FreeThenNil(FColumns);
  FreeThenNil(FGCache.AccumWidth);
  FreeThenNil(FGCache.AccumHeight);
  FreeThenNil(FCols);
  FreeThenNil(FRows);

Moving this code after the inherited destroy or maybe use FreeAndNil "fix" the bug (yes, it's a bug...just try a debug session)

You should separate data/logic and GUI as much as possible. Copy the data to/from the GUI controls. The life-time of a control should not depend so much on the life-time of data.

I recommend that you let the Owner of the Grid handle its life-time. You can show/hide it with Visible := True/False, and reuse them as needed.
This is the best way resource-wise, too. Creating a GUI control takes longer that just showing an existing control.

Yeah, create and release a resource requires more time, it is true, but hide / show it only when required is a contorted solution at least in my case (too much works for nothing). Anyway i fix the problem simply called PerformTab before the Free (a perfect solution for me) but anyone who tries to free a focused grid will take the same issue.

[JuhaManninen]

Ok, it looks like a bug, especially as the other focused controls can be freed.
I applied your code change in r39544 and updated the bug report accordingly.

Thanks, and sorry I first doubted the bug report strongly.
I personally still would not free a focused control but that is another issue.

Juha

[Edit:] Jesus Reyes already fixed it in a better way. Thanks. See the bug report for details.

[mmxngg]

Oh perfect. Can you tell me what is the file patched ? I prefer don't install a new version

Thank you

[Edit1:] Jesus Reyes fix is pefect, well done.
[Edit2:] Crash with a different issue, work in progress.