Add test for special case in FillWithUndefined (code in js/src/jsarray.cpp)

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla58
sec-want
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [adv-main57-][post-critsmash-triage])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
This particular special case in js/src/jsarray.cpp seems to be not covered by our test suite:

http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/src/jsarray.cpp#2082

anba made me a test for this that I'll land as part of this bug so we can cover this location better.

Filing this bug s-s because it potentially points to a an area of our codebase that lacks the proper testing. We should only unhide this bug if the affected code is unlikely to be s-s or once we landed proper test support.
(Assignee)

Comment 1

a year ago
Created attachment 8913242 [details] [diff] [review]
bug1403962.patch
Attachment #8913242 - Flags: review?(andrebargull)
Comment on attachment 8913242 [details] [diff] [review]
bug1403962.patch

LGTM!
Attachment #8913242 - Flags: review?(andrebargull) → review+
https://hg.mozilla.org/mozilla-central/rev/d1d32fa5437d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Might as well land this on Beta too to help the fuzzers.
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Whiteboard: [checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/d1bb539f6325
status-firefox57: affected → fixed
Whiteboard: [checkin-needed-beta]
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main57-]
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Out of curiosity, how did you conclude that the line was not covered? Looking at https://coveralls.io/builds/14179024/source?filename=js%2Fsrc%2Fjsarray.cpp#L2022 , it seems like it is. But perhaps you're only looking at the coverage from running... jit-tests? jit-tests+jstests? (I'm asking because I'd like to know whether test262 covers that line or not.)
Oh, wait. That coverage report would be from *after* this test landed...
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.