353364 - Same-document references in xml-stylesheet PI processed incorrectly

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Bjoern Hoehrmann]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.2; SV1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20060616 Minefield/3.0a1

<?xml-stylesheet href='' type='text/xsl'?> should be equivalent to
<?xml-stylesheet href='...url of document...' type='text/xsl'?> and
hence the attached test case render as "This test did pass."

Reproducible: Always

Comment 1

[Bjoern Hoehrmann]

Attachment: test case

Comment 2

[Martin]

Attachment: Possible fix

The href.isEmpty() check and early return looks wrong to me, but not totally clear from tracing through the code if removing it would expose a problem lower down.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 345633 [details] [diff] [review]
Possible fix

>=== modified file 'content/xml/content/src/nsXMLStylesheetPI.cpp'

>   nsAutoString href;
>   GetAttrValue(nsGkAtoms::href, href);
>-  if (href.IsEmpty()) {
>-    return;
>-  }

Actually, you want

  nsAutoString href;
  if (!GetAttrValue(nsGkAtoms::href, href)) {
    return;
  }

The attribute is still required.

Comment 4

[Martin]

Attachment: Actual fix

Good catch, had not considered that case. With hope of an eyeball-only fix gone, I actually built and tried this one...

What's the prefered method of doing automated testing on XSLT things?

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 345821 [details] [diff] [review]
Actual fix

>=== modified file 'content/xml/document/src/nsXMLContentSink.cpp'

> nsXMLContentSink::ParsePIData(const nsString &aData, nsString &aHref,

>   aIsAlternate = alternate.EqualsLiteral("yes");
>+  return PR_TRUE;

Add a newline before the return.

Comment 6

[Martin]

Attachment: Some of my xml-stylesheet bits adapted to reftest form

I omitted some tests which depend on features is not implemented in mozilla - notably bug 109553 and bug 275196 - and some where I couldn't find the behaviour actually specified. Of particular relevance to this bug, lreas_selflink_empty_href.svg and xslt_selflink_empty_href.xslt pass with the patch applied, and error_no_href.svg catches mistakes of the class pointed out in the first review.

Now the tree is open again I'd like to get this landed. It's a low-risk fix of a minor bug, with no api/performance/etc repercussions. I can provide a bundle with the nit above addressed and tests included.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345821 [details] [diff] [review]
Actual fix

a191=beltzner, please do include the bundle as described

Comment 8

[Martin]

Attachment: Bundle for merging

Comment 9

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/96b47ad640e9
http://hg.mozilla.org/mozilla-central/rev/a80045f6089e

Comment 10

[Shawn Wilsher :sdwilsh]

I had to mark a test added by this bug as failing on linux:
http://hg.mozilla.org/mozilla-central/rev/120131b3a086

Comment 11

[Shawn Wilsher :sdwilsh]

Bah.  I had the nuke the test completely from the manifest file - it was still timing out, so apparently marking it as failing on one platform doesn't catch timeouts:
http://hg.mozilla.org/mozilla-central/rev/58bae41ad72d

Comment 12

[Martin]

My best guess is it's a mime-types registry issue. If the .xslt extension isn't mapped to an xml type but instead defaults to application/octet-stream, it might prompt for download, blocking completion. I've got the file renamed to end .xml and a couple more tests along the same lines in my tree, will try to find someone to try it out.

Comment 13

[Martin]

Attachment: Re-enable problem test and add a couple more