Browse Source

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: trunk@16010 -

sergei 15 years ago
parent
commit
2786259d77
2 changed files with 37 additions and 5 deletions
  1. 12 5
      packages/fcl-xml/src/dom.pp
  2. 25 0
      packages/fcl-xml/tests/extras.pp

+ 12 - 5
packages/fcl-xml/src/dom.pp

@@ -1403,7 +1403,7 @@ function TDOMNode_WithChildren.ReplaceChild(NewChild, OldChild: TDOMNode):
   TDOMNode;
   TDOMNode;
 begin
 begin
   InsertBefore(NewChild, OldChild);
   InsertBefore(NewChild, OldChild);
-  if Assigned(OldChild) then
+  if Assigned(OldChild) and (OldChild <> NewChild) then
     RemoveChild(OldChild);
     RemoveChild(OldChild);
   Result := OldChild;
   Result := OldChild;
 end;
 end;
@@ -1665,7 +1665,7 @@ var
   I: Integer;
   I: Integer;
 begin
 begin
   for I := FList.Count-1 downto 0 do
   for I := FList.Count-1 downto 0 do
-    TDOMNode(FList[I]).Free;
+    TDOMNode(FList.List^[I]).Free;
   FList.Free;
   FList.Free;
   inherited Destroy;
   inherited Destroy;
 end;
 end;
@@ -2131,7 +2131,7 @@ constructor TDOMDocument.Create;
 begin
 begin
   inherited Create(nil);
   inherited Create(nil);
   FOwnerDocument := Self;
   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);
   FPools := AllocMem(FMaxPoolSize);
   FNames := THashTable.Create(256, True);
   FNames := THashTable.Create(256, True);
   SetLength(FNamespaces, 3);
   SetLength(FNamespaces, 3);
@@ -2163,6 +2163,8 @@ var
   pp: TNodePool;
   pp: TNodePool;
   size: Integer;
   size: Integer;
 begin
 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);
   size := (AClass.InstanceSize + sizeof(Pointer)-1) and not (sizeof(Pointer)-1);
   if size > FMaxPoolSize then
   if size > FMaxPoolSize then
   begin
   begin
@@ -2250,7 +2252,9 @@ begin
      ((nType = DOCUMENT_TYPE_NODE) and (OldChild = DocType)) then   // and so can be DTD
      ((nType = DOCUMENT_TYPE_NODE) and (OldChild = DocType)) then   // and so can be DTD
   begin
   begin
     inherited InsertBefore(NewChild, OldChild);
     inherited InsertBefore(NewChild, OldChild);
-    Result := RemoveChild(OldChild);
+    Result := OldChild;
+    if OldChild <> NewChild then
+      RemoveChild(OldChild);
   end
   end
   else
   else
     Result := inherited ReplaceChild(NewChild, OldChild);
     Result := inherited ReplaceChild(NewChild, OldChild);
@@ -2709,7 +2713,8 @@ begin
   Include(FFlags, nfDestroying);
   Include(FFlags, nfDestroying);
   if Assigned(FOwnerDocument.FIDList) then
   if Assigned(FOwnerDocument.FIDList) then
     FOwnerDocument.RemoveID(Self);
     FOwnerDocument.RemoveID(Self);
-  FreeAndNil(FAttributes);
+  FAttributes.Free;
+  FAttributes := nil;
   inherited Destroy;
   inherited Destroy;
 end;
 end;
 
 
@@ -2816,6 +2821,8 @@ var
   ColonPos: Integer;
   ColonPos: Integer;
   AttrName, nsuri: DOMString;
   AttrName, nsuri: DOMString;
 begin
 begin
+  if nfDestroying in FOwnerDocument.FFlags then
+    Exit;
   Attr := TDOMAttr(AttrDef.CloneNode(True));
   Attr := TDOMAttr(AttrDef.CloneNode(True));
   AttrName := Attr.Name;
   AttrName := Attr.Name;
   ColonPos := Pos(WideChar(':'), AttrName);
   ColonPos := Pos(WideChar(':'), AttrName);

+ 25 - 0
packages/fcl-xml/tests/extras.pp

@@ -30,6 +30,7 @@ type
     procedure attr_ownership03;
     procedure attr_ownership03;
     procedure attr_ownership04;
     procedure attr_ownership04;
     procedure attr_ownership05;
     procedure attr_ownership05;
+    procedure replacesamechild;
     procedure nsFixup1;
     procedure nsFixup1;
     procedure nsFixup2;
     procedure nsFixup2;
     procedure nsFixup3;
     procedure nsFixup3;
@@ -135,6 +136,30 @@ begin
   AssertNull('ownerElement_after', attr.ownerElement);
   AssertNull('ownerElement_after', attr.ownerElement);
 end;
 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
 const
   nsURI1 = 'http://www.example.com/ns1';
   nsURI1 = 'http://www.example.com/ns1';
   nsURI2 = 'http://www.example.com/ns2';
   nsURI2 = 'http://www.example.com/ns2';