230360 - XML prettyprinter doesn't clean up namespaces properly

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

XML prettyprinting (eg the URL in the URL field of this bug) triggers the
"Namespaces left on the stack!" assert I added in bug 230236.

This means that namespaces are not getting popped when they should be and that
some nodes could end up with incorrect namespace resolution...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This would fix it....

The problem is that the CSSLoader is disabled for "load as data" documents, so
attempts to load styles return NS_ERROR_NOT_AVAILABLE and this code was bailing
out too early.

The fallout is that namespaces declared on the <link> in question would stay on
the namespace stack, which is _not_ desirable... ;)

I'm not sure this patch is the right way to go, but I'm pretty sure we want to
do something along these lines.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Over to xslt, since this is a general problem with XSL stylesheets...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138588 [details] [diff] [review]
This would fix it....

IMHO it would be better if  the error was cought in
nsXMLContentSink::CloseElement and converted to a successcode.
However there are a lot more errors in CloseElement that would need catching
and errorhandling in the contentsink is generally fubared so I'm fine with this
too.
Please add a comment though.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and this code is not specific to XSLT, it's used for loading all xml

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hey, I'm happy with catching the error and whatnot.  But yes, CloseElement can
error in all sorts of ways....

One thing that bothers me is whether we should append the node if CloseElement
failed.  Should we?  Should OOM errors in CSSLoader or CloseElement or something
CloseElement calls prevent the node being added to the doc, eg?

Perhaps the right course of action is to add a new error code that CSSLoader can
use here?   Then catch that and convert it to NS_OK (and do the same for
HTMLPARSER_BLOCK, maybe?).

I'd really like toward having sane error-handling in the sink instead of the
state of things we have now....

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> One thing that bothers me is whether we should append the node if CloseElement
> failed.  Should we?  Should OOM errors in CSSLoader or CloseElement or something
> CloseElement calls prevent the node being added to the doc, eg?

IMHO we should not add the node, or do anything else, if CloseElement fails. The
only exception I can think of is that we might need to put the contensink into a
'good' state again (which includes popping namespaces as appropriate).

This seems hard though given the number of ways CloseElement can fail. Which
brings us to what IMHO is the real problem. A lot of the operations happeing
here should not return error-codes but rather different successcodes. An
errorcode indicates that things have failed (and a failed functioncall should
ideally not have any sideeffects) but IMHO nothing has failed when you update
stylesheets in a non-styled document, or when you insert a script and need to
block the parser.

These are pretty big changes though and I don't think we should hold this bug
for that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138588 [details] [diff] [review]
This would fix it....

I agree that parser blocking is not a failure code (and I just need to remove
that crap for sheets).	But failing to load a sheet when you have been asked to
should certainly throw, from the CSSLoader's point of view....

That said, if you and peterv are both happy with this interim patch...

Imo the right course of action for a failing CloseElement is to immediately
propagate the error out and abort the document load at this point (pop the
namespace stack, cancel the channel, close out all open nodes).  That should
quickly catch bogus error returns... ;)

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sure, failing a load should throw from the cssloader, but
nsIStyleSheetLinkingElement::UpdateStyleSheet should probably not propagate that
error. And it should certainly not propagate an error if the stylesheetload
fails because this is a datadocument. All IMHO of course :)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 138588 [details] [diff] [review]
This would fix it....

   result = CloseElement(content, &appendContent);
-  NS_ENSURE_SUCCESS(result, result);

   if (mDocElement == content) {
     mState = eXMLContentSinkState_InEpilog;
   }
   else if (appendContent) {
     nsCOMPtr<nsIContent> parent = GetCurrentContent();
     NS_ENSURE_TRUE(parent, NS_ERROR_UNEXPECTED);

     parent->AppendChildTo(content, PR_FALSE, PR_FALSE);
   }

   nsINameSpace* nameSpace = PopNameSpaces().get();
   NS_IF_RELEASE(nameSpace);
+
+  NS_ENSURE_SUCCESS(result, result);

I'd prefer to see the ammount of code between the CloseElement() call and the
error check for that call be as small as possible. Would it not be better to
pop the namespace immediately after calling CloseElement(), and then do the
error check, and only after that's done go ahead and append to the parent etc?

sr=jst either way.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to jst's comment

Comment 12

[Boris Zbarsky [:bzbarsky]]

And checked in.  sicking, mind filing a follow-up on what you think the right
behavior would be for cssloader and StyleSheetLinkingElement?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

filed bug 231798