What is wrong with this code?

Hello to all.
As I work with Delphi, I find interesting pitfalls that lead to errors. A couple of them I want to share with you in the form of problems. I will post the answers to them in a couple of days, but for now you can try to figure out the obvious behavior on your own. If interested - welcome under cat.

Task 1

type IData = interface function Ptr: Pointer; end; procedure AddData(Data1, Data2: IData; out OutData: IData); begin OutData := CreateMerged_IData(Data1.Ptr, Data2.Ptr); // Create_IData    IData end; var DataArr: array of IData; procedure AddDataToAll(const AExtraData: IData); var i: Integer; begin if not Assigned(AExtraData) then Exit; for i := 0 to Length(DataArr) - 1 do AddData(DataArr[i], AExtraData, DataArr[i]); end; 

So, we have an IData interface that stores some data. The AddData function should create a new IData instance based on the other two.
We also have a DataArr array, in which there are no null elements, and at some point we call AddDataToAll. But the procedure does not work as we expect. Why?

Task 2

 var FCollection: TDictionary<TObject, Integer>; procedure KillObject(var Obj: TObject); begin if FCollection.ContainsKey(Obj) then begin //DoSomething obj.Free; FCollection.Remove(obj); obj := nil; end; end; 

Here, FCollection was created like this: FCollection: = TDictionary <TObject, Integer> .Create; No one has set OnKeyNotify and OnValueNotify notifications.
Obj is always a valid existing object. But meanwhile, this code sometimes falls. Why?

The answer to the puzzle 1

As you know, Delphi automatically works with a reference count.
This means that when we assign a new variable to the interface variable under the hood, Delphi does something like this:

 // Value := NewValue; //     : if Assigned(Pointer(Value)) then Value._Release; if Assigned(Pointer(NewValue)) then NewValue._AddRef; Pointer(Value) := Pointer(NewValue); 

Now let's look at passing parameters inside functions.
')

 procedure DoWork(AValue: IUnknown); 

AValue can change inside a function. At the same time outside the value should not change. This means that the variable must be explicitly copied onto the stack, increasing its reference count. After all, if inside the function this variable is assigned a new value, then ._Release will be called. Also inside the DoWork compiler will carefully insert AValue._Release when exiting;
Pseudocode manipulations with the counter:

 procedure DoWork(AValue: Pointer); begin try //DoWork implementation finally IUnknown(AValue)._Release; end; end; MyValue._AddRef; DoWork(Pointer(MyValue)); 

In case of:

 procedure DoWork(const AValue: IUnknown); 

The compiler based on the fact that the value of AValue inside the function does not change - there will be nothing to do with the reference count. The interface pointer will simply be copied onto the stack.
Pseudocode:

 procedure DoWork(const AValue: Pointer); begin //DoWork implementation end; DoWork(Pointer(MyValue)); 

For the case of transfer by reference:

 procedure DoWork(var AValue: IUnknown); 

Data which can be read is transferred to function, and outside it is expected that the data can be changed. Here the compiler, as in the case of const, does not make any gestures with a reference counter.
Pseudocode:

 procedure DoWork(var AValue: Pointer); begin //DoWork implementation end; DoWork(Pointer(MyValue)); 

However, for the case of:

 procedure DoWork(out AValue: IUnknown); 

Everything works differently from var. Out parameter means that uninitialized data can be passed to the function. For uninitialized data ._Release cannot be called. Therefore, before calling Delphi, it necessarily inserts ._Release, and inside the function cleans this out parameter.

Pseudocode manipulations with the counter:

 procedure DoWork(out AValue: Pointer); begin Pointer(AValue) := nil; // ,          ._Release   //DoWork implementation end; MyValue._Release; DoWork(Pointer(MyValue)); 

In the case of the function:

 function DoWork: IUnknown; 

The compiler allocates a temporary variable on the stack, and works with it as with the var parameter.
The pseudocode is as follows:

 procedure DoWork(var Result: Pointer); begin //DoWork implementation end; var FuncResult: IUnknown; Pointer(FuncResult) := nil; DoWork(Pointer(FuncResult)); MyValue := FuncResult; 

