2

we have a strange effect using Delphi XE8 with FastMM4 (Version 4.992) in FullDebugMode.

To reproduce the effect, just create a new TForm application, put FastMM4 in the first line of the DPR file, put a Button on the Form and put the following code in the clickhandler:

(You need to have FastMM 4 installed, FullDebugMode must be enabled in the FastMM4Options.inc file and the FullDebugMode.dll has to be in the output folder of your programm!)

procedure TForm3.Button4Click(Sender: TObject);
var
    dm: tdatamodule;
    doc: txmldocument;
begin
    //this causes the messagebox to be shown directly after clicking the button. 
    //without it the box is shown when the program is exited.
    FastMM4.FullDebugModeScanMemoryPoolBeforeEveryOperation := true;        

    //prepare a xml document
    dm := tdatamodule.create(nil);
    doc := txmldocument.create(dm);
    doc.LoadFromXml('<?xml version="1.0" encoding="ISO-8859-1" standalone="yes"?><root/>');

    //doc.Active := true;   //no need to set active to true. doc is already active at this point!

    //the following does matter. ChildNodes needs to be accessed twice!
    //doc.DocumentElement.ChildNodes.FindNode('first');
    //doc.DocumentElement.ChildNodes.FindNode('second');

    //alternative access:
    doc.DocumentElement.ChildNodes.count;                                       //first access to ChildNodes
    doc.DocumentElement.ChildNodes.count;                                       //second access to ChildNodes

    //this does matter. if doc stays active then there is no problem!
    doc.Active := false;                                                        //active needs to be set to false!

    //doc.free; //doesn't matter if doc is freed manually. doc will be freed when datamodule is freed. problem occurs either way.
    dm.free;
end;

When you click the button, FastMM4 will show a large messagebox with the following report:

FastMM has detected an error during a free block scan operation. FastMM detected that a block has been modified after being freed. 

Modified byte offsets (and lengths): 80(1)

The previous block size was: 228

This block was previously allocated by thread 0xC74, and the stack trace (return addresses) at the time was:
406C46 [System.pas][System][@GetMem$qqri][4565]
407A73 [System.pas][System][TObject.NewInstance$qqrv][15975]
5DF25D [Xml.XMLDoc.pas][Xml.XMLDoc][Xmldoc.TXMLDocument.NewInstance$qqrv][2368]
408202 [System.pas][System][@ClassCreate$qqrpvzc][17290]
5DF116 [Xml.XMLDoc.pas][Xml.XMLDoc][Xmldoc.TXMLDocument.$bctr$qqrp25System.Classes.TComponent][2344]
5E2094 [Unit3.pas][Unit3][TForm3.Button4Click$qqrp14System.TObject][47]
407E6F [System.pas][System][@IsClass$qqrxp14System.TObjectp17System.TMetaClass][16465]
51EB39 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.Click$qqrv][7361]
535CF3 [Vcl.StdCtrls.pas][Vcl.StdCtrls][Stdctrls.TCustomButton.Click$qqrv][5327]
536801 [Vcl.StdCtrls.pas][Vcl.StdCtrls][Stdctrls.TCustomButton.CNCommand$qqrr26Winapi.Messages.TWMCommand][5788]
51E5C8 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7245]

The block was previously used for an object of class: TXMLDocument

The allocation number was: 650

The block was previously freed by thread 0xC74, and the stack trace (return addresses) at the time was:
406C62 [System.pas][System][@FreeMem$qqrpv][4613]
407A91 [System.pas][System][TObject.FreeInstance$qqrv][15984]
40824D [System.pas][System][@ClassDestroy$qqrxp14System.TObject][17333]
5DF241 [Xml.XMLDoc.pas][Xml.XMLDoc][Xmldoc.TXMLDocument.$bdtr$qqrv][2363]
4C3661 [System.Classes.pas][System.Classes][Classes.TComponent.DestroyComponents$qqrv][15648]
4C3100 [System.Classes.pas][System.Classes][Classes.TComponent.$bdtr$qqrv][15445]
4C5543 [System.Classes.pas][System.Classes][Classes.TDataModule.$bdtr$qqrv][16694]
407B97 [System.pas][System][TObject.Free$qqrv][16052]
5E20F2 [Unit3.pas][Unit3][TForm3.Button4Click$qqrp14System.TObject][63]
51EB39 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.Click$qqrv][7361]
535CF3 [Vcl.StdCtrls.pas][Vcl.StdCtrls][Stdctrls.TCustomButton.Click$qqrv][5327]

