134295 - position() wrong for sorted nodesets (<xsl:sort>)

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Matthew Woodcraft]

From Bugzilla Helper:
BuildID:    2002032404

With an XSLT fragment like this:

  <xsl:for-each select="ele">
    <xsl:sort select="@attr"/>
    <xsl:value-of select="position()"/>
  </xsl:for-each>

The position() function should return values in order 1, 2, 3 and so on, no
matter what the sort does. Currently, it returns values with no apparent pattern.

Reproducible: Always
Steps to Reproduce:
1. Put the attached position.xml and position.xsl in the same directory
2. Open position.xml


Actual Results:  It shows
1
0


Expected Results:  Correct is
1
2

Explanation:

XPath 1.0 para 4.1 says:
|The position function returns a number equal to the context position
|from the expression evaluation context.

XSLT 1.0 para 4 says:
[when evaluating XPath expressions]
|the context position comes from the position of the current node in
|the current node list; the first position is 1

XSLT 1.0 para 10 says:
[Inside <xsl:sort>]
|the current node list list (sic) consists of the complete list of
|nodes being processed in sorted order



This worked correctly with Mozilla 0.9.8. It was wrong in Mozilla 0.9.9. I've
reproduced it on both GNU/Linux and Windows. The build-id above is actually from
the Debian mozilla-snapshot package.

With more complicated testcases, the position() values are irregular, but
frequently zero.

Comment 1

[Matthew Woodcraft]

Attachment: xml part of testcase

Comment 2

[Matthew Woodcraft]

Attachment: xslt part of testcase

Comment 3

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

uhoh, this is a regression, doing NodeSet::indexOf using NodeSet::findPosition
won't work for non-documentorder-sorted nodesets!

This specific testcase will probably be fixed by Pikes branch, but i'm not sure
if there could be any other cases where we'll fail anyway

Comment 4

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

for the record, it's not the for-each that is giving us trouble, it's the 
sorting.

Comment 5

[Axel Hecht]

two-fold problem:
- indexOf() is broken for our NodeSet impl if content isn't sorted in document
order.
- position() returns wrong number for xsl:sort.

The first one is an architecture bug for our nodesets, the second is more or
less a problem of the *right* context.

Fix for second is part of bug 113611. (tested on standalone)

(adding deps, adjusting topic)

Comment 6

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

Attachment: temporary workaround

for now i think we should do this. It might regress some uses of
attribute-nodes, but the current brokenness is IMHO far worse

Comment 7

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

taking since I caused the regression and i have the patch

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 81780 [details] [diff] [review]
temporary workaround

Fix the comment ("attribute-nodes arn't broken"). r=peterv

Comment 9

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

Comment on attachment 81780 [details] [diff] [review]
temporary workaround

checked in on trunk

Comment 10

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

Peter, do you think this would be serious enough for Netscape 7.0 RTM? What
does/could the patch break?

Comment 11

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

No point in keeping this open since it won't make the branch.

Comment 12

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

Huh? Mozilla 1.0.1 is long ways out, so this certainly has a chance to get in.
Make your case why we need this and we'll consider.

Comment 13

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

/me realizes there is a life after 1.0 :-)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 81780 [details] [diff] [review]
temporary workaround

Please land this on the 1.0.1 branch.  Once there, remove the
"mozilla1.0.1+" keyword, and add the "fixed1.0.1"


(This approval is based on the comment in sicking's email:  "The patch backs
out part of the patch from bug 85893 which caused a pretty
serious regression in the XPath engine.")

Comment 15

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

checked in to branch

Comment 16

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.