I recently added the JCL debugging helpers to our production applications, and I think it’s the greatest Delphi debugging tool since the invention of the call stack. For those not in the know, the tool allows the end user to display a detailed error report at runtime, giving a call stack, list of loaded DLLs, and lots of other information when an exception is raised. They can send this in to us, and it has allowed us to fix bugs we can’t even reproduce here.
But reading these reports is an entirely different kind of mindset for debugging. While the call stack is an important tool while debugging in the IDE, when working with a JCL report you have nothing else. So you have to approach the problem from a different mental point of view.
Yesterday, I received the following call stack in my email. It took me a while to figure out what was going on, but when I did I realized that I had made an important error in my code which is probably a fairly common mistake in the Delphi world. Here’s the (top of the) report:
Access violation at address 00913A54 in module 'TRAC.EXE'. Read of address 00000000. Access violation at address 00913A54 in module 'TRAC.EXE'. Read of address 00000000 Exception class: EAccessViolation Exception address: 00913A54 ------------------------------------------------------------------------------ Stack list, generated 08/23/2004 3:39:24 PM [00913A54] VerDM.TVertexDM.ClearDatasetEvents + $8 [004F65EE] IB.IBDataBaseError + $232 [40005F12] System.@HandleFinally + $2A [004E9456] IBServices.TIBCustomService.Attach + $136 [40005F12] System.@HandleFinally + $2A [004E9C2B] IBServices.TIBCustomService.SetActive + $23 [007C72CB] IBPropertiesDM.TdmIBProperties.GetServerProperties + $DF [40005F12] System.@HandleFinally + $2A [007C741B] IBPropertiesDM.TdmIBProperties.GetSupportsServicesAPI + $B [007C8060] DBUserManagementDM.TdmDBUserSecurity.Create + $98 [40005F12] System.@HandleFinally + $2A [007CC171] UserManagementDlg.TfrmUserManagement.CreateDataModule + $D [007CC268] UserManagementDlg.TfrmUserManagement.FormCreate + $0 [400EDDA1] Forms.TCustomForm.DoCreate + $31
I’ve snipped quite a lot here, but all the essentials are in the stack above. The trick question is: What is the bug here?
As with the IDE’s call stack window, the report above should be read from the bottom to the top to learn the chronological sequence of events. What we see is that a form’s OnCreate event calls a method called CreateDataModule which (drum roll, please) creates a data module of class TdmDBUserSecurity. Its constructor is calling a method on another DM called GetSupportsServices API. It looks like this method, or, more precisely, one of the methods it calls, is causing the InterBase server to return an error, because IBDatabaseError is an IBX helper function which raises an error when the server has returned an error code.
So far, nothing complicated here. But the error returned is not an EIBInterBaseError, it’s an access violation in another method. TVertexDM is the parent class of TdmUserSecurity, but the ClearDatasetEvents method doesn’t show up anywhere in the methods below it in the call stack. That’s odd.
Here’s the code for this method:
procedure TVertexDM.ClearDatasetEvents;
var
i: integer;
begin
for i := 0 to Pred(FDatasetEvents.Count) do begin
FDatasetEvents.Objects[i].Free;
end; // for
FDatasetEvents.Clear;
end;
Since the read address in the AV was 00000000 it’s clear that the method is trying to dereference a nil pointer, and the nil pointer must be FDatasetEvents itself, since the Free method is "special" and works even if the object you’re calling it with is nil. FDatasetEvents is allocated in the TVertexDM constructor.
The next step, then, is to see where ClearDatasetEvents is called, and that’s when the problem began to unravel. I found a call to it in the TVertexDM.Destroy override. This method doesn’t appear anywhere in the call stack, but I remembered that destructors are automatically called when there’s an exception raised in the constructor of a class, which, from the call stack is certainly the case here.
So the reason for the AV is that the exception in the subclass results in a call to the parent class’s destructor, which — and this is a bug — isn’t prepared to deal with incompletely constructed instances. I fixed this by putting an if Assigned(FDatasetEvents) then… around the call to ClearDatasetEvents in the destructor, and an Assert(Assigned(FDatasetEvents)) in the preconditions of ClearDatasetEvents. To repeat, it’s really important that destructors should never presume that a constructor was successful, and destructors shouldn’t raise exceptions at all, either directly or in methods they call. Yes, the assertion will raise an exception if it fails, but due to the Assigned check in the destructor it can’t fail there.
So what was the first error, the one which started this chain? I still don’t know, and that’s why this is a trick question. The destructor bug hid it, and so I had to create a patched executable to send to the customer. The patch won’t fix the problem, but it will at least tell me what it is.
{ 5 } Comments
Shouldn’t your for to do loop be:
for i := Pred(FDatasetEvents.Count) downto 0 do
My logic would be the following on it:
It starts and there are say 10 items, and it frees the first one (item 0). Now there are nine, and everything is reordered (what was item 1 is now item 0), so now it frees item 1 (which was item 2 before the first free) and skips the "new" item 0.
Going the other way through the items doesn’t have that issue.
Robert: not the case here because freeing those objects doesn’t remove them from the list - it doesn’t look like a ComponentList so no free notifications there
otherwise a good lesson … I’ll keep it in mind
I’m not sure I would check that FDatasetEvents is assigned before calling ClearDatasetEvents. It means the calling method needs to know something about what ClearDatasetEvents uses and needs. It also means the same check has to be coded (and maintained) anywhere else this method winds up being called. I would do the checking inside ClearDataEvents itself.
Just a thought.
Yes, Lee, that’s correct.
Bruce, FDatasetEvents is allocated in the constructor, so the only place where such a check is necessary is in the destructor. The check doesn’t need to be coded anywhere else because if the constructor succeeds FDatasetEvents will exist. In other words, there’s no way to call the method, save for the constructor, where it will fail.
But I did add the assertion since it’s a precondition of the method. I don’t expect it to ever fail, however.
If FDatasetEvents is a list of objects, then it has some parameter to own or not to own the contained objects. If that flag is set, then on destruction (i.e. FDatasetEvents.Clear) it will try to FREE all the contained objects before removing them from the list!
In which case, you don’t even need to free them in your code, since FDatasetEvents.Clear will do that for you! Otherwise, if you want to keep the FREEing code - you need to change the value of the flag that is described above.
Hope this helps
Post a Comment