73160 - Handling of <?xml-stylesheet?> without href attribute

Status: VERIFIED FIXED

(Core :: XML, defect, P5)

Description

[basic]

When loading an xml file with a <?xml-stylesheet type="text/css"?> tag, mozilla
would just stop loading the page.

It doesn't give an error message, it doesn't display a blank page and it doesn't
even display an error message in the console.

In fact according to the console the page was successfully loaded.

I'm not sure what the right behavior should be, but the current behavior is bad.

Comment 1

[basic]

Attachment: an xhtml document with <?xml-stylesheet type="text/css"?>

Comment 2

[basic]

Attachment: an xhtml document with <?xml-stylesheet href="" type="text/css"?>

Comment 3

[basic]

The first attachment will display nothing.
The second attachment works.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

In fact the page loaded ok. The content area just does not get refreshed. If you
do view source, you will see the page loaded. But yeah, this is a valid bug and
should be fixed. I don't see it as too serious though, you will immediately
notice something is vevy, vevy wong if you try to it...

Comment 5

[clayton]

Will try to fix, probably trivial.  But breakage is obvious to any developer and
the workaround is simple.

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix, do not propagate error return if no href

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The fix was indeed simple. If there is no href attribute,
nsParserUtils::GetQuotedAttributeValue() will return NS_ERROR_FAILURE. If we
propagate that, we won't display the page. If there was an href but it was empty
we bail out early but do not return with an error value, hence the difference in
behaviour. With this fix we treat !href and href="" the same. The patch also
includes a couple of changes that might slightly improve performance on some
platforms. I wanted to remove all the NS_Convert... calls to NS_LITERAL_STRING,
but it turns out that would have required rather substantial changes elsewhere.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I did some measuring, and AssignWithConversion is faster than
Assign(NS_LITERAL_STRING()), so I decided not to change those, just the return
value (so the fix is one line change).

The fix is also included in the patch to bug 67887, in block @@ -1231,8 +1279,8 @@.

Comment 9

[harishd]

r=harishd

Comment 10

[vidur (gone)]

sr=vidur. We should confirm that no content sink codepaths force an
unintentional error return code. I'm guessing that implementors (including me)
weren't aware of the repercussions of returning error codes to the parser.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nisheeth says r=nisheeth

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fix checked in.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Really.

Comment 14

[Chris Petersen]

Marking verified in the May 25th build.