Что не так с этим кодом?

Привет всем.
По мере моей работы с Delphi я нахожу интересные подводные камни, приводящие к ошибкам. Парочкой из них я хочу поделиться с вами в форме задачек. Ответы на них я опубликую через пару дней, а пока вы можете попробовать разобраться в не очевидном поведении самостоятельно. Если интересно — добро пожаловать под кат.

Задачка 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;

Итак, у нас есть интерфейс IData, который хранит какие-то данные. Функция AddData должна создавать новый экземпляр IData на базе двух других.
Так же у нас есть массив DataArr, в котором нет нулевых элементов, и в какой-то момент мы вызываем AddDataToAll. Но процедура работает не так как мы ожидаем. Почему?

Задачка 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;

Здесь FCollection был создан вот так: FCollection := TDictionary<TObject, Integer>.Create; Нотификации OnKeyNotify и OnValueNotify никто не устанавливал.
Obj — всегда валидный существующий объект. Но между тем данный код иногда падает. Почему?

Ответ на задачку 1

Как известно Delphi автоматически работает с со счетчиком ссылок.
Это значит что когда мы присваиваем интерфейсной переменной новое значение под капотом делфи делает примерно так:

//присвоение
Value := NewValue;
//под капотом превратится в подобный псевдокод:
if Assigned(Pointer(Value)) then Value._Release;
if Assigned(Pointer(NewValue)) then NewValue._AddRef;
Pointer(Value) := Pointer(NewValue);

Теперь давайте рассмотрим передачу параметров внутрь функций.

procedure DoWork(AValue: IUnknown);

AValue может меняться внутри функции. При этом снаружи значение не должно измениться. Это значит, что переменную надо явно скопировать на стек, увеличив ей счетчик ссылок. Ведь если внутри функции этой переменной присвоят новое значение — то будет вызван ._Release. Так же внутри DoWork при выходе компилятор заботливо вставит AValue._Release;
Псевдокод манипуляций со счетчиком:

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

MyValue._AddRef;
DoWork(Pointer(MyValue));

В случае с:

procedure DoWork(const AValue: IUnknown);

Компилятор опираясь на то, что значение AValue внутри функции не меняется — не будет ничего делать со счетчиком ссылок. Указатель на интерфейс просто будет скопирован на стек.
Псевдокод:

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

DoWork(Pointer(MyValue));

Для случая передачи по ссылке:

procedure DoWork(var AValue: IUnknown);

В функцию передаются данные которые можно читать, а снаружи ожидается, что данные могут быть изменены. Здесь компилятор как и в случае с const не делает никаких телодвижений со счетчиком ссылок.
Псевдокод:

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

DoWork(Pointer(MyValue));

Однако для случая:

procedure DoWork(out AValue: IUnknown);

Все работает не так как с var. Out параметр означает что в функцию могут передаваться неинициализированные данные. Для неинициализированных данных ._Release вызывать нельзя. Поэтому перед вызовом Delphi обязательно вставляет ._Release, а внутри функции чистит этот out параметр.

Псевдокод манипуляций со счетчиком:

procedure DoWork(out AValue: Pointer);
begin
  Pointer(AValue) := nil; //инициализация нужна, чтобы дальнейшие манипуляции с переменной не приводили к вызову ._Release у мусора
  //DoWork implementation
end;

MyValue._Release;
DoWork(Pointer(MyValue));

В случае с функцией:

function DoWork: IUnknown;

Компилятор аллоцирует временную переменную на стеке, и работает с ней как с var параметром.
Псевдокод такой:

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

var FuncResult: IUnknown;
Pointer(FuncResult) := nil;
DoWork(Pointer(FuncResult));
MyValue := FuncResult;

Теперь мы можем перейти непосредственно к ответу на задачу.
Посмотрим на объявление

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

Здесь перед вызовом счетчик ссылок будет увеличен у Data1, Data2, и уменьшен у OutData.
В случае с нашим вызовом

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

У DataArr[i] счетчик будет и увеличен и уменьшен. Если счетчик будет сначала уменьшен и станет равным нулю — то объект будет уничтожен.
Однако порядок манипуляций со счетчиком ссылок не определен. Поэтому если DataArr[i]._Release будет вызван первым, то внутри AddData при обращении к Data1 мы рискуем получить Access Violation at address: 00000000
Правильным решением будет изменить AddData на

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

или завести временную переменную перед вызовом:

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

Поздравляю Kemet с правильным объяснением проблемы в Задачке 1.

Ответ на задачку 2

То что объект сначала уничтожается, а потом удаляется из коллекции — заметили естественно многие. Оно и понятно, ведь в примере по сути только это и продемонстрировано. Но что же с этим не так? Ведь код:

var obj: TObject;
obj := Nil;
FCollection.Add(obj, 13);
WriteLn(FCollection.Items[obj]); //будет выведено 13, т.е. значение есть в коллекции
FCollection.Remove(obj)

вполне рабочий и проблем не имеет. Хеш вроде бы берется от указателя… но это вроде бы.
На самом же деле когда мы создаем TDictionary, мы можем передать ему компаратор IEqualityComparer, или же TDictionary будет использовать свой компаратор по умолчанию. В задачке специально оговорено, что TDictionary создается без параметров, а значит использует компаратор по умолчанию. Дава��те на него посмотрим.
Описан он в Generics.Defaults. При созданнии вызывается

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

которая из таблицы компараторов VtableInfo получает нужный указатель на функцию в зависимости от типа, переданого в info: PTypeInfo. Для объектов (tkClass) будет в таблице хранится указатель на EqualityComparer_Instance_Class: Pointer = @EqualityComparer_Vtable_Class;
а EqualityComparer_Vtable_Class — это вот такая таблица функций:

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

Здесь нас интересуют:

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;

Как видно, если в функцию передается nil — то 42. В противном случае — вызываются какие-то методы у TObject. Вот они:

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

Это два виртуальных метода. Это значит, что в случае передачи объекта, который не nil — происходит гарантированное разименование указателя.
Два данных метода очень похожи на то, как оно устроено в C#. Однако C# это managed язык, и у нас просто не может быть ссылки на мусор в памяти. Поэтому разименование тут допустимо всегда. Лично я нахожу не логичным копирование этого поведения. Гораздо правильнее было бы объявить эти два метода так:

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

Сохраняются возможности перекрытия, при этом повед��ние по умолчанию: хеш по указателям можно реализовать. Ну да ладно.
Я согласен, что надо сначала удалять с коллекций, а только потом уничтожать данные. Но я уверен, что код выше для многих просто является подозрительным, когда на самом деле он содержит грубую ошибку.
Помимо этого я бы хотел обратить внимание на то, как реализован хеш по указателям у TObject (по крайней мере в Delphi 2010):

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

Как видим очень плохо. В случае аллокций мы скорее всего будем получать выравненные данные + гранулярные размеру объекта, что потенциально порождает много коллизий.