TMultiFormatReader.FreeReaders... potential access violation when called from Destroy... and useless code inside of it

[csm101]

This is how this procedure is currently written... See my comments inside the code

procedure TMultiFormatReader.FreeReaders;
var
  Reader: IReader;
  OneDReader: TOneDReader;
begin
  if readers <> nil then
  begin

    for Reader in readers do
    begin

      // I think that you are doing this cast from Reader to TOneReader just because the compiler 
      // does not allow you to assign Nil to Reader, being it the control loop variable, but here you are 
      //  missing the point.. 
      if (Reader is TOneDReader) then
      begin
         // by doing this assignment you are adding +1 to the internal reference count of the object 
         // if you are compiling for ARC...
        OneDReader := TObject(Reader) as TOneDReader;
        // and by then assigning nil you are subtracting 1 to the same reference count....
        OneDReader := nil;
        // so if the reference count was 5, you first increase it to 6, then you put 
        // it again to 5 and no deallocation is done at all, since the reference count goes 
        //  back to the original value: this code has absolutely NO effect to the whole 
        // program no matter if you are compiling for ARC or Win32/Win64
      end;
    end;
  end;

  // here is the second problem: you are calling readers.clear OUTISDE the "if" block where you 
  // tested that  readers is not nil... so here it might be nil and you could get an access violation.
  readers.Clear(); 
  readers.Free;
  readers := nil;

end;

The first thing you could do if you actually want to release the interfaces contained in the list is to use an integer to iterate over the values, so you are free to assign the list values by index:

procedure TMultiFormatReader.FreeReaders;
var
  Reader: IReader;
  OneDReader: TOneDReader;
  i:integer;
begin
  if readers <> nil then
  begin
    for i:= 0 to readers.Count- 1 do
       readers[i] := nil; // this will actually decrease the reference count by 1
    readers.Clear(); // notice that now this is inside the if
  end;

  readers.Free;
  readers := nil;
end;

In second place.. It is totally useless to explicitly free the interfaces by assigning nil to them because the generic class TList already does it inside its Clear method, so the whole procedure should be just this:

procedure TMultiFormatReader.FreeReaders;
begin
  if readers <> nil then
    readers.Clear();

  readers.Free;
  readers := nil;
end;

[Spelt]

Thanks for the feedback! It's greatly appreciated!

The TMultiformatReader has been greatly simplified in branch v_3.0 but not his procedure. I will check this out and will if successful merge changes in v3 in a final bug fix round.

See: https://github.com/Spelt/ZXing.Delphi/tree/v_3.0

I'll keep this issue open until more is known.

[Spelt]

fixed in branch 2 and branch 3