Freeing buttons in a list in OnClick

What I'm trying to achieve, in simplified form, is to create a list of dynamically created buttons. When clicking on one of the buttons it should be removed from the list and its object should be freed. My approach is :

• Create a TList<TButton>
• Create a couple of TButton objects and add them to the TList<TButton>
• Assign the Form as the Parent for each of the created TButton objects
• Assign a Position for each of the created TButton objects
• Assign an OnClick handler method to each of the created TButton objects
• The OnClick handler sets the Sender TButton's Parent to nil and deletes it from the TList<TButton>, so that ARC can free the TButton object that was clicked on.

When I click on one of the dynamically created buttons, I get a "Segmentation Fault". I suspect it is because I am freeing the TButton object in its own OnClick handler and the class is trying to do some other stuff with it after my handler.

I have tested this on Android. I assume the same will happen on iOS, or any other ARC platform for that matter.

Is there a better/right way to do this, or another approach I should be following to get it working the way I want?

Here is some example code. It is for a form with one design-time button (Button1) on it. Clicking this button repeatedly dynamically creates new buttons and adds them to the list.

unit Unit2;

interface

uses
  System.SysUtils, System.Types, System.UITypes, System.Classes, System.Variants,
  FMX.Types, FMX.Controls, FMX.Forms, FMX.Graphics, FMX.Dialogs,
  FMX.Controls.Presentation, FMX.StdCtrls, System.Generics.Collections;

type
  TForm2 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
    procedure FormCreate(Sender: TObject);
  private
    ButtonList : TList<TButton>;
    procedure ButtonClick(Sender: TObject);
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form2: TForm2;

implementation

{$R *.fmx}

procedure TForm2.ButtonClick(Sender: TObject);
var
    pos : Integer;
begin
    pos := ButtonList.IndexOf(TButton(Sender));
    TButton(Sender).Parent := nil;
    ButtonList.Delete(pos);
end;

procedure TForm2.FormCreate(Sender: TObject);
begin
    ButtonList := TList<TButton>.Create;
end;

procedure TForm2.Button1Click(Sender: TObject);
var
    pos : Integer;
begin
    pos := ButtonList.Add(TButton.Create(nil));
    ButtonList.Items[pos].Parent := Form2;
    ButtonList.Items[pos].Position.Y := 50 * ButtonList.Count;
    ButtonList.Items[pos].OnClick := ButtonClick;
end;

end.

When I click on one of the dynamically created buttons, I get a "Segmentation Fault". I suspect it is because I am freeing the TButton object in its own OnClick handler and the class is trying to do some other stuff with it after my handler.

That is exactly what is happening. After the event handler exits, the RTL still needs access to the button object to finish processing the click and message handling. It is never safe to destroy a UI object from inside of its own events. So you have to make sure the object remains alive during event handling.

I have tested this on Android. I assume the same will happen on iOS, or any other ARC platform for that matter.

Yes. And it would also happen on non-ARC platforms if you tried to Free the button explicitly, eg:

procedure TForm2.ButtonClick(Sender: TObject);
var
  btn: TButton;
begin
  btn := TButton(Sender);
  ButtonList.Remove(btn);
  {$IFDEF AUTOREFCOUNT}
  btn.Parent := nil;
  {$ELSE}
  btn.Free;
  {$ENDIF}
end;

Is there a better/right way to do this, or another approach I should be following to get it working the way I want?

You could have the OnClick handler post an asynchronous message to the main thread (such as by calling TThread.Queue() inside of TThread.CreateAnonymousThread()/TTask.Run(), or using TThread.ForceQueue() in 10.2 Tokyo and later), and then exit immediately, letting the message handler free the button at a later time when the button is no longer being used, eg:

procedure TForm2.ButtonClick(Sender: TObject);
var
  btn: TButton
begin
  btn := TButton(Sender);
  ButtonList.Remove(btn);

  TThread.CreateAnonymousThread(
    procedure
    begin
      TThread.Queue(nil, btn.DisposeOf);
    end
  ).Start;

  { or:
  TThread.ForceQueue(nil, btn.DisposeOf);
  }
end;

Alternatively, you could move the button object to another list, and then start a short timer (or use a TThread.(Force)Queue() message) to run through that list freeing its objects, eg:

unit Unit2;

interface

uses
  System.SysUtils, System.Types, System.UITypes, System.Classes, System.Variants,
  FMX.Types, FMX.Controls, FMX.Forms, FMX.Graphics, FMX.Dialogs,
  FMX.Controls.Presentation, FMX.StdCtrls, System.Generics.Collections;

type
  TForm2 = class(TForm)
    Button1: TButton;
    Timer1: TTimer;
    procedure Button1Click(Sender: TObject);
    procedure FormCreate(Sender: TObject);
    procedure Timer1Timer(Sender: TObject);
  private
    { Private declarations }
    ButtonList : TList<TButton>;
    DisposeList : TList<TButton>;
    procedure ButtonClick(Sender: TObject);
    procedure DisposeOfButtons;
  public
    { Public declarations }
  end;

var
  Form2: TForm2;

implementation

{$R *.fmx}

procedure TForm2.ButtonClick(Sender: TObject);
var
  btn: TButton;
begin
  btn := TButton(Sender);
  ButtonList.Remove(btn);
  DisposeList.Add(btn);

  Timer1.Enabled := true;

  { or:
  TThread.CreateAnonymousThread(
    procedure
    begin
      TThread.Queue(nil, DisposeOfButtons);
    end
  ).Start;
  }

  { or:
  TThread.ForceQueue(nil, DisposeOfButtons);
  }
end;

procedure TForm2.FormCreate(Sender: TObject);
begin
  ButtonList := TList<TButton>.Create;
  DisposeList := TList<TButton>.Create;
end;

procedure TForm2.Button1Click(Sender: TObject);
var
  btn: TButton;
begin
  btn := TButton.Create(nil);
  ButtonList.Add(btn);
  btn.Parent := Self;
  btn.Position.Y := 50 * ButtonList.Count;
  btn.OnClick := ButtonClick;
end;

procedure TForm2.DisposeOfButtons;
var
  btn: TButton;
begin
  for btn in DisposeList do
    btn.DisposeOf;
  DisposeList.Clear;
end;

procedure TForm2.Timer1Timer(Sender: TObject);
begin
  Timer1.Enabled := False;
  DisposeOfButtons;
end;

end.