265933 - cloneNode() does not work for XTF elements

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Allan Beaufour]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: 

cloneNode does not seem to work for XTF elements.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Allan Beaufour]

Attachment: cloneNode test form

Here's an example of cloneNode for various elements, including a non-working
XForms <input> element.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: implement CloneNode for XTF wrapper elements

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 163568 [details] [diff] [review]
implement CloneNode for XTF wrapper elements

oops, I should handle also that case when xtf element is an attribute handler.

Comment 4

[Allan Beaufour]

Attribute handler or not, the patch fixes my problem :) Thanks.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Copying also the attributes from original attribute handler

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

r=me (at some point we'll need to consider whether content node properties
should be copied as well)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

>Index: content/xtf/src/nsXTFElementWrapper.cpp
>+  nsContentUtils::GetXTFServiceWeakRef()->CreateElement(&it, mNodeInfo);

This addrefs "it".

>+  nsCOMPtr<nsIDOMNode> kungFuDeathGrip = NS_STATIC_CAST(nsIDOMNode*, wrapper);

This addrefs "it" again.

>+  if (NS_SUCCEEDED(rv)) {
>+    kungFuDeathGrip.swap(*aResult);
>+  }

This does NOT release "it".

So this code leaks the new node, since it comes out of here with a refcount of
2, not 1.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: This should not leak.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 164012 [details] [diff] [review]
This should not leak.

- In nsXTFElementWrapper::CloneNode():

+  nsXTFElementWrapper* wrapper =
+    NS_STATIC_CAST(nsXTFElementWrapper*, NS_STATIC_CAST(nsIContent*,
it.get()));
+  nsresult rv = CopyInnerTo(wrapper, aDeep);
+
+  if (NS_SUCCEEDED(rv)) {
+    if (mAttributeHandler) {
+      PRUint32 innerCount = 0;
+      mAttributeHandler->GetAttributeCount(&innerCount);
+      for (PRUint32 i = 0; i < innerCount; ++i) {
+	 nsCOMPtr<nsIAtom> attrName;
+	 mAttributeHandler->GetAttributeNameAt(i, getter_AddRefs(attrName));
+	 if (attrName) {
+	   nsAutoString value;
+	   if (NS_SUCCEEDED(mAttributeHandler->GetAttribute(attrName, value)));
+	     it->SetAttr(kNameSpaceID_None, attrName, value, PR_TRUE);
+	 }
+      }
+    }
+    kungFuDeathGrip.swap(*aResult);
+  }

I'd flip that around to do the deep copy after copletely cloning the node
that's passed in, i.e. set attributes first to complete the root node, and then
copy children etc.

sr=jst either way though.

Comment 10

[Brian Ryner (not reading)]

checked in.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

clearing obsolete sr request