277777 - (XPath-FCP) Unevaluated XPath 2.0 expressions throwing errors

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Robin Lionheart]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

XSLT 1.0 processors handling an XSLT 2.0 must not raise an error on unevaluated
XPath expressions. Mozilla and family should not be halting on code like:

	<xsl:when test="system-property('xsl:version') >= 2">
		<xsl:if test="some $x in ./* satisfies local-name(.)='bar'">
			<p>bar exists</p>
		</xsl:if>
 	</xsl:when>

because the XPath 2.0 expression is wrapped such that Mozilla's XSLT 1.0
processor can never evaluate it.

According to the XSLT spec, section 2.5:

   If an expression occurs in an attribute that is processed in
   forwards-compatible mode, then an XSLT processor must recover
   from errors in the expression as follows:

   * if the expression does not match the syntax allowed by the
   XPath grammar, then an error must not be signaled unless the
   expression is actually evaluated;


Reproducible: Always

Steps to Reproduce:

Comment 1

[Axel Hecht]

I'd like to see a testcase for this.
See http://www.mozilla.org/projects/xslt/bug-reporting.html, Creating testcases.

Comment 2

[Robin Lionheart]

Attachment: XSL style sheet demonstrating bug

Comment 3

[Robin Lionheart]

Attachment: Test Case - trivial XML document invoking XSL attachment

Here's your test case.

Comment 4

[Axel Hecht]

http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/xslt/txStylesheetCompileHandlers.cpp#194
is the culprit. We require that the expr is both optional and that it is in fcp.

Jonas is our god of reading the fcp parts of the spec (and it is his code, too), 
I wonder what he thinks.

Comment 5

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

Ugh, it indeed seems like we're doing the wrong thing here. We're not supporting
forwards compatible parsing of expressions well at all. Apparently I only did
fcp of elements, not of expressions.

Comment 6

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

What we should do is to create an ErrorExpr that always throws an error (or just
use txErrorFunctionCall). Then make getExprAttr and getAVTAttr return an
instance of this when the parsing fails and fcp is enabled.

Additionally we should set fnCall to a new txErrorFunctionCall at
http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/ExprParser.cpp#553
when fcp is enabled.

Both these things could be done always rather then when fcp is enabled if we
wanted to.

Comment 7

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

Note that the current check in getAVTAttr and getExprAttr is correct. It is
implementing the third bullet in the first list in section 2.5. An additional
check is needed though to implement the first bullet in the second list.

Comment 8

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

Err, the change shouldn't be made in ExprParser since that'll make it impossible
for extensions (like xslt) to implement extensions in the null namespace. But we
should rather add code to txStylesheetCompilerState::resolveFunctionCall.

That will also make it trivial to only do it in fcp mode.

Comment 9

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

Pike pointed out that we already take care of unrecongized functions. So all
that's needed is the first paragraph of comment 6.

Though I wonder if we handle unrecongized functions in a good way. The current
solution makes us parse bad functions in DOM-XPath, though they'll fail when
evaluated. Either we could push the txErrorFunctionCall creation to the
resolveFunctionCall implementors, or we could add an inFCP() function to
txIParseContext and only create the txErrorFunctionCall then. I think I like the
latter better.

Comment 10

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

I have a patch in the works for this

Comment 11

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

Attachment: patch to fix

Comment 12

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

crap! my editor took it upon itself to do some whitespace cleanup. I won't check
the ws only stuff in.

Comment 13

[Robin Lionheart]

Attachment: Testcase: stylesheet w/ XPath 2 expression evaluated only by XSLT 2

Correct logic of XPath 2.0 expression, though this doesn't affect demonstration
of bug.

Comment 14

[Robin Lionheart]

Attachment: Testcase: trivial XML document invoking bug-demo stylesheet

Changed URL on stylesheet to revised stylesheet attachment.

Comment 15

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

Attachment: patch to fix v1.1

without whitespace 'cleanup's. Same otherwise

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 171821 [details] [diff] [review]
patch to fix v1.1

>Index: source/xslt/txStylesheetCompileHandlers.cpp
>===================================================================

>@@ -214,15 +224,29 @@ getAVTAttr(txStylesheetAttr* aAttributes
>                                aName, aRequired, &attr);
>     if (!attr) {
>         return rv;
>     }
> 
>     aAVT = txExprParser::createAttributeValueTemplate(attr->mValue, &aState);
>-    if (!aAVT && (aRequired || !aState.fcp())) {
>-        // XXX ErrorReport: XPath parse failure
>-        return NS_ERROR_XPATH_PARSE_FAILURE;
>+    if (!aAVT) {

>+        if (aRequired) {

>+        }
>+        else {
>+            aAVT = nsnull;

You can drop this, aAVT is already null.

>--- /dev/null	2005-01-20 01:48:08.375000000 +0100
>+++ source/xpath/txErrorExpr.cpp	2005-01-19 16:59:09.718750000 +0100

>+ * Portions created by the Initial Developer are Copyright (C) 2002

2005

Comment 17

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

check in, thanks for reviews

Comment 18

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

Robin: please mark this bug verified if things now work as they should.

Comment 19

[Robin Lionheart]

Works now in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b)
Gecko/20050125. Marking verified.