Closed
Bug 903391
Opened 12 years ago
Closed 8 years ago
String.prototype.lastIndexOf needs to evaluate both arguments
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: anba, Assigned: till)
Details
Attachments
(1 file, 1 obsolete file)
4.79 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
Test case:
---
"".lastIndexOf("a", {valueOf:function(){throw 0}})
---
Expected: second argument is evaluated and `0` is thrown
Actual: lastIndexOf() returns -1
Assignee | ||
Comment 1•12 years ago
|
||
Yeah, we're a bit over-eager, here. Easy enough to fix, though.
Attachment #788242 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•12 years ago
|
Assignee: general → till
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Reporter | ||
Comment 2•12 years ago
|
||
`"".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`.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #788242 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•9 years ago
|
Component: JavaScript Engine → JavaScript: Standard Library
Reporter | ||
Comment 4•8 years ago
|
||
New patch. Also added steps references and renamed variables to match current spec.
Attachment #788242 -
Attachment is obsolete: true
Attachment #8802257 -
Flags: review?(till)
Reporter | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
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.
Attachment #8802257 -
Flags: review?(till) → review+
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0a1da93428
Evaluate both arguments in String.prototype.lastIndexOf. r=till
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•