352601 - XSLT forwards compatible processing enabled when it shouldn't

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Joao Eiras]

User-Agent:       Opera/8.53 (Windows NT 5.1; U; pt)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060913 Minefield/3.0a1

The XSLT specification states the following
  http://www.w3.org/TR/xslt.html#forwards
  An element enables forwards-compatible mode for itself, its attributes, its descendants and their attributes if either it is an xsl:stylesheet element whose version attribute is not equal to 1.0 (...)

however Gecko always parses any XSLT stylesheet with forwards compatible processing enabled, disregarding the version attribute of the stylesheet element, being it's behaviour therefore incompatible with other multiple XSLT engines.



Reproducible: Always

Steps to Reproduce:
Refer to the attached testcases
Actual Results:  
All testcases render just fine, without any xslt processing error.

Expected Results:  
All testcases that end with *-v1.1.xhtml have a xsl stylesheet attached with version 1.1, which must trigger forwards compatible processing, therefore these testcases must render well.
All testcases that end with *-v1.0.xhtml have a xsl stylesheet attached with version 1.0, which must result in a xslt processing error, because forwards compatible processing mustn't be enabled.

All xhtml files are the testcases.
All xsl files are the stylesheets.
The stylesheets must be placed in the same place as the testcases.

Considering the order of the 3 forward compatible processing rules at the online specification
http://www.w3.org/TR/xslt.html#forwards

files xslt-forwardcompat-topchildren-* test the xslt engine for rule 1
files xslt-forwardcompat-unknownelement-* test the xslt engine for rule 2
files xslt-forwardcompat-customattribute-* test the xslt engine for rule 3

Comment 1

[Joao Eiras]

Attachment: A zip file with the 6 testcases, and the 6 stylesheets

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

This adds an additional loop over the attributes. Let me know if you have a better idea.

Comment 3

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

Is this really right? I think you want to throw an error on *any* unknown attribute, even if it is in the xslt namespace.

Comment 4

[Joao Eiras]

Errors must be thrown for unknown elements and attribute in the xslt namespace. There rules aren't applicable in other namespaces.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #3)
> Is this really right? I think you want to throw an error on *any* unknown
> attribute, even if it is in the xslt namespace.

Hmm, that sounds wrong too. Shouldn't it be any unknown attribute in the xslt namespace and additionally for elements in the xslt namespace any unknown attribute in the null namespace?

Comment 6

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

Whatever the spec says :)

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> Whatever the spec says :)

"Vendors must not extend the XSLT namespace with additional elements or attributes."
...
"An element from the XSLT namespace may have any attribute not from the XSLT namespace, provided that the expanded-name of the attribute has a non-null namespace URI."
...
"It is an error for an element from the XSLT namespace to have attributes with expanded-names that have null namespace URIs (i.e. attributes with unprefixed names) other than attributes defined for the element in this document."

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 9

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

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

>Index: content/xslt/src/xslt/txStylesheetCompileHandlers.cpp

