Bug 41913 – XPath substring function does not correctly handle non-positive values for the position argument

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Steve Block]

In the XPath substring function, if a negative value is supplied for the position argument and a length argument is present, we reset the position argument to the first character of the string and adjust the length argument such that the final character in the substring is the same as it would have been if the length were counted from the original negative position value. We then return the substring using the new position and length.

However, in the case where no length argument is present, we call String::substring() with the negative position value. Since String::substring() takes two unsigned int arguments, the position value gets wrapped to a large integer and we likely return the empty string because the wrapped position value exceeds the string length. This seems incorrect and is inconsistent with the above case.

See Bug 41862 for some background.

[Alexey Proskuryakov]

Yes, substring("12345", -1) should return "12345". This is not only about negative numbers -substring("12345", 0) is also wrong for the same reason.

[Alexey Proskuryakov]

See <http://www.w3.org/TR/xpath/#function-substring>.

[Steve Block]

> Yes, substring("12345", -1) should return "12345". This is not only about
> negative numbers -substring("12345", 0) is also wrong for the same reason.
Yes, I meant non-positive numbers. Patch coming soon.

[Steve Block]

Created attachment 61054 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 61054 [details]
Patch

> +        we reset the position to 1. This matches the current behaviour when a length
> +        argument is supplied.

This fix is not really about consistency - there is a rigorous spec, and we now match it. Does the new test pass in Firefox?

r=me

[Steve Block]

> This fix is not really about consistency - there is a rigorous spec, and we 
> now match it.
OK, have updated the bug title and will land with an updated description

> Does the new test pass in Firefox?
Yes

[Steve Block]

Committed r63066: <http://trac.webkit.org/changeset/63066>