dom.pp:

* r15443 changed the node class with biggest instance size from TDOMAttr to TDOMEntity. Changed that in TDOMDocument constructor, too. Otherwise nodes created with TDOMEntity.CloneNode will leak (they cannot be inserted into tree).
* Do not restore default attributes during document destruction.
* Also added a general check that raises exception if someone tries to allocate from node pool during destruction.
* Fixed replaceChild() method: it was deleting node if that node was replaced by itself.
+ Test for replaceChild.

git-svn-id: http://svn.freepascal.org/svn/fpc/trunk@16010 3ad0048d-3df7-0310-abae-a5850022a9f2

2 files changed, 37 insertions, 5 deletions

diff --git a/packages/fcl-xml/src/dom.pp b/packages/fcl-xml/src/dom.pp
index 4c9631aaee..25e8b0dbf2 100644
--- a/packages/fcl-xml/src/dom.pp
+++ b/packages/fcl-xml/src/dom.pp
@@ -1403,7 +1403,7 @@ function TDOMNode_WithChildren.ReplaceChild(NewChild, OldChild: TDOMNode):
   TDOMNode;
 begin
   InsertBefore(NewChild, OldChild);
-  if Assigned(OldChild) then
+  if Assigned(OldChild) and (OldChild <> NewChild) then
     RemoveChild(OldChild);
   Result := OldChild;
 end;
@@ -1665,7 +1665,7 @@ var
   I: Integer;
 begin
   for I := FList.Count-1 downto 0 do
-    TDOMNode(FList[I]).Free;
+    TDOMNode(FList.List^[I]).Free;
   FList.Free;
   inherited Destroy;
 end;
@@ -2131,7 +2131,7 @@ constructor TDOMDocument.Create;
 begin
   inherited Create(nil);
   FOwnerDocument := Self;
-  FMaxPoolSize := (TDOMAttr.InstanceSize + sizeof(Pointer)-1) and not (sizeof(Pointer)-1) + sizeof(Pointer);
+  FMaxPoolSize := (TDOMEntity.InstanceSize + sizeof(Pointer)-1) and not (sizeof(Pointer)-1) + sizeof(Pointer);
   FPools := AllocMem(FMaxPoolSize);
   FNames := THashTable.Create(256, True);
   SetLength(FNamespaces, 3);
@@ -2163,6 +2163,8 @@ var
   pp: TNodePool;
   size: Integer;
 begin
+  if nfDestroying in FFlags then
+    raise EDOMError.Create(INVALID_ACCESS_ERR, 'Attempt to allocate node memory while destroying');
   size := (AClass.InstanceSize + sizeof(Pointer)-1) and not (sizeof(Pointer)-1);
   if size > FMaxPoolSize then
   begin
@@ -2250,7 +2252,9 @@ begin
      ((nType = DOCUMENT_TYPE_NODE) and (OldChild = DocType)) then   // and so can be DTD
   begin
     inherited InsertBefore(NewChild, OldChild);
-    Result := RemoveChild(OldChild);
+    Result := OldChild;
+    if OldChild <> NewChild then
+      RemoveChild(OldChild);
   end
   else
     Result := inherited ReplaceChild(NewChild, OldChild);
@@ -2709,7 +2713,8 @@ begin
   Include(FFlags, nfDestroying);
   if Assigned(FOwnerDocument.FIDList) then
     FOwnerDocument.RemoveID(Self);
-  FreeAndNil(FAttributes);
+  FAttributes.Free;
+  FAttributes := nil;
   inherited Destroy;
 end;
 
@@ -2816,6 +2821,8 @@ var
   ColonPos: Integer;
   AttrName, nsuri: DOMString;
 begin
+  if nfDestroying in FOwnerDocument.FFlags then
+    Exit;
   Attr := TDOMAttr(AttrDef.CloneNode(True));
   AttrName := Attr.Name;
   ColonPos := Pos(WideChar(':'), AttrName);
diff --git a/packages/fcl-xml/tests/extras.pp b/packages/fcl-xml/tests/extras.pp
index eabe20ffd6..c2db154d45 100644
--- a/packages/fcl-xml/tests/extras.pp
+++ b/packages/fcl-xml/tests/extras.pp
@@ -30,6 +30,7 @@ type
     procedure attr_ownership03;
     procedure attr_ownership04;
     procedure attr_ownership05;
+    procedure replacesamechild;
     procedure nsFixup1;
     procedure nsFixup2;
     procedure nsFixup3;
@@ -135,6 +136,30 @@ begin
   AssertNull('ownerElement_after', attr.ownerElement);
 end;
 
+// verify that replacing a node by itself does not remove it from the tree
+// (specs say this is implementation-dependent, but guess that means either
+//  no-op or raising an exception, not removal).
+procedure TDOMTestExtra.replacesamechild;
+var
+  doc: TDOMDocument;
+  root, el, prev, next: TDOMNode;
+begin
+  LoadStringData(doc, '<root><child1/><child2/><child3/></root>');
+  root := doc.DocumentElement;
+  el := root.ChildNodes[1];
+  prev := el.PreviousSibling;
+  next := el.NextSibling;
+  AssertEquals('prev_name_before', 'child1', prev.NodeName);
+  AssertEquals('next_name_before', 'child3', next.NodeName);
+  root.replaceChild(el, el);
+  prev := el.PreviousSibling;
+  next := el.NextSibling;
+  AssertNotNull('prev_after', prev);
+  AssertNotNull('prev_after', next);  
+  AssertEquals('prev_name_after', 'child1', prev.NodeName);
+  AssertEquals('next_name_after', 'child3', next.NodeName);
+end;
+
 const
   nsURI1 = 'http://www.example.com/ns1';
   nsURI2 = 'http://www.example.com/ns2';