The current thread ID is 0xC74, and the stack trace (return addresses) leading to this error is:
416526 [fastmm4.pas][FastMM4][InternalScanMemoryPool$qqruiui][10018]
416601 [fastmm4.pas][FastMM4][ScanMemoryPoolForCorruptions$qqrv][10092]
4161DF [fastmm4.pas][FastMM4][DebugFreeMem$qqrpv][9761]
406C62 [System.pas][System][@FreeMem$qqrpv][4613]
407A91 [System.pas][System][TObject.FreeInstance$qqrv][15984]
40824D [System.pas][System][@ClassDestroy$qqrxp14System.TObject][17333]
407B8A [System.pas][System][TObject.$bdtr$qqrv][16044]
40D5DD [System.pas][System][TInterfacedObject._Release$qqsv][37311]
40D4B7 [System.pas][System][@IntfClear$qqrr44System.%DelphiInterface$17System.IInterface%][36327]
40B189 [System.pas][System][@FinalizeArray$qqrpvt1ui][31704]
40B079 [System.pas][System][@FinalizeRecord$qqrpvt1][31407]

Current memory dump of 256 bytes starting at pointer address 7EA18B60:
98 72 5F 00 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 7F 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
80 80 80 80 BD 4A 52 7A 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 00 00 00 00 00 00 00 00
˜  r  _  .  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €    €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €
€  €  €  €  ½  J  R  z  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  €  .  .  .  .  .  .  .  .

If we interpret this correctly, then FastMM is telling us, that the Memory of the TXMLDocument Object has been modified, after it was freed.

"Some Code" seems to have written the $7F in the middle of all these $80 in the memoryblock of the TXMLDocument that has already been freed.

This does happen only if ChildNodes is accessed twice (!) and if the Active property of the TXMLDocument is set to false before the Object is freed!

Questions:

Can someone explain what is going on here?

  • is setting TXMLDocument.Active to false generally considered wrong or "bad"? (Is it known to cause Problems?)

  • are we making some other mistake here?

  • is this a problem in FastMM4?

  • is this a problem in TXMLDocument?

Additional observation:

If the TXMLDocument is freed without active being set to false, then there is no problem.

If we look at the destructor of TXMLDocument we see that there is some additional code before active is set to false there:

destructor TXMLDocument.Destroy;
begin
  Destroying;
  if FOwnerIsComponent and Active and Assigned(FDocumentNode) and (FRefCount > 1) then      //additional code
    (FDocumentNode as IXMLNodeAccess).ClearDocumentRef;                                     //additional code
  SetActive(False);
  FreeAndNil(FXMLStrings);
  inherited;
end;

Now, if we modify our own examplecode and call 

(FDocumentNode as IXMLNodeAccess).ClearDocumentRef;

before setting active to false, then the problem is gone!

Code would look something like this:

type
  TMyXMLDocument = class(TXMLDocument);

procedure TForm3.Button4Click(Sender: TObject);
var
    dm: tdatamodule;
    doc: txmldocument;
begin
    FastMM4.FullDebugModeScanMemoryPoolBeforeEveryOperation := true;

    dm := tdatamodule.create(nil);
    doc := txmldocument.create(dm);
    doc.LoadFromXml('<?xml version="1.0" encoding="ISO-8859-1" standalone="yes"?><root/>');

    doc.DocumentElement.ChildNodes.count;
    doc.DocumentElement.ChildNodes.count;

    (TMyXMLDocument(doc).GetDocumentElement as IXMLNodeAccess).ClearDocumentRef;  //<-- with this  additional hack the problem is gone!

    doc.Active := false;                                                        //no more problem!

    dm.free;
end;
user2703897
  • 95
  • 6

1 Answers1

5

When calling methods/functions that return interfaces, the compiler implicitly generates variables to put these results into. They live until the end of the method and are then being finalized/cleared.

In your case doc.DocumentElement.ChildNodes does 2 method calls that return interfaces. Now when you destroy the TXMLDocument instance these implicit variables still point to some memory and due to the _Release call being made when the compiler generated code calls IntfClear they call into some method that has no object anymore - FastMM is able to track and report that.

So this mentioned call is translated into this:

var
  ...
  nodes1: IXMLNodeList;
  node1: IXMLNode;
  nodes2: IXMLNodeList;
  node2: IXMLNode;
begin
  ...

  node1 := doc.DocumentElement;
  nodes1 := node1.ChildNodes;
  nodes1.count;
  node2 := doc.DocumentElement;
  nodes2 := node2.ChildNodes;
  nodes2.count;

  dm.free;

  nodes1 := nil;
  node1 := nil;
  nodes2 := nil;
  node2 := nil; // <- boom

