448312 - Failure when parsing XPath expressions with nested predicates

Status: RESOLVED DUPLICATE of bug 466712

(Core Graveyard :: XForms, defect)

Description

[John L. Clark]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.16) Gecko/20080715 Ubuntu/7.10 (gutsy) Firefox/2.0.0.16
Build Identifier: 0.8.5

In some cases, XPath expressions with nested predicates cause the XForms XPath-parsing code to generate an XPathCompilerException.  (In debug builds, this causes Firefox to crash.)  I will attach a test case shortly.

Reproducible: Always

Comment 1

[John L. Clark]

Attachment: Simple test case that demonstrates this bug

Comment 2

[John L. Clark]

Attachment: Test case showing a very similar XPath expression working in core DOM code

Comment 3

[John L. Clark]

When you load attachment #1 [details] [diff] [review] in a debug build of the trunk, Firefox crashes, giving the following traceback:

(gdb) bt

<snip/>


#10 0xb7d513fd in Abort (

    aMsg=0xbf8ca3b4 "###!!! ABORT: file /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp, line 59")

    at /home/john/development/mozilla/xpcom/base/nsDebugImpl.cpp:371

#11 0xb7d5198d in NS_DebugBreak_P (aSeverity=3, aStr=0x0, aExpr=0x0, 

    aFile=0xb498d60c "/home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp", aLine=59)

    at /home/john/development/mozilla/xpcom/base/nsDebugImpl.cpp:326

#12 0xb7da5625 in NS_DebugBreak (aSeverity=3, aStr=0x0, aExpr=0x0, 

    aFile=0xb498d60c "/home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp", aLine=59)

    at /home/john/development/mozilla/xpcom/stub/nsXPComStub.cpp:264

#13 0xb493eb93 in XPathCompilerException (aMsg=0xb498db94 "Expected ]", aExpression=@0xbf8caaa8, aOffset=48, aLength=1)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:59

#14 0xb493f849 in nsXFormsXPathParser::Predicate (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:602

#15 0xb493f968 in nsXFormsXPathParser::Step (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:779

#16 0xb493efc7 in nsXFormsXPathParser::RelativeLocationPath (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:713

#17 0xb493f2ea in nsXFormsXPathParser::PathExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:561

#18 0xb493f3c1 in nsXFormsXPathParser::UnionExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:841

#19 0xb493f475 in nsXFormsXPathParser::UnaryExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:794

#20 0xb493f489 in nsXFormsXPathParser::MultiplicationExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:422

#21 0xb493f502 in nsXFormsXPathParser::AdditiveExpression (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:215

#22 0xb493f577 in nsXFormsXPathParser::RelationalExpression (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:656

#23 0xb493f63d in nsXFormsXPathParser::EqualityExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:317

#24 0xb493f6b9 in nsXFormsXPathParser::AndExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:239

#25 0xb493f6f7 in nsXFormsXPathParser::OrExpr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:513

#26 0xb493f735 in nsXFormsXPathParser::Expr (this=0xbf8caaa0)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:342

#27 0xb493f7a0 in nsXFormsXPathParser::Parse (this=0xbf8caaa0, aExpression=@0xbf8cae68)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsXPathParser.cpp:871

#28 0xb48f07c6 in nsXFormsUtils::EvaluateXPath (aExpression=@0xbf8cae68, aContextNode=0xad31683c, aResolverNode=0xad319a9c, 

    aResultType=9, aResult=0xbf8caefc, aContextPosition=1, aContextSize=1, aSet=0xad2a4a08, aIndexesUsed=0xbf8cafe8)

    at /home/john/development/mozilla/extensions/xforms/nsXFormsUtils.cpp:605


<snip/>

Comment 4

[John L. Clark]

Attachment: Attempt 1 to fix this bug

In almost all cases, the `PrimaryExpr` method was returning PR_FALSE, which was short circuiting `FilterExpr` from ever processing the following predicate.  Since `FilterExpr` checks the next token itself, there is no need to have `PrimaryExpr` return a boolean at all.  This also makes it more consistent with the remaining parse tree methods.

Comment 5

[aaronr]

From what I can tell with my quick glance at the code, we expect to encounter predicates either through nsXFormsXPathParser::Step or just after a function (when we currently return PR_TRUE from PrimaryExpr).  What your change will do would also allow predicates after parentheses (which I assume is the case you are encountering) but also recognizing predicates after variables, literals, and numbers and I don't know if those cases are valid.

cc'ing Allan (who wrote this code) and Jonas (an xpath expert) on this bug so that they can give their opinions.  I would think that you'd just want to fix the paren case but I could be wrong.

Comment 6

[John L. Clark]

(In reply to comment #5)
> From what I can tell with my quick glance at the code, we expect to encounter
> predicates either through nsXFormsXPathParser::Step or just after a function
> (when we currently return PR_TRUE from PrimaryExpr).  What your change will do
> would also allow predicates after parentheses (which I assume is the case you
> are encountering) but also recognizing predicates after variables, literals,
> and numbers and I don't know if those cases are valid.

They are valid according to the grammar, but they are not valid when the expression is evaluated:

"It is an error if the expression to be filtered does not evaluate to a node-set." -- Section 3.3 of the XPath 1.0 Rec

To me, this means that the parser should create the parse tree according to the grammar, and then the evaluator should reject cases where the above rule is violated.  I tested a DOM-only case using a literal before a predicate, and it triggered an assertion in the evaluator:

WARNING: NS_ENSURE_TRUE(exprRes->getResultType() == txAExprResult::NODESET) failed: file /home/john/development/mozilla/content/xslt/src/xpath/txFilterExpr.cpp, line 68

With my patch, a similar XPath expression in an XForm triggered the same assertion.

Comment 7

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

Yes, the syntax allows

(expression)[1]

But evaluation will fail if 'expression' doesn't produce a nodeset. This is similar to

count(expression)

which always parses fine, but fails during evaluation if the expression doesn't return a nodeset.

Comment 8

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

Expressions like

$foo[1]

are not valid though, even if $foo returns a nodeset. So the parenthesis are required.

Comment 9

[John L. Clark]

(In reply to comment #8)
> Expressions like
> 
> $foo[1]
> 
> are not valid though, even if $foo returns a nodeset. So the parenthesis are
> required.

They're legal according to the grammar, and if $foo returns a nodeset, then they're legal according to the above processing rule, so I don't see a problem with this.  Also, I just ran a test of an expression such as this through two different XSLT processors (4Suite and libxml2), and they both successfully processed a filtered variable.

Comment 10

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

Ah, you are indeed correct, and it looks the transformiix does this correctly too.

Comment 11

[aaronr]

Comment on attachment 331756 [details] [diff] [review]
Attempt 1 to fix this bug

Sounds like John and Jonas agree that my concerns can be ignored.  Patch looks good.

Comment 12

[aaronr]

Normally, I'd dupe the newer bug against the older bug, but since that patch already has two reviews and this one only has one, I'll do it the other way around this time.