Closed
Bug 903391
Opened 11 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•11 years ago
|
||
Yeah, we're a bit over-eager, here. Easy enough to fix, though.
Attachment #788242 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•11 years ago
|
Assignee: general → till
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Reporter | ||
Comment 2•11 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•11 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
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf94147997f0676e8a12b2416eadd8dcc32676
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 |
https://hg.mozilla.org/mozilla-central/rev/6f0a1da93428
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
•