In many cases this error does not manifest without using FastMM unless the previously deallocated memory was not being reused for another allocation which then results in weird AVs.

Rule of thumb: do not destroy instances that are referenced by some interface in the same scope as this might lead to implicitly created interface variables still pointing to them.

Stefan Glienke
  • 20,860
  • 2
  • 48
  • 102
  • 1
    A better rule of thumb is *Use XMLDocument as an interface from the start, and don't ever free anything yourself; let the compiler handle it.* Declaring `XML: IXMLDocument;` and then `XML := LoadXMLDocument(....);` (and removing `XML.Free;` at the end) would eliminate the issue entirely. – Ken White Jul 19 '18 at 12:32
  • @KenWhite That would not be better but more specific for this exact case - but I prefer a more general advice because that can be applied to other cases where one might work with interfaces (as this does not only apply to mixing interface and object references) – Stefan Glienke Jul 19 '18 at 12:37
  • @StefanGlienke >In many cases this error does not manifest without using FastMM unless the previously deallocated memory was not being reused for another allocation which then results in weird AVs. - so is this really a memory corruption then? I was debugging strange random avs in a multithreaded application and resorted to fastmm4 as i was not finding the problem. Now fastmm pointed me to this situation and i am unsure if this problem here is the source of my strange avs or if it's something more related to fastmm... – user2703897 Jul 19 '18 at 12:49
  • 1
    FastMM is pointing out a defect in your code (accessing something that has been destroyed - aka dangling pointer) - what makes it a bit complicated to grasp is that its not you explicitly doing that but implicitly via the compiler generated code. Calling into the dangling pointer though could cause memory corruption because it might modify memory that belongs to someone else already. – Stefan Glienke Jul 19 '18 at 12:52
  • @StefanGlienke is it possible to somehow tell the compiler to free these interface variables immediately after the usage and not wait until the end of the scope? – user2703897 Jul 19 '18 at 13:14
  • Yes, you can set the interface variable to nil in order to release the interface. – Schneider Infosystems Ltd Jul 19 '18 at 13:21
  • @SchneiderInfosystemsLtd i was looking for a way that the compiler would do this for me automatically, because we have a lot of code and this problem seems hard to find manually – user2703897 Jul 19 '18 at 13:25
  • As soon the variable life time ends (e.g. for a local variable in a function/procedure when the funct/proc terminates) the compiler releases an interface variable automatically. So my tip was just in cases you wan't not wait until that time point (e.g. because of huge function or if you use a lot of interfaces just for a short operation). – Schneider Infosystems Ltd Jul 19 '18 at 13:33
  • @SchneiderInfosystemsLtd Yes, i understand. But the fact that the compiler will set the variables to nil at the end of the function **after** I freed the txmldocument is what is causing the problem for me (well it's of course our incorrect usage that causes the problem;) so i was looking for a way to tell the compiler to automatically immediately set these interface variables to nil **before** I free the txmldocument, so that I don't have to manually find all the code that does it wrong! – user2703897 Jul 19 '18 at 13:37
  • Can somebody explain why the problem does not manifest itself when the object is freed without Active being set to false or when (TMyXMLDocument(doc).GetDocumentElement as IXMLNodeAccess).ClearDocumentRef; is called? – user2703897 Jul 19 '18 at 13:56
  • a) there is no way to do that - either use explicit variables or don't mix interfaces and objects as explained before b) because then some internal interface references between the objects are being detached - my strong advice is to not bother or count on it. – Stefan Glienke Jul 19 '18 at 14:33
  • @StefanGlienke Thank you for the explanation! Do you happen to know where this behaviour is documented? Or some resources on the web (possibly from Embarcadero) where I could read more about the generation of these "hidden" variables? – user2703897 Jul 19 '18 at 14:57
  • Another way to ha dle this situation is to move all of the XML usage logic into another procedure, and then call that procedure after creating the XML Document, and then free it after the procedure exits. That way, all of the temp variables will be inside the procedure and go out of scope before the XMLDocument is freed. – Remy Lebeau Jul 19 '18 at 15:36
  • @RemyLebeau Thanks, that is probably what i will do. Would it be safe to use a local procedure for that? That way I still could access the local variables of the original procedure without passing them as parameters to the new encapsulation-procedure (I tried it and FastMM4 didn't report a problem, so it seems to be ok). – user2703897 Jul 20 '18 at 06:39