Now we can go directly to the answer to the problem.
Look at the announcement

 procedure AddData(Data1, Data2: IData; out OutData: IData); 

Here, before calling, the reference count will be increased for Data1, Data2, and reduced for OutData.
In the case of our challenge

 AddData(DataArr[i], AExtraData, DataArr[i]); 

With DataArr [i], the counter will be increased and decreased. If the counter is first reduced and becomes zero, then the object will be destroyed.
However, the order of manipulations with the reference counter is not defined. Therefore, if DataArr [i] ._ Release is called first, then inside AddData, when accessing Data1, we risk getting Access Violation at address: 00000000
The correct solution is to change AddData to

 function AddData(Data1, Data2: IData): IData; 

or set up a temporary variable before calling:

 AddData(DataArr[i], AExtraData, TmpData); DataArr[i] := TmpData; 

Congratulations to Kemet for correctly explaining the problem in Task 1.

The answer to the puzzle 2

The fact that an object is first destroyed and then removed from the collection is naturally noticed by many. It is understandable, because in the example, in fact, only this is demonstrated. But what is wrong with that? After all the code:

 var obj: TObject; obj := Nil; FCollection.Add(obj, 13); WriteLn(FCollection.Items[obj]); //  13, ..     FCollection.Remove(obj) 

quite working and has no problems. The hash seems to be taken from the pointer ... but it seems to be.
In fact, when we create a TDictionary, we can pass it an IEqualityComparer comparator, or TDictionary will use its default comparator. The problem is specifically stated that the TDictionary is created without parameters, and therefore uses the default comparator. Let's take a look at it.
It is described in Generics.Defaults. When created, it is called

 function _LookupVtableInfo(intf: TDefaultGenericInterface; info: PTypeInfo; size: Integer): Pointer; 

which, from the comparators table, VtableInfo gets the necessary function pointer depending on the type passed to info: PTypeInfo. For objects (tkClass) there will be a pointer in the table to EqualityComparer_Instance_Class: Pointer = @EqualityComparer_Vtable_Class;
EqualityComparer_Vtable_Class is such a table of functions:

  EqualityComparer_Vtable_Class: array[0..4] of Pointer = ( @NopQueryInterface, @NopAddref, @NopRelease, @Equals_Class, @GetHashCode_Class ); 

Here we are interested in:

 function Equals_Class(Inst: PSimpleInstance; Left, Right: TObject): Boolean; begin if Left = nil then Result := Right = nil else Result := Left.Equals(Right); end; function GetHashCode_Class(Inst: PSimpleInstance; Value: TObject): Integer; begin if Value = nil then Result := 42 else Result := Value.GetHashCode; end; 

As you can see, if nil is passed to the function, then 42. Otherwise, some TObject methods are called. Here they are:

  function Equals(Obj: TObject): Boolean; virtual; function GetHashCode: Integer; virtual; 

These are two virtual methods. This means that in the case of the transfer of an object that is not nil, the pointer is guaranteed to be renamed.
These two methods are very similar to how it works in C #. However, C # is a managed language, and we simply cannot have a reference to garbage in memory. Therefore, the name here is always valid. Personally, I find it not logical to copy this behavior. It would be much more correct to declare these two methods as follows:

  class function Equals(Instance: TObject; Other: TObject): Boolean; virtual; class function GetHashCode(Instance: TObject): Integer; virtual; 

Overlap capabilities are preserved, with this default behavior: a hash on pointers can be implemented. Anyway.
I agree that you must first remove from the collections, and only then destroy the data. But I am sure that the code above is simply suspicious for many, when in fact it contains a gross error.
In addition, I would like to draw attention to how the hash is implemented by the pointers in TObject (at least in Delphi 2010):

 function TObject.GetHashCode: Integer; begin Result := Integer(Self); end; 

As we see it is very bad. In the case of allocations, we are more likely to get aligned data + granular size of the object, which potentially gives rise to many collisions.