332239 - Saved xml content gives xml parsing error in this case

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
To reproduce:
- Open testcase
- Save the testcase, with "Save Page As"
- Open the saved page

Expected result:
The saved page should not give an xml parsing error.

Actual result:
XML parsing error.

This regressed between 2005-11-29 and 2005-11-30:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-11-29+06&maxdate=2005-11-30+07&cvsroot=%2Fcvsroot
I guess a regression from bug 301260?

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Peter Van der Beken [:peterv]]

The problem is that nsDocumentEncoder::SerializeNodeStart ends up calling nsEncoderNodeFixup::FixupNode, which clones the node. That node gets passed to nsXMLContentSerializer::AppendElementStart but the original node gets passed to nsXMLContentSerializer::AppendElementEnd and so we fail to detect that we need to pop namespaces.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm... Is that a regression?  Or did we do that all along?

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

This fixes all the problems. The first issue was that AppendElementStart gets the fixed up element, but AppendElementEnd gets the original element and so we fail to  pop the namespaces in AppendElementEnd because we look for the wrong owner in the namespace stack. I tried passing the fixed up element into AppendElementEnd instead, but that gets very complicated because AppendElementEnd needs to know if the original element has children to omit the end tag. So I've opted for passing in both original element and fixed up element to AppendElementStart, we can then store the namespaces with the original element. The second issue with the testcase was that it uses XBL and the XBL code copied the xmlns attributes without using the prefix, we ended up with two non-prefixed attributes on the div |xbl="http://www.mozilla.org/xbl"|. There might be other places where we don't use the prefix when setting the attribute.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217182 [details] [diff] [review]
v1

>Index: content/base/public/nsIContentSerializer.h
>   NS_IMETHOD AppendElementStart(nsIDOMElement *aElement,
>-                                PRBool aHasChildren,
>+                                nsIDOMElement *aFixedupElement,

IID rev?

That said, I think I would be happier if we left aElement as the fixed up element and passed in aOriginalElement.  That would mean we could avoid changes to most serializer impls, avoid subtly bitrotting any in-flight patches, etc.

It would also make the changes much easier to review; right now the fix for this bug is in the part of the code _not_ appearing in this diff... :(

>Index: content/xbl/src/nsXBLBinding.cpp
>-        mBoundElement->SetAttr(namespaceID, name, value2, PR_FALSE);
>+        mBoundElement->SetAttr(namespaceID, name, attrName->GetPrefix(),
>+                               value2, PR_FALSE);

So is this needed to fix this bug, or just a nice-to-have?  If the former, why?

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217254 [details] [diff] [review]
v1.1

r+sr=bzbarsky; please file a followup to sort out the prefix thing for attribute sets, and another followup to add a regression test for this bug once the document encoder is fully scriptable (that will be fixed in bug 330517).

Comment 8

[Peter Van der Beken [:peterv]]

Regression tests bug is bug 333060. The bug for adding assertions about xmlns prefix mapping is bug 333059.

Comment 9

[Scott MacGregor]

This bug appears to have caused Bug 333229 although I'm not sure how yet :). Backing it out fixes Bug 333229 for me.