903391 - String.prototype.lastIndexOf needs to evaluate both arguments

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
"".lastIndexOf("a", {valueOf:function(){throw 0}})
---

Expected: second argument is evaluated and `0` is thrown

Actual: lastIndexOf() returns -1

Comment 1

[Till Schneidereit [:till]]

Attachment: never skip evaluating String#lastIndexOf's second argument.

Yeah, we're a bit over-eager, here. Easy enough to fix, though.

Comment 2

[André Bargull [:anba]]

`"".lastIndexOf("\0", 0)` returns 0 instead of -1 with the attached patch. 

Actually even `"".lastIndexOf("\0" + "\uDADA".repeat(7), -1)` returns 0 for me in a debug build, but I cannot tell whether it's always 0xDADA, because I don't know which magic numbers are used in SpiderMonkey. So basically it's possible to access memory after `textstr`.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 788242 [details] [diff] [review]
never skip evaluating String#lastIndexOf's second argument.

Clearly, I'm not smart enough to do this without painstakingly following the spec steps. *sigh*.

I'll upload a new patch doing just that once I get around to it. That won't be before my pto next week, though.

Comment 4

[André Bargull [:anba]]

Attachment: bug903391.patch

New patch. Also added steps references and renamed variables to match current spec.

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf94147997f0676e8a12b2416eadd8dcc32676

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8802257 [details] [diff] [review]
bug903391.patch

Review of attachment 8802257 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks.

::: js/src/jsstr.cpp
@@ +1756,5 @@
>          return true;
>      }
> +    MOZ_ASSERT(0 <= start && size_t(start) < len);
> +
> +    JSLinearString* text = str->ensureLinear(cx);

Not in this bug, but I wonder if it'd make sense to not require a linear string here and operate on ropes instead. Could well be that in any relevant case the cost of linearization is amortized pretty quickly, though.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0a1da93428
Evaluate both arguments in String.prototype.lastIndexOf. r=till

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f0a1da93428