Is this Delphi Thread code correct?

Question

I know that Delphi threads have been discussed on many threads. I tried reviewing them, but didn't find an answer to my question.

Background: I have found that freeing a TWebBrowser can take 10+ second after the browser loaded up Adobe Acrobat Reader DC. I think somehow it is checking for updates or something. It is annoying when trying to close a form with a browser in it.

I thought perhaps I could have a background thread release the browser. So I moved the browser variable to a global variable (privately stored in the implementation part of the unit). Only one of these forms would be used at a time. Then I tried to have a thread free it in the background. It's not working as I would have expected.

Example code

interface
  TMyform = class(TForm)
    pnlBowserHolder: TPanel;
    procedure FormDestroy(Sender: TObject);
    procedure FormCreate(Sender: TObject);
  private
    //WebBrowser : TWebBrowser;  <-- moved to global variable
  public
    { Public declarations }
  end;

implementation

type
  TBackgroundBrowserKillerThread = class(TThread)
  public
    procedure Execute; override;
  end;

var
  WebBrowser : TWebBrowser;
  BrowserKillerThread : TBackgroundBrowserKillerThread;

procedure TfrmLabImageViewer.FormCreate(Sender: TObject);
begin
  WebBrowser := TWebBrowser.Create(Self);
  TWinControl(WebBrowser).Parent := pnlBowserHolder;
  WebBrowser.Align := alClient;
end;

procedure TfrmLabImageViewer.FormDestroy(Sender: TObject);

begin
  BrowserKillerThread := TBackgroundBrowserKillerThread.Create(true);
  Application.ProcessMessages;
  BrowserKillerThread.Execute();  
  //WebBrowser.Free;
end;

procedure TBackgroundBrowserKillerThread.Execute();
begin
  TWinControl(WebBrowser).Parent := nil;
  FreeAndNil(WebBrowser);
  self.FreeOnTerminate := true;
  BrowserKillerThread := nil;  //free reference to thread, shouldn't affect ability of self to free itself (?)
end;

Questions:

• When I step through the FormDestroy code in debug mode, the line containing BrowserKillerThread.Execute(); takes 10 seconds still to execute. I had thought that this would launch the other thread and return immediately. But it doesn't. Is my understanding wrong of what .execute does? Or is something funny going on?
• Is what I am doing a bad thing? I have read that the VCL is not thread safe, and that one can't/shouldn't access VCL objects from the other thread. I was hoping that this wouldn't apply in this case since I am just freeing the object with no further interaction planned.
• If a thread free's itself upon termination, I assume this would leave my BrowserKillerThread pointer dangling. So is it OK to assign nil to this as I do?
• Any suggestions on how to do this better?

Thanks so much in advance.

KT

Answer 1

Is my understanding wrong of what .execute does??

Yes. This is not how you do threading. Generally, you first have to decide whether you need to do anything whatsoever with the thread after you have initially started it. If so, keep a reference, don't use FreeOnTerminate, and take care of the thread's destruction yourself after it has terminated. If you don't, don't keep a reference, set FreeOnTerminate, and send it off.

You would neither set FreeOnTerminate within a thread's execute, nor keep a global reference to a thread that has this set. You also don't start a thread by calling it's Execute method, but by either creating it non-suspended, or by calling Start. Just calling Execute doesn't actually start the thread, but executes this procedure within the context of the current thread, that's why it still takes 10 seconds.

Your example would become:

procedure TfrmLabImageViewer.FormDestroy(Sender: TObject);
  var BrowserKillerThread: TBackgroundBrowserKillerThread;
begin
  BrowserKillerThread := TBackgroundBrowserKillerThread.Create(true);
  BrowserKillerThread.FreeOnTerminate := true;
  BrowserKillerThread.Start;
end;

procedure TBackgroundBrowserKillerThread.Execute();
begin
  TWinControl(WebBrowser).Parent := nil;    // I don't think you need to nil the parent here, but probably does no harm
  FreeAndNil(WebBrowser);
end;

Now the threading is correct, but the logic is still wrong. As you already noticed you must not modify VCL objects from threads other than the main thread, this includes Free. You'd have to do this with Synchronize, which would defeat the purpose of threading.

I also think that the idea of killing the browser in a thread because otherwise it takes too long is only battling the symptoms, rather than the cause. I think it would be best to find out why that takes so long in the first place, and fix that, rather than trying to circumvent the issue this way. But this is outside of the scope of this question, if you have issues with that you should ask a separate question for this specific issue.