>+checkUnhandledStyleAttributes(txStylesheetAttr* aAttributes,
>+                              PRInt32 aAttrCount)
>+{

Pass in the state and move the fcp() test to inside this function to avoid having to do it at every callsite

>+    PRInt32 i;
>+    for (i = 0; i < aAttrCount; ++i) {
>+        txStylesheetAttr& attr = aAttributes[i];
>+        if ((attr.mNamespaceID == kNameSpaceID_XSLT ||
>+             attr.mNamespaceID == kNameSpaceID_None) &&
>+            attr.mLocalName) {

Move the attr.mLocalName test first for speed?



>@@ -495,16 +518,27 @@ txFnStartLREStylesheet(PRInt32 aNamespac
>                        PRInt32 aAttrCount,
>                        txStylesheetCompilerState& aState)
> {
>     txStylesheetAttr* attr;
>     nsresult rv = getStyleAttr(aAttributes, aAttrCount, kNameSpaceID_XSLT,
>                                txXSLTAtoms::version, PR_TRUE, &attr);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    if (!aState.fcp()) {
>+        PRInt32 i;
>+        for (i = 0; i < aAttrCount; ++i) {
>+            txStylesheetAttr& attr = aAttributes[i];
>+            if (attr.mNamespaceID == kNameSpaceID_XSLT && attr.mLocalName) {
>+                // XXX ErrorReport: unknown attribute
>+                return NS_ERROR_XSLT_PARSE_FAILURE;
>+            }
>+        }
>+    }
>+

You have to do this after the call to txFnStartLRE to not break xsl:use-attribute-sets. You can then call checkUnhandledStyleAttributes here if you set mLocalName to null in the loop in txFnStartLRE.

Actually, looks like you can remove this entierly since txFnStartLRE will check for you.

with that, r/sr=sicking

Comment 10

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

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

Oh, this breaks the extension-element-prefixes attribute since that attribute is handled in the stylesheet compiler itself rather than in any of the handlerfunctions.

Another thing that i'm worried about though is that there might still be some attributes that we don't parse. The current effect is simply that they are ignored, however with this patch we'd end up refusing to use the stylesheet entierly, even though it might be perfectly valid.

Could you check if that is the case?

Comment 11

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

Oh, would it be possible to move the checkUnhandledStyleAttributes call into the txStylesheetCompiler. That way we only have to call it in one place rather than in every handler-function.

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to comment #11)
> Oh, would it be possible to move the checkUnhandledStyleAttributes call into
> the txStylesheetCompiler. That way we only have to call it in one place rather
> than in every handler-function.

I did this, note that it does mean clearing the localname of XSLT attributes in txFnStartElementIgnore/txFnStartSetElementIgnore.

With this patch we regress two testcases in buster: lre07 and lre11. They both have an xsl:if attribute, which should mean the stylesheet is invalid. lre7 claims that it is allowed to have that attribute because the spec says for lre's: "The created element node will have the attribute nodes that were present on the element node in the stylesheet tree, other than attributes with names in the XSLT namespace." I don't think that should make us allow an xsl:if attribute.

There are still a number of elements that we allow to have children (xsl:apply-imports, xsl:copy-of, ...) or for which we allow children of the wrong type (xsl:apply-templates, xsl:attribute-set, ...). Was that a deliberate decision or should we have a bug filed for changing that?

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 14

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

Comment on attachment 239509 [details] [diff] [review]
v2

>Index: content/xslt/src/xslt/txStylesheetCompileHandlers.cpp
>@@ -148,6 +149,29 @@ parseUseAttrSets(txStylesheetAttr* aAttr
> }
> 
> nsresult
>+parseExcludeResultPrefixes(txStylesheetAttr* aAttributes,
>+                           PRInt32 aAttrCount,
>+                           PRInt32 aNamespaceID)
>+{
>+    txStylesheetAttr* attr = nsnull;
>+    nsresult rv = getStyleAttr(aAttributes, aAttrCount, aNamespaceID,
>+                               txXSLTAtoms::excludeResultPrefixes, PR_FALSE,
>+                               &attr);
>+    if (!attr) {
>+        return rv;
>+    }
>+
>+/*
>+    txTokenizer tok(attr->mValue);
>+    while (tok.hasMoreTokens()) {
>+        const nsASingleFragmentString& name = tokenizer.nextToken();
>+    }
>+*/

Just remove this and leave a comment stating that the implementation is missing.

>@@ -394,6 +418,20 @@ txFnTextError(const nsAString& aStr, txS
>     return NS_ERROR_XSLT_PARSE_FAILURE;
> }
> 
>+void
>+clearStyleAttributes(txStylesheetAttr* aAttributes,
>+                     PRInt32 aAttrCount)
>+{
>+    PRInt32 i;
>+    for (i = 0; i < aAttrCount; ++i) {
>+        txStylesheetAttr& attr = aAttributes[i];
>+        if (attr.mNamespaceID == kNameSpaceID_XSLT ||
>+            attr.mNamespaceID == kNameSpaceID_None) {
>+            attr.mLocalName = nsnull;
>+        }

Alternativly you could simply clear all attributes. I'm fine either way.

>@@ -841,6 +898,35 @@ txFnEndKey(txStylesheetCompilerState& aS
>     return NS_OK;
> }
> 
>+// xsl:namespace-alias
>+nsresult
>+txFnStartNamespaceAlias(PRInt32 aNamespaceID,

Add a comment to this function stating that the implementation is missing.

>@@ -1235,9 +1324,14 @@ txFnStartLRE(PRInt32 aNamespaceID,
>         attr = aAttributes + i;
>         
>         if (attr->mNamespaceID == kNameSpaceID_XSLT) {
>+            if (attr->mLocalName && !aState.fcp()) {
>+                // XXX ErrorReport: unknown attribute
>+                return NS_ERROR_XSLT_PARSE_FAILURE;
>+            }
>+

Is this needed? Won't the stylesheet compiler catch this?

>Index: content/xslt/src/xslt/txStylesheetCompiler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xslt/src/xslt/txStylesheetCompiler.cpp,v
>retrieving revision 1.26
>diff -u -p -r1.26 txStylesheetCompiler.cpp
>--- content/xslt/src/xslt/txStylesheetCompiler.cpp	19 May 2006 10:29:43 -0000	1.26
>+++ content/xslt/src/xslt/txStylesheetCompiler.cpp	21 Sep 2006 13:32:30 -0000
>@@ -284,6 +284,8 @@ txStylesheetCompiler::startElementIntern
>                     return NS_ERROR_OUT_OF_MEMORY;
>                 }
>             }
>+
>+            attr->mLocalName = nsnull;
>         }
> 
>         // version
>@@ -338,6 +340,18 @@ txStylesheetCompiler::startElementIntern
> 
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    if (aNamespaceID == kNameSpaceID_XSLT && !fcp()) {

Why only do this for xslt elements?

r/sr=sicking with that fixed or explained.

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to comment #14)
> >@@ -1235,9 +1324,14 @@ txFnStartLRE(PRInt32 aNamespaceID,
> >         attr = aAttributes + i;
> >         
> >         if (attr->mNamespaceID == kNameSpaceID_XSLT) {
> >+            if (attr->mLocalName && !aState.fcp()) {
> >+                // XXX ErrorReport: unknown attribute
> >+                return NS_ERROR_XSLT_PARSE_FAILURE;
> >+            }
> >+
> 
> Is this needed? Won't the stylesheet compiler catch this?

I added it here because we were already looping over the attributes (I changed this, see below).

> >@@ -338,6 +340,18 @@ txStylesheetCompiler::startElementIntern
> > 
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+    if (aNamespaceID == kNameSpaceID_XSLT && !fcp()) {
> 
> Why only do this for xslt elements?

Because the others were handled in txFnStartLRE. Anyway, I removed the stuff in txFnStartLRE, it'll be a little more costly now (especially for LREs) but we only check attributes in one place then.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 17

[Peter Van der Beken [:peterv]]

All checked in, with an additional fix (http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/xslt/src/xslt&command=DIFF_FRAMESET&file=txStylesheetCompileHandlers.cpp&rev1=1.20&rev2=1.21&root=/cvsroot)

Comment 18

[Joao Eiras]

Great ! This is fixed...
However, I get a toooooooooooooo vague error message wehn loading those testcases:
  
     Error loading stylesheet: (null)


Hardly something easy to debug. Keep up the good work.