Delphi Seattle: I get an Invalid Pointer operation when freeing an object I created

Question

I use Delphi Seattle.

My problem occurs when I attempt to free an object I created.

I searched in this site (and other sites as well) for answers already posted for this question, but they all are a bit different. According to those discussions, my code should work, but obviously something isn't quite right.

So, I need help...

Flow of execution:

a) in form fmLoanRequest, I create an object based on Class TStorageLoan (a Sub-class of TLoan). The Constructor loads all kinds of values into some of the object's attributes (now shown here).

b) Later, I pass the object's address to another form (fmLoan) to an appropriate public variable. fmLoan is the form where all the user's dealings with the contents of Loan happen. Note that fmLoanRequest remains as is while we're in fmLoan. We'll return to fmLoanrequest when fmLoan closes.

c) The fmLoan form is displayed (and shows the data in the object - all that is working well).

d) When closing fmLoan, a procedure is called to Free the Loan object - if it is assigned (see line 10 of second code snippet). That seems to work OK (no error).

e) The 'Invalid Pointer Operation' error occurs when the code in line 14 below is executed: ( if Assigned(oLoan) then oLoan.Free; ).

I had added this line to make sure the object would be freed if fmLoan didn't for some reason deal with it. I realize that the object has been Freed by this time, but shouldn't the 'if Assgned()' prevent unnecessary freeing of the object?

Partial code from form fmLoanRequest (I added some line numbers for reference)

1  // In form fmLoanRequest 
2  // Create new Loan Object (from a Loan sub-class as it happens)
3    // Create the object here; Object address will be passed to fmLoan later for handling.
4    oLoan := TStorageLoan.Create(iNewLoanID);
5  ...
6  ...
7     fmLoan.oLoan := oLoan; // pass the address to the other form
8     fmLoan.show;
9     // User would click the 'btnClose' at this point. See event code below.
10  ...
11  ...
12    procedure TfmLoanRequests.btnCloseClick(Sender: TObject);
13    begin 
14      if Assigned(oLoan) then oLoan.Free; // <--- ERROR HERE 
15      fmLoanRequests.Close;
16  end;

Partial code from form fmLoan (I added some line numbers for reference)

1  //Form fmLoan
2  ...
3    public
4      oLoan : TLoan;
5  ... 
6  // In form fmLoan, I call the following upon closing the Form
7  //                 in the OnClick event of the 'btnClose' button. 
8  Procedure TfmLoan.Clear_Loan_Object;
9  begin
10    if Assigned(oLoan) then oLoan.Free; // <-- THIS WORKS FINE
11  end;

Should I try a different approach?

Should I just remove that line (line 14 - first code snippet) and hope for the best. That's not at all my philosophy on proper coding!

Am I going at it the wrong way?

Note: I obviously don't use pointers.

Any help would be appreciated!

Answer 1

I had added this line to make sure the object would be freed if fmLoan didn't for some reason deal with it. I realize that the object has been freed by this time, but shouldn't the if Assigned() prevent unnecessary freeing of the object?

This is a key misunderstanding. Consider the following program:

{$APPTYPE CONSOLE}

var
  obj: TObject = nil;

begin
  Writeln(Assigned(obj));

  obj := TObject.Create;
  Writeln(Assigned(obj));

  obj.Free;
  Writeln(Assigned(obj));

  Readln;
end.

This outputs the following:

FALSE
TRUE
TRUE

Note that the final line of output is TRUE. In other words, when you destroy an object be calling its Free method, the reference variable is not set to nil.

Your mistake is that you believe that Assigned tests whether or not the object has been destroyed. It does not do that. It merely tests whether or not the reference variable is nil or not. Let's look at the code again in more detail.

obj := TObject.Create;

Here we create a new object, allocated on the heap, with a call to TObject.Create. We also assign to obj the address or reference of that object. After this line executes, obj is a reference variable that contains the address of a valid object.

obj.Free;

This destroys the object to which obj refers. The destructor is run, and then the memory is destroyed. After this line executes, the object has been destroyed, but obj still refers to that destroyed and now invalid piece of memory. That is why Assigned(obj) yields true.

Note: I obviously don't use pointers.

That's an interesting point. In fact, you are using pointers whenever you use a reference variable. Although the language hides this fact, an object reference variable is nothing more than a pointer to memory allocated on the heap. We use the terminology reference rather than pointer but really these are the same things. They behave identically, the assignment operator has identical semantics, you are still subject to potential for leaking, double freeing, access after free, and all the other pitfalls of pointers. So although you do not explicitly use pointers, it still pays to think about object reference variables as though they are pointers.

I wrote a detailed answer on this whole topic for a different question. I suggest that you read that answer: https://stackoverflow.com/a/8550628/505088.

One of the points that you will take away is that code like

if Assigned(oLoan) then 
  oLoan.Free;

is pointless. The Free method also checks whether or not the object reference is nil. That line of code is in effect expanded to:

if Assigned(oLoan) then 
  if Assigned(oLoan) then 
    oLoan.Destroy;

So, instead of

if Assigned(oLoan) then 
  oLoan.Free;

you should simply write

oLoan.Free;

Now, back to the access violation. I think that it should now be obvious that you are attempting to destroy an object that has already been destroyed. You must not do that. You'll need to re-examine your lifetime management. Reasoning like "if fmLoan didn't for some reason deal with it" really are not good enough. You need to be 100% sure about lifetime management. You need to make sure that your objects are destroyed exactly once. Not being able to see all your code I don't want to make specific recommendations.

One pattern that is sometimes useful is to set the object reference to nil when you destroy the object. If the object may be destroyed in multiple places then this technique can be used to make sure that that you don't attempt to destroy it twice. You may even use the FreeAndNil helper function. However, it is worth stressing that if you are unsure of whether or not you have already destroyed the object, then that is usually indicative of poor design. If you find yourself want to add calls to Free to act "just in case" then you are almost certainly doing something seriously wrong.

Answer 2

It is clear that you are freeing the Loan object twice, that is why you are getting the error. You need to free it only once. fmLoanRequests creates the object, but you say it can be closed before fmLoan is closed, so fmLoan should take ownership of the object and free it when fmLoan is closed. Don't free the object at all when fmLoanRequest is closed.

The alternative is to define an ILoan interface that TLoan and descendants implement, and then pass ILoan around instead of TLoan directly. Interfaces are reference counted, so the Loan object will be freed automatically, and only once, after both fmLoanRequests and fmLoan have released their references to it.