Skip to content

[Packages] Bug fix to TObjectDictionary unexpectedly freeing object if SetValue is called with the object it is already set to

J. Gareth "Kit" Moreton requested to merge CuriousKit/optimisations:i40024 into main

Summary

This merge request fixes a bug where if a TObjectDictionary specialization sets a value to the same object that it is already equal to, it will erroneously free the object and leave the pointer dangling (reported in #40024 (closed)).

What is the current bug behavior?

In the following sample project (from #40024 (closed)), the object y has been freed after the second call to AddOrSetValue:

program i40024;

uses
  Generics.Collections,
  SysUtils;

var
  x: specialize TObjectDictionary<Integer, TObject>;
  y: TObject;

begin
  x := specialize TObjectDictionary<Integer, TObject>.Create ([doOwnsValues]);
  y := TObject.Create;
  x.AddOrSetValue (0, y);
  x.AddOrSetValue (0, y);
  y.Free; // access violation
  // x intentionally not being freed
end.

What is the behavior after applying this patch?

Objects are no longer freed in the case where the value being set is equal to what is already stored.

Additional Notes

  • The fix required changing the TCustomDictionary<CUSTOM_DICTIONARY_CONSTRAINTS>.SetValue method by making it dynamic and overriding it for TObjectOpenAddressingLP<OPEN_ADDRESSING_CONSTRAINTS> descendants, where the values are explicitly checked for equality (objects specialized and descended from this generic class expect to have value types that descend from TObject).
  • This may break Delphi compatibility slightly because when a value is set to the object it is already equal to, the OnValueNotify events are no longer triggered, not even the cnAdded one (logically this makes sense because nothing has been added or removed and is just an identity operation).
  • The choice of dynamic over virtual is to follow the original Delphi recommendation of using dynamic if the base class has a lot of descendents but the method is seldom overridden.
Edited by J. Gareth "Kit" Moreton

Merge request reports