Nov 242018

There was a bug in the (yet unreleased) GExperts code that caused an access violation every time the Delphi IDE was closed. I have just found it, but boy was that difficult!

I knew the problem existed in the current source code and by trial and error I found a source code revision that did not yet have it: #2415. So I compared those revisions and step by step narrowed it down to the changes in the unit GX_IdeFormChangeManager in revision #2433 which was a fix for a redrawing bug in the Delphi 10.2 Search Path dialog.

So I removed the code I had added, which consisted of two parts:

procedure TFormChangeManagerInternal.ProcessActivatedForm(Form: TCustomForm);
  i: Integer;
  NeedsWorkaround: Boolean;

  // We are executing the callbacks in last registered first order to allow callbacks to
  // unregister themselves. (And we don't check whether a callback only unregisters itself, so
  // please be cautios!). That means registering a new callback while executing a callback
  // is not allowed.
  FIsInFormChangeCallback := True;
// ------------- workaround part 1 starts here ------------
    // Workaround for redraw issue in Delphi 10.2 when theming is enabled:
    // Disable redraws while we change the window
    NeedsWorkaround := HasRedrawProblems(Form);
    if NeedsWorkaround then
      SendMessage(Form.Handle, WM_SETREDRAW, WParam(False), 0);
// ------------- workaround part 1 ends here ------------
      for i := FFormChangeCallbacks.Count - 1 downto 0 do begin
          TFormChangeCallbackItem(FFormChangeCallbacks[i]).FCallback(Self, Form);
          // just so we can have a look at any exceptions that might be raised ...
// ------------- workaround part 2 starts here ------------
      if NeedsWorkaround then begin
        // Allow and force a redraw of the whole window
        SendMessage(Form.Handle, WM_SETREDRAW, WParam(True), 0);
        RedrawWindow(Form.Handle, nil, 0, RDW_INVALIDATE or RDW_ALLCHILDREN);
        // see Remarks on
// ------------- workaround part 2 ends here ------------
    FIsInFormChangeCallback := False;

I didn’t really think this could make a difference because the HasRedrawProblems function only returns true in Delphi 10.2 but the AV occurred in all versions of the IDE. And guess what? It didn’t make a difference at all! The AV still happened.

So I reverted all the changes to the unit, which included the addition of a few units to the uses clause in the implementation section: Messages, Registry and GX_IdeSearchPathEnhancer

The AV was gone!

Now what? I added Messages and Registry again, everything still was fine. Then I added GX_IdeSearchPathEnhancer and boom, the AV was back.

After I knew what to look for, the cause was simple to spot: Both units GX_IdeSearchPathEnhancer and GX_IdeFormChangeManager have got a finalization section. They both free an instance of a class that gets created some time during the life time of a GExperts session. By adding GX_IdeSearchPathEnhancer to the uses list of GX_IdeFormChangeManager, the execution order of the finalization sections changed.

The unit GX_IdeFormChangeManager provides a singleton instance of TFormChangeManagerInternal which gets created on demand in any of the class methods of TIDEFormChangeManager. That instance is freed in the finalization section.

The unit GX_IdeSearchPathEnhancer has an internal instance of TSearchPathEnhancer which also gets freed in its finalization section. TSearchPathEnhancer.Create registers itself for Screen.OnActiveFormChanged events to detect when the IDE’s Sarch Path dialog is being shown. In the Destructor it unregisters itself for that event.

So far, so good, now the problem occurs when the finalization of GX_IdeFormChangeManager runs before the one of GX_IdeSearchPathEnhancer: The singleton instance of TFormChangeManagerInternal gets freed and afterwards the destructor of TSearchPathEnhancer tries to use it to unregister itself from the OnActiveFormChanged event. This created a new TFormChangeManagerInternal instance which hooked the Screen.OnActiveFormChanged event. Since the finalization of GX_IdeFormChangeManager had already run, this new instance never got freed, so as soon as the GExperts DLL was unloaded, the event pointed to uninitialized memory and thus the access violation occurred. This of course will never show anything usable in the debugger as the offending code has already been unloaded.

Now you might ask, why in the world I added the additional unit to the uses clause? That was because of the fix introduced in that revision. It needed to know if the new active form as the Search Path dialog, and following the Don’t repeat yourself (DRY) principle [1] of good software development, I moved the code for detecting that, which was already in a method of TSearchPathEnhancer, to a public class method of that class. So I avoided duplicating the code. but in order to call that class method, I had to add the unit to the uses clause. Which introduced the bug.

Even though it was difficult to trace it, at least it was a reproducible bug, so I could finally track it down. Now I’m going to fix it and then I will start testing GExperts in Delphi 10.3 so I can make a new release.

[1] Yes, it is DRY, because the knowledge of how to detect if a form is the Sarch Path dialog is “business knowledge”, in this case knowledge about what class and name that form has in any of the IDE versions.

 Posted by on 2018-11-24 at